Skip to content
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

Remove maximum number of posts that can be loaded in ReaderPostAdapter #20958

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

RenanLukas
Copy link
Contributor

Fixes #19811

sometimes posts are loaded above the last post (shifting content around)

I wasn't able to reproduce this part. I suspect it was fixed by this PR.

eventually posts stop loading.

By removing the mCanRequestMorePostsTemp logic (originally implemented in this PR) the unfollowed tag post list loaded a few more items, but eventually it stops. I thought it was something in the client, but this also happens in web which makes me think it's something in the back-end that eventually stops sending us more items.


To Test:

See the issue


Regression Notes

  1. Potential unintended areas of impact

    • None
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • Manual testing
  3. What automated tests I added (or what prevented me from doing so)

    • UI changes only

PR Submission Checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@RenanLukas RenanLukas marked this pull request as ready for review June 7, 2024 22:01
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 7, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20958-cb7260b
Commitcb7260b
Direct Downloadjetpack-prototype-build-pr20958-cb7260b.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 7, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20958-cb7260b
Commitcb7260b
Direct Downloadwordpress-prototype-build-pr20958-cb7260b.apk
Note: Google Login is not supported on these builds.

Copy link

sonarcloud bot commented Jun 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@thomashorta
Copy link
Contributor

thomashorta commented Jun 10, 2024

I believe most of the issues from the original GH issue were previously fixed thanks to your work on using dates instead of strings as well as some other minor changes we did throughout past Reader projects, so removing this limit should make sure we have an infinite list of posts as long as the user keeps scrolling and there are posts available in that feed. 🙇🏼

That said, I still think there's some issue going on on mobile, check the rest of this comment below:

I thought it was something in the client, but this also happens in web which makes me think it's something in the back-end that eventually stops sending us more items.

Not sure which tag you used, but could you have reached the last post from the tag you were testing?

I tested with a calm tag and on Mobile (with your changes) and I can't get past the post in the screenshot below. I opened the App Inspector and noticed in the DB that the date_published field looks pretty weird (2024-05-12T11:00:07-125:56). Look at the timezone segment. App inspector screenshot also below.

As mentioned in the original issue (#19811) for tag/topic feed, the field used for the before query parameter is date_published because of the logic here. So this could be causing the issue, though I'm not sure what the proper fix is.

App Last Post for calm tag/topic feed:

App Inspector (last post highlighted):
Screenshot 2024-06-10 at 14 16 17

On the Web I was able to get past it. Note that it looked stuck for a bit when I reached that same post, but after I scrolled up and down again, the posts loaded below it as expected. Also notice that the web says "Invalid Date" for the post date.
Screenshot 2024-06-10 at 14 20 00

Copy link
Contributor

@thomashorta thomashorta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at my previous comment, I ended up sending it without the Code Review, but it has all the information about what I investigated with your changes.

I think the changes as-is have value, removing this unnecessary soft-limit that exists on the Android Client, but there is still some issue going on related to fetching by date, though it does look like an issue in the data retrieved by the backend, so I'm not sure what we can do from a client-side. Maybe we can try to figure out what the web does, since in the web they seem to have some workaround that "invalid date" issue (see #20958 (comment)).

@RenanLukas
Copy link
Contributor Author

Thanks for the review and investigation, @thomashorta !

I agree this PR provides some value by removing the maximum number of posts to be loaded but it doesn't fully address the problem. Since it seems like the web is doing a workaround for invalid data returned by the back-end and we'll need to understand what they're doing, I propose we merge this PR and create a new issue/update the existing issue including your investigation. WDYT?

@thomashorta thomashorta removed the request for review from develric June 14, 2024 15:18
Copy link

sonarcloud bot commented Aug 28, 2024

@nbradbury nbradbury self-assigned this Aug 28, 2024
@nbradbury nbradbury self-requested a review August 28, 2024 16:11
Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously approved and tested fine after updating with trunk :shipit:

@nbradbury nbradbury merged commit 481cff8 into trunk Aug 28, 2024
21 checks passed
@nbradbury nbradbury deleted the issue/19811-paging-unfollowed-tags-not-working branch August 28, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reader] Pagination for unfollowed tags doesn't work properly
5 participants