-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
With media client APIs migrated to retrofit #2998
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments
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.
|
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. |
* 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
* 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
* 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
* 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
…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
No description provided.