With media client APIs migrated to retrofit #2998
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
@maskaravivek you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
At a high level this refactor looks clean to me. There seems to be some reused strings across files that would benefit from being in a constants folder.
Reviewed with ❤️ by PullRequest
| @@ -35,7 +40,7 @@ public MediaDataExtractor(MediaWikiApi mwApi, | |||
| */ | |||
| public Single<Media> fetchMediaDetails(String filename) { | |||
| Single<Media> mediaSingle = getMediaFromFileName(filename); | |||
| Single<Boolean> pageExistsSingle = mediaWikiApi.pageExists("Commons:Deletion_requests/" + filename); | |||
| Single<Boolean> pageExistsSingle = mediaClient.doesPageExist("Commons:Deletion_requests/" + filename); | |||
There was a problem hiding this comment.
"Commons:Deletion_requests" seems to be used multiple times throughout the code. Perhaps a there should be a constants file defining these strings.
There was a problem hiding this comment.
Will take this up in another PR. Shouldn't be clubbed with this PR. :)
app/src/main/java/fr/free/nrw/commons/media/MediaInterface.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2998 +/- ##
=========================================
+ Coverage 4.43% 4.55% +0.11%
=========================================
Files 259 260 +1
Lines 12261 12261
Branches 1051 1051
=========================================
+ Hits 544 558 +14
+ Misses 11678 11664 -14
Partials 39 39
Continue to review full report at Codecov.
|
|
I get such build fails, does it seem related to you @maskaravivek ? |
|
|
This appears on clicking next button @maskaravivek |
ashishkumar468
approved these changes
Jun 17, 2019
|
@neslihanturan Can you share logs. It is working for me. I entered the same title and description.
|
|
Here is logs I see on click next button: |
|
@neslihanturan Can you share verbose level logs. Is the network request/response printed in the logs? |
|
@neslihanturan I found the issue. I wasn't using the right build. It is reproducible for me as well. :) |
|
I should have say it is beta debug:) |
|
@neslihanturan Thanks a lot for identifying the issue. :) I have fixed the issue now. |
|
Hi @maskaravivek , currently tests have failed. Do you know why? |
|
Yes after updating the data-client version something else broke. :( I have created an upstream PR to get it reviewed by @dbrant. If we get the properties back, I will bump the library version and fix this build. |
|
I think testing before your update will be safe, because I assume your changes from now on will not effect functionality. However, I can wait for manual testing until automated tests fixed too. |
|
@neslihanturan The PR is finally ready for testing. :) |
|
Hi @maskaravivek , I started to test it:
This irrelevant looking log keeps coming: Verbose logs are also very fast. Can you guide me about what to filter, Okhttp maybe? Retry button doesn't seem like to be effective. Note: happened on API 24 emulator, betaDebug |
|
Any other problem I experienced:
|
neslihanturan
requested changes
Jun 19, 2019
|
Additionally, I have tested peer review too. Unfortunately, none of the questions except mis-category question have an effect, like as in master. |
Also, I haven't made changes in how the warnings are shown. Instead of calling the XML API, I am simply calling the JSON API. Does this work on master?
Yes, this I will fix in a separate PR. |
Unfortunately it is not a reproducible fail, do you think it was our known upload fail issue? If you guarantee it can't be related with your PR I am okay.
I just tested, and same on master.
Sorry I have missed your previous explanation inside parenthesis |
Without the logs i cannot guarantee but as I have not touched the upload logic so I am fairly confident. |
|
Okay then I think we can merge this since it does requirements. Let's request a test from @misaochan or @ashishkumar468 and if they don't experience any upload fails, so we can go ahead and merge this. |
|
Thanks, @neslihanturan. If the PR looks okay then can you approve it? :) @ashishkumar468 @misaochan Can you test the PR once and see if it works fine for you? |
|
Just tested this on API 27 emulator, uploading from Google Photos. My upload failed as well. Additionally, upload count fetching failed (logs below). I logged out and logged in again to a different account. This time the upload succeeds. Upload count fetching still fails. @maskaravivek @neslihanturan How about creating a So I think you can submit your PR to that branch and we can merge it so you can continue your work. Before we merge the branch into master, we'll test for upload failures again. The upload failures would need to have been fixed at that point anyway, so any upload failures would be cause for concern at that point regardless of the reason. |
neslihanturan
approved these changes
Jun 20, 2019
|
At least @misaochan was able to keep logs. Maybe it can be easier to understand its reason. Good to note that I very rarely experience upload fails with my test emulator. Additionally I agree to @misaochan about moving to another branch. |
maskaravivek
added a commit
to maskaravivek/apps-android-commons
that referenced
this pull request
Jul 6, 2019
* With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests
maskaravivek
added a commit
to maskaravivek/apps-android-commons
that referenced
this pull request
Jul 7, 2019
* With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests
maskaravivek
added a commit
to maskaravivek/apps-android-commons
that referenced
this pull request
Jul 7, 2019
* With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests
maskaravivek
added a commit
to maskaravivek/apps-android-commons
that referenced
this pull request
Jul 10, 2019
* With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests
maskaravivek
pushed a commit
that referenced
this pull request
Jul 15, 2019
…Client (#3056) * With media client APIs migrated to retrofit (#2998) * With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests * Categories related client API's migrated to retrofit (#3053) * Commit 1 * searchCategories migrated to retrofit * SearchCategoriesFragment migrated to new API * Removed unused code * Created tests * implemented searching by prefix fixed SearchCategoryFragment behaviour where the same categories would be added to the list instead of new ones. * added tests * Migrated searchTitles to searchCategories, function behaviour seems identical * With media client APIs migrated to retrofit (#2998) * With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests * Categories related client API's migrated to retrofit (#3053) * Commit 1 * searchCategories migrated to retrofit * SearchCategoriesFragment migrated to new API * Removed unused code * Created tests * implemented searching by prefix fixed SearchCategoryFragment behaviour where the same categories would be added to the list instead of new ones. * added tests * Migrated searchTitles to searchCategories, function behaviour seems identical * OkHttpJsonApi#getMediaList migrated to retrofit (#3054) * Migrated OkHttpJsonApi#getMediaList partially to MediaClient#getCategoryImages * Migrated rest of OkHttpJsonApi#getMediaList functionality to MediaClient#getCategoryImages * Removed unused code and tests * Fixed small bug * Added tests * Removed unused CategoryImageController * getSubCategoryList and getParentCategoryList migrated to retrofit (#3055) * Migrated getSubCategoryList to retrofit * Migrated getParentCategoryList to retrofit * Removed obsolete functions * Added tests * Fixed small bugs * Migrated OkHttpJsonApiClient#getMedia and getPictureOfTheDay to MediaClient * Removed obsolete functions and added tests * Fixed merge errors * With media client APIs migrated to retrofit (#2998) * With media client APIs migrated to retrofit * Add test cases and java docs * Fix test * Fix build * Fix build and other minor issues * Fix tests * Categories related client API's migrated to retrofit (#3053) * Commit 1 * searchCategories migrated to retrofit * SearchCategoriesFragment migrated to new API * Removed unused code * Created tests * implemented searching by prefix fixed SearchCategoryFragment behaviour where the same categories would be added to the list instead of new ones. * added tests * Migrated searchTitles to searchCategories, function behaviour seems identical * OkHttpJsonApi#getMediaList migrated to retrofit (#3054) * Migrated OkHttpJsonApi#getMediaList partially to MediaClient#getCategoryImages * Migrated rest of OkHttpJsonApi#getMediaList functionality to MediaClient#getCategoryImages * Removed unused code and tests * Fixed small bug * Added tests * Removed unused CategoryImageController * getSubCategoryList and getParentCategoryList migrated to retrofit (#3055) * Migrated getSubCategoryList to retrofit * Migrated getParentCategoryList to retrofit * Removed obsolete functions * Added tests * Fixed small bugs * Consume login client from data client library (#2894) Fix actions for review client Use data client library for notifications With delete helper migrated to data client With wikidata edits With notifications and modifications migrated to data client With upload migrated to retrofit Delete unused code Reuse thank interface from the library
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.