Use JSON APIs for explore #2731
Merged
Conversation
This file contains hidden or 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
7c2ab06 to
8dc8baf
Compare
Codecov Report
@@ Coverage Diff @@
## master #2731 +/- ##
=========================================
+ Coverage 2.72% 2.8% +0.07%
=========================================
Files 267 267
Lines 12801 12758 -43
Branches 1137 1125 -12
=========================================
+ Hits 349 358 +9
+ Misses 12426 12375 -51
+ Partials 26 25 -1
Continue to review full report at Codecov.
|
Help neededI am stuck at making mockwebserver + okhttp tests work. I have added a It would be great if someone can help me with it. @ashishkumar468 @domdomegg |
ashishkumar468
added a commit
to ashishkumar468/apps-android-commons
that referenced
this pull request
Mar 24, 2019
* Increased sdk version to 23
|
I think @ashishkumar468 has fixed it. I've noticed something interesting - the ordering that it shows category images in is different (these are consistently the first thing on the beta featured page):
There doesn't seem to be much of a reason for their ordering - previously I believe they were in reverse order of when they were added to the category (i.e. so you'd see the most recently added ones first - perfect for explore, especially featured and uploaded via mobile). Is this something we could fix? |
|
Also, I have no idea what the conditions are for this (sorry!), but now I sometimes get this error when navigating to explore: |
|
I seem to have got the app into some state where this always happens. Not sure how. However clearing the data makes it work, and then after a few more times going around explore it'll start to crash with this error again. More logs |
|
@domdomegg Do you also have access to logs just before this fatal crash. I want to take a look at the response from the API. My guess is that some of the fields for a particular image might be not present. |
This is happening on prod build, right? |
Full logs |
I've just been testing on Beta so far - will test on prod now. Logs above are all from beta. |
On prod it seems to get random images each time instead of having any ordering - this might have been the case on beta too actually. Going to try messing around until it crashes now. |
|
Figured it out - and it makes sense why beta only was crashing. Explore crashes now at the end of the category - on beta the featured category is short enough that it reaches the end and crashes. On prod this can be reproduced by viewing a small category, or when searching for a nonsense term (as there are no results) |
Prod crash (searching for 'domdomegg') |
Thanks a lot for testing it in so much detail. I will fix all of these issues and update the PR. :) |
057f1e0 to
543277c
Compare
|
@domdomegg I have fixed the crash and other issues reported by you. Also, I have added unit tests for the changes I have made. |
|
Tested I'm happy to merge in it's current state, but do still wonder about why the ordering has changed. Is there any way to get the old behaviour (sorted by most recently added by category), or close (e.g. sorted by most recently uploaded)? |
domdomegg
approved these changes
Mar 25, 2019
|
I am sending parameters to sort by IMO, we can merge this and create an upstream issue to get it fixed in the backend. |
|
Merging this one to master, @maskaravivek Can you create a separate issue for the upstream ? |
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.
Description (required)
Fixes #2729
What changes did you make and why?
Tests performed (required)
Tested explore and search until several pages in prodDebug.