-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Reader: Update Reader fetch followed sites to fetch all sites #22842
Conversation
Generated by 🚫 Danger |
a799104
to
02d58b1
Compare
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr22842-259c480 | |
Version | 24.5 | |
Bundle ID | org.wordpress.alpha | |
Commit | 259c480 | |
App Center Build | WPiOS - One-Offs #9315 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr22842-259c480 | |
Version | 24.5 | |
Bundle ID | com.jetpack.alpha | |
Commit | 259c480 | |
App Center Build | jetpack-installable-builds #8359 |
Hey @wargcm 👋 - I installed this PR by updating from an older one and then navigated to the Reader. As expected, the Subscriptions feed was already selected, since i was already in that feed in the old version of the app. However, despite waiting, the Site pill did not update. Attempting to pull to refresh did nothing. Even force closing the app and restarting it did not resolve the issue. |
if case .success(let sites) = result { | ||
allSites.append(contentsOf: sites) | ||
} |
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.
Hmm, I'm wondering what we should do if errors occur within 2...totalPages
. Currently, the errors are ignored, and this may lead to missing sites from the followed blogs list despite the successful result. But I'm not sure myself how we should address this without making it overly complicated... 😅
Some ideas: we could store the failed page numbers to retry. But in case of network errors, it could very well throw the user into a loop of multi-requests, especially considering that fetchAllFollowedSites
are getting called multiple times after the app launches.
Anyways, it's still better than just fetching the first page! 😄
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.
On Android, we're clearing the local sites list and replacing it with new sites fetched from the remote server. To keep the logic straightforward in this initial iteration, we've assumed that a network calls chain is successful only if all paged network calls are completed successfully. At the conclusion of the last call, we consider it a success, store the results locally, and emit an event to update the UI. If an issue arises in any of the calls, the entire chain is marked as a failure, and the local sites are NOT updated.
I suggest keeping the logic as simple as possible. Imagine you have 100+ sites in the app. The Sites pill indicates 100+ sites, and at some point, you perform a pull to refresh, or the app updates the sites upon opening the Reader. If an error occurs during a middle call, you will see fewer sites than before without a clear explanation.
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.
I'll update it to fail if one of the pages doesn't succeed. It'll just use the first error we encounter. Updated in: 648e6cf.
Which error does Android use if multiple of the calls fail? I could see the logic getting pretty complicated if we tried to take each into account.
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.
Similarly on Android, we log the first error encountered, even though we don't actually propagate the error to the UI.
public void onErrorResponse(VolleyError volleyError) {
AppLog.e(AppLog.T.READER, volleyError);
serverBlogs.clear();
taskCompleted(UpdateTask.FOLLOWED_BLOGS);
}
This is a separate issue with the chip not updating and is being addressed in: #22829.
We've noticed this sometimes occurs too, but we aren't sure why either. It's an existing issue that should be unrelated to these changes. We could probably add some sort of filter to that list to remove the empty sites, but I'm not sure if we ever run into a scenario where one of those empty sites actually has content. If we did, then hiding them wouldn't be ideal because then the user wouldn't be able to individually view that site. |
9aa9fc6
to
648e6cf
Compare
Previously, the API call was only returning the first page of a user's followed sites. This change adds support for a page parameter and uses that to fetch all the pages at the same time. Author: Chris McGraw <2454408+wargcm@users.noreply.github.com>
648e6cf
to
259c480
Compare
Heads up: with 259c480 I've updated the Since it's been pre-approved before, I'll set this PR on auto-merge on behalf of @wargcm. |
Fixes #15651
WordPressKit PR: wordpress-mobile/WordPressKit-iOS#753
Description
Previously, the API call was only returning the first page of a user's followed sites. This change adds support for a page parameter and uses that to fetch all the pages at the same time.
Testing
I found the easiest way to test it is by reducing the page size and adding print statements to the new function when it fetches pages. You can copy this code block to make it easier:
Test code
To test:
Subscriptions
feed in the ReaderBlogs
chipEdit
Regression Notes
Potential unintended areas of impact
Should be none.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: