Refactoring the API calls

[psh]

In the process of refactoring related to the networking stack, I introduced a class ApacheHttpClientMediaWikiApi and felt the need to at least smoke test a few API calls to make sure that requests and responses are correctly handled. I used the MockWebServer (from OkHttp) to serve up canned responses and assert that requests are well formed at a unit-test level. This was already a transitive dependency from the mapbox library, made explicit in the build.gradle file.

My refactor pulls all of the API calls into a single place so that the body of the app depends on a MediaWikiApi interface returning application specific models, with a concrete ApacheHttpClientMediaWikiApi class that hides the implementation. This makes the code more testable (isolating the networking tests to a single class) and makes it easier to use mocks in the rest of the app if you want to unit test just the interactions later.

Although the testing isn't yet complete, I wanted to get the code into your hands as (IMO) it forms a solid basis to address your high-priority item #705 (upload tests). This PR doesn't include those tests, but it does aim to set you up for success in this endeavor. :)

[psh]

Well, that's disappointing - Travis CI timed out starting the emulator.

I notice in the build history this is happening a lot.

Want to maybe look at using a tool like Robolectric to be able to run the tests locally without an emulator?

[misaochan]

Thanks for the PR, @psh ! It's huge, please give us some time to look through it. ;)

[misaochan]

Hi @psh ,

I've tested this and it works well except for one (significant) problem - it crashes each time it attempts to finalize an upload. When the loading bar has completed, the app crashes with the error log:

07-10 19:36:21.971: E/AndroidRuntime(2252): FATAL EXCEPTION: UploadService
07-10 19:36:21.971: E/AndroidRuntime(2252): Process: fr.free.nrw.commons, PID: 2252
07-10 19:36:21.971: E/AndroidRuntime(2252): java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.String.equals(java.lang.Object)' on a null object reference
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at fr.free.nrw.commons.upload.UploadService.uploadContribution(UploadService.java:248)
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at fr.free.nrw.commons.upload.UploadService.handle(UploadService.java:130)
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at fr.free.nrw.commons.upload.UploadService.handle(UploadService.java:38)
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at fr.free.nrw.commons.HandlerService$ServiceHandler.handleMessage(HandlerService.java:25)
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at android.os.Handler.dispatchMessage(Handler.java:102)
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at android.os.Looper.loop(Looper.java:154)
07-10 19:36:21.971: E/AndroidRuntime(2252): 	at android.os.HandlerThread.run(HandlerThread.java:61)

The image upload actually does go through, e.g. https://commons.wikimedia.org/wiki/File:Fog_in_Brisbane.jpg , but it displays erroneously in the contributions view as a failed upload:

[misaochan]

Issue with upload - see thread comment

[codecov-io]

Codecov Report

Merging #763 into master will decrease coverage by 0.19%.
The diff coverage is 0.56%.

@@           Coverage Diff           @@
##           master   #763     +/-   ##
=======================================
- Coverage     6.8%   6.6%   -0.2%     
=======================================
  Files          94     99      +5     
  Lines        5073   5134     +61     
  Branches      460    465      +5     
=======================================
- Hits          345    339      -6     
- Misses       4700   4768     +68     
+ Partials       28     27      -1

Impacted Files	Coverage Δ
.../commons/contributions/ContributionController.java	0% <ø> (ø)	⬆️
.../java/fr/free/nrw/commons/upload/GPSExtractor.java	0% <ø> (ø)	⬆️
.../main/java/fr/free/nrw/commons/auth/LoginTask.java	0% <ø> (ø)	⬆️
...n/java/fr/free/nrw/commons/auth/LoginActivity.java	0% <ø> (ø)	⬆️
...main/java/fr/free/nrw/commons/WelcomeActivity.java	0% <ø> (ø)	⬆️
...ee/nrw/commons/media/MediaDetailPagerFragment.java	0% <ø> (ø)	⬆️
.../free/nrw/commons/upload/SingleUploadFragment.java	0% <ø> (ø)	⬆️
...free/nrw/commons/upload/MultipleShareActivity.java	0% <ø> (ø)	⬆️
...in/java/fr/free/nrw/commons/utils/LengthUtils.java	84.21% <ø> (ø)	⬆️
...a/fr/free/nrw/commons/upload/UploadController.java	0% <ø> (ø)	⬆️
... and 31 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0da5145...8e47f82. Read the comment docs.

[psh]

NPE fixed and the branch has been brought up to date with current master.

[whym]

It looks like my last patch #738 brought some conflicts here. Please free to ping me if you need help.

[misaochan]

Sorry, @psh , I missed your comment from 2 days ago. Should I go ahead and re-test the upload issue?

[psh]

The branch is now up-to-date with master. The conflicts were trivial (just imports) I deal with much worse on my team in work! A PR of this size is bound to run into them.

@misaochan - I gave uploading a quick smoke test, the NPE is fixed for the images I tried - please retest.

[misaochan]

Works great for me now, thanks @psh ! :)