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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reader: Fix tag stream wiping saved posts on pull-to-refresh #23331

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Jun 7, 2024

Cherry-picked from #23317

When performing a pull-to-refresh on the tag stream, the service wipes out all posts associated with the tag, and this could also delete saved posts (i.e., data loss).

Root cause: ReaderPostStreamService is used to fetch data for tag streams. When fetching data for the first page, the service wipes all data related to the tag, but there's no check in place for posts that are in use or saved.

fbf98ca solves this by skipping posts that are in use (inUse == true) or saved (isSavedForLater == true) when deleting posts associated to the tag. The fetch operation merges the new remote object with existing objects based on the globalID property (see ReaderPost.createOrReplace). Since inUse and isSavedForLater is purely a client-side property, it won't be overridden by the values from remote.

To test

Run the unit tests and ensure they pass 馃煝 . In addition, here are the manual testing steps for each scenario:

  1. Pre-requisite: Ensure that you at least follow one tag
  2. Launch the Jetpack app
  3. Navigate to Reader
  4. Go to the 'Your Tags' stream
  5. On one of the tags, scroll to the end and tap 'More'
  6. In the Tags stream page, tap the ellipsis button on the first post and tap 'Save'
  7. Perform a pull-to-refresh
  8. Find the post that you've saved before
  9. 馃攷 Verify that when tapping the ellipsis menu, the menu item says 'Remove Saved Post' (or the equivalent in your locale)
  10. Go to the 'Saved' stream from the stream switcher
  11. 馃攷 Verify that the post is visible in this stream

Regression Notes

  1. Potential unintended areas of impact
    Should be none.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually tested the changes

  3. What automated tests I added (or what prevented me from doing so)
    Added unit tests to cover the specific scenarios covered by this PR.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn鈥檛 complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

When the guard condition is not fulfilled, the success block is
called. A second success block is then called in the `completion`
closure.
@dvdchr dvdchr added Reader Core Data Issues related to Core Data labels Jun 7, 2024
@dvdchr dvdchr added this to the 25.1 milestone Jun 7, 2024
@dvdchr dvdchr requested review from wargcm and staskus June 7, 2024 17:46
@dvdchr dvdchr self-assigned this Jun 7, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Jun 7, 2024

1 Warning
鈿狅笍 This PR is assigned to the milestone 25.1 鉂勶笍. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 馃毇 Danger

@wpmobilebot
Copy link
Contributor

Jetpack Alpha馃摬 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23331-9e57051
Version25.0
Bundle IDcom.jetpack.alpha
Commit9e57051
App Center Buildjetpack-installable-builds #9168
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

WordPress Alpha馃摬 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23331-9e57051
Version25.0
Bundle IDorg.wordpress.alpha
Commit9e57051
App Center BuildWPiOS - One-Offs #10119
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@dvdchr dvdchr removed the request for review from staskus June 10, 2024 10:17
@dvdchr dvdchr modified the milestones: 25.1, 25.1 鉂勶笍 Jun 10, 2024
Copy link
Contributor

@wargcm wargcm left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

try mainContext.save()

// When
try await withCheckedThrowingContinuation { continuation in
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 馃挴, I like this over using an expectation.

@dvdchr dvdchr changed the base branch from trunk to release/25.1 June 10, 2024 16:44
@dvdchr dvdchr merged commit 0b22ad8 into release/25.1 Jun 10, 2024
30 of 31 checks passed
@dvdchr dvdchr deleted the fix/reader-tag-stream-data-loss-on-refresh branch June 10, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Data Issues related to Core Data Reader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants