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

[Reader] Fix tag posts list paging #19881

Conversation

RenanLukas
Copy link
Contributor

@RenanLukas RenanLukas commented Jan 3, 2024

Fixes #19879

We were sorting the tag related blog posts comparing date_published as a string instead of a date. This was causing paging not to work properly.

I've replaced ...ORDER BY date_published... with ...ORDER BY datetime(date_published)... in our query so the posts are sorted as dates correctly.

Before

SELECT <columns> FROM tbl_posts WHERE tag_name=? AND tag_type=? ORDER BY date_published DESC
Screen.Recording.2024-01-03.at.8.02.56.PM.mov

After

SELECT <columns> FROM tbl_posts WHERE tag_name=? AND tag_type=? ORDER BY datetime(date_published) DESC
Screen.Recording.2024-01-03.at.7.09.23.PM.mov

To Test:

Please test this change with different SQLite versions. Also compare the results with web to make sure now we're displaying the same results using the same order.

  • Install JP and sign-in
  • Open "Reader"
  • Select a tag
  • Start scrolling: it will stop loading new posts at some point, even though there are many more (compare with web)

Another way to test is opening the tag posts list from blogging prompts card:

  • Install JP and sign-in
  • Open "My Site"
  • Tap on "View all responses" in blogging prompts card
  • Start scrolling: it will stop loading new posts at some point, even though there are many more (compare with web)

Regression Notes

  1. Potential unintended areas of impact

    • SQLite function datetime(...) might not available in every version?
  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)

    --


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.

UI Changes Testing Checklist:

  • 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 January 3, 2024 22:30
@RenanLukas
Copy link
Contributor Author

There are other places where we might also be comparing the strings instead of dates (example). Might be worth to investigate them when we can.

@RenanLukas
Copy link
Contributor Author

RenanLukas commented Jan 3, 2024

This might solve the inserted items animation issue that made us revert the updated fetched tag posts limit. I'm submitting a PR with that change again since the performance gain was meaningful.

You might still see items being moved only the first time it loads if you already had posts for that tag in the local database.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 3, 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
Versionpr19881-abc64da
Commitabc64da
Direct Downloadjetpack-prototype-build-pr19881-abc64da.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 3, 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
Versionpr19881-abc64da
Commitabc64da
Direct Downloadwordpress-prototype-build-pr19881-abc64da.apk
Note: Google Login is not supported on these builds.

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.

Thanks for this PR, this issue has been happening to several tags and feeds and it looks like you found one of the main root causes! 🙇🏼

I added a few suggestions regarding other date fields and other queries using sorting by date, maybe we want to fix them too.

Just a note: it's not possible to test this specific fix with the "View all responses" from Blogging Prompts after syncing this branch with trunk, since trunk already had a fix for the prompts order issue (#19730) that solved it by making the getSortColumnForTag function use the date_tagged column (which seems to be normalized in terms of timezone).

@@ -751,7 +751,7 @@ private static String getSortColumnForTag(ReaderTag tag) {
} else if (tag.isTagTopic() || tag.isBookmarked()) {
return "date_tagged";
} else {
return "date_published";
return "datetime(date_published)";
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ So it seems like the issue is that we store this date as a string but it has format like "2024-01-08T14:42:19+00:00", and not all posts have the same timezone, causing the order to be inconsistent since the backend actually treats the field as a date. This is a really nice catch, thanks!

💡 Also, since this is the issue, I think it makes sense to add datetime around the other date-related cases above (lines 746, 748, and 752), since they would suffer the same problem.

💡 Do you think it also makes sense to use the datetime function on other queries in this file that use a date as order clause? (here, here, here, here, and here)

Copy link
Contributor Author

@RenanLukas RenanLukas Jan 8, 2024

Choose a reason for hiding this comment

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

Thanks for the review, @thomashorta !

💡 Also, since this is the issue, I think it makes sense to add datetime around the other date-related cases above (lines 746, 748, and 752), since they would suffer the same problem.

💡 Do you think it also makes sense to use the datetime function on other queries in this file that use a date as order clause? (here, here, here, here, and here)

I agree 100%, but this also means we'd have to test every single scenario that relies on the logic you've mentioned in this same PR, right? Since this already solves the original tag post list issue, does it make sense to create a separate issue for the other places where we can thoroughly investigate and test the new changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, let's make the datetime change for the other places in a different PR then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Jan 8, 2024
@dangermattic
Copy link
Collaborator

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

Generated by 🚫 Danger

@RenanLukas RenanLukas merged commit f0a33fb into feature/reader-ia Jan 8, 2024
19 checks passed
@RenanLukas RenanLukas deleted the issue/19879-tag-posts-list-not-fetching-new-pages branch January 8, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants