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: Add remote fetching for the tags stream #23076

Merged
merged 2 commits into from Apr 25, 2024

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Apr 24, 2024

Part of #23069
Depends on #23057

Adds fetching logic to the 'Your Tags' stream. Note that the loading state will be addressed in the next PR.

In this PR, I had to add a variable on ReaderStreamViewController to track whether we've remotely synced posts for a certain tag. This is done to prevent infinite looping from the remote request and the cell being reloaded. For example:

  1. Cell is loaded.
  2. Cell fetches data remotely.
  3. New posts are saved into context.
  4. WPTableViewHandler receives a didChange notification and triggers a reload (because ReaderPost has a relation with ReaderAbstractTopic.)
  5. Cell is reloaded. Go back to step 2.

The variable, tagStreamSyncTracker, is added to ensure that we only fetch once per tag. This doesn't completely remove the unnecessary reloading, but it does limit and prevent it from reloading indefinitely. Also, I've added a note that this is a temporary workaround. The proper way would be to create a new view controller conforming to ReaderContentViewController protocol and should no longer be driven by a FetchedResultsController.

To test

Tip

To make it easy to verify, you could add a print statement in ReaderTagCardCellViewModel:L80, right below the TODO for the loading state:

    // TODO: Add loading state.
    print("🗒️ Remotely fetching for: \(tag.slug)")
  • Go to the Reader tab and select 'Your Tags' stream.
  • 🔎 Verify that new contents are fetched once per tag.
    • If you've added the debug print above, verify that 🗒️ Remotely fetching for: \(tag.slug) appears in the console as you scroll down the list.
    • When scrolling up, also verify that 🗒️ Remotely fetching for: \(tag.slug) no longer appears for tags that have previously fetched data remotely.
  • Perform a pull-to-refresh.
  • 🔎 Verify that the tags are re-fetching the contents.
  • Navigate to a different stream, and then come back to 'Your Tags'.
  • 🔎 Verify that the tags are re-fetching the contents.

Regression Notes

  1. Potential unintended areas of impact
    Should be none. Changes are isolated to the new stream.

  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)
    N/A.

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’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@dvdchr dvdchr added this to the 24.8 milestone Apr 24, 2024
@dvdchr dvdchr requested a review from wargcm April 24, 2024 13:00
@dvdchr dvdchr self-assigned this Apr 24, 2024
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 24, 2024

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 Numberpr23076-a479a19
Version24.7
Bundle IDcom.jetpack.alpha
Commita479a19
App Center Buildjetpack-installable-builds #8705
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 24, 2024

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 Numberpr23076-a479a19
Version24.7
Bundle IDorg.wordpress.alpha
Commita479a19
App Center BuildWPiOS - One-Offs #9661
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

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:

I found a few issues while testing, but they're unrelated to your changes. The issues I found are:

  • Gap markers appearing in the tags posts list (I added a commit to fix this in my previous PR: 42c455b)
  • Reused cells with collection views maintaining their current scroll position (I fixed this locally, will create a PR in the future)
  • An API call failure from the fetch menu request would recreate the stream view controller and cause it to blank out (I fixed this locally, will create a PR in the future)

@dvdchr dvdchr added the Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. label Apr 25, 2024
Base automatically changed from issue/23013-add-tag-actions to trunk April 25, 2024 14:01
@dvdchr dvdchr merged commit a4b6a03 into trunk Apr 25, 2024
26 of 27 checks passed
@dvdchr dvdchr deleted the issue/23069-fetch-tag-posts branch April 25, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. Reader [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants