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

Use Episode IDs from the sync task response instead of our local ones #2448

Merged
merged 7 commits into from
Nov 19, 2024

Conversation

bjtitus
Copy link
Contributor

@bjtitus bjtitus commented Nov 16, 2024

Uses the UDIDs of the episodes from the sync response instead of re-querying for the unsynced episodes. This fixes an issue where users with 2000+ episodes in a background sync could send the same episodes during background sync repeatedly.

  • Adds a useSyncResponseEpisodeIDs feature flag in case we need to disable
  • Adds duplicate method for marking episodes synced with their uuid instead of the local id
  • Updates processServerData to use these uuids when handling the server response

Unit Tests

  • Adds unit tests to test the new update behavior from the server response

To test

  • Build and run the app
  • Subscribe to some podcasts
  • Add several episodes
  • Mark several episodes as archived
  • Refresh via the profile or kill and relaunch the app
  • Make sure the sync occurs. You should see some logs in the console:
SyncTask: sending 6 changed items to the server

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

@CookieyedCodes
Copy link

Will this help fix my issue Automattic/pocket-casts-android#628?

@bjtitus
Copy link
Contributor Author

bjtitus commented Nov 18, 2024

@CookieyedCodes I doubt it. How many subscriptions do you have? This would mostly affect large subscription counts where synced episodes routinely exceed 2000.

@CookieyedCodes
Copy link

I have 2111 feeds 😅,

@bjtitus bjtitus marked this pull request as ready for review November 18, 2024 23:20
@bjtitus bjtitus requested a review from a team as a code owner November 18, 2024 23:20
@bjtitus bjtitus requested review from danielebogo and removed request for a team November 18, 2024 23:20
@bjtitus bjtitus added the [Type] Bug Used for issues where something is not functioning as intended. label Nov 18, 2024
@bjtitus bjtitus changed the base branch from release/7.77 to trunk November 18, 2024 23:25
@bjtitus bjtitus added this to the 7.78 milestone Nov 18, 2024
Copy link
Contributor

@danielebogo danielebogo left a comment

Choose a reason for hiding this comment

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

It works!
@bjtitus QQ, do we want this in 7.78 or 7.77?

@bjtitus
Copy link
Contributor Author

bjtitus commented Nov 19, 2024

I initially targeted 7.77 but thought:

  • It's a little late in the cycle for the full testing
  • We don't have specific user reports that this is actually causing issues

But it is flagged, so I'm open to either.

@danielebogo
Copy link
Contributor

@bjtitus I'm ok with 7.78

# Conflicts:
#	Modules/Utils/Sources/PocketCastsUtils/Feature Flags/FeatureFlag.swift
@bjtitus bjtitus merged commit 75eec42 into trunk Nov 19, 2024
4 of 6 checks passed
@bjtitus bjtitus deleted the bjtitus/sync-response-episode-ids branch November 19, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Used for issues where something is not functioning as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants