-
Notifications
You must be signed in to change notification settings - Fork 129
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
Signing back in: fixes issues with outdated podcast information #1778
Conversation
@@ -65,7 +65,9 @@ class AuthenticationHelper { | |||
ServerSettings.refreshToken = response.refreshToken | |||
|
|||
// we've signed in, set all our existing podcasts to be non synced | |||
DataManager.sharedManager.markAllPodcastsUnsynced() | |||
if ServerSettings.lastSyncTime == nil { | |||
DataManager.sharedManager.markAllPodcastsUnsynced() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geekygecko Before merging this I'd like to double-check with you as I might be missing any user case.
The app will only mark all podcasts as unsynced if the user has never logged in to any account in this device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they have just signed in, the last sync should be nil already. 🤔
If they signed out for six months and subscribed to many podcasts, then signed in again, won't that mean those new podcasts won't appear on their other devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't that mean those new podcasts won't appear on their other devices?
Based on my tests it syncs just fine (as per the PR description)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and tested the scenario described and all worked correctly.
I do wonder why do we need to do step 4, Clear all folder information, the end user will not be able to do that, shouldn't this work correctly even without doing that?
Very good question, @SergioEstevao. I should have been more explicit in the PR description. if you test these same steps in This is to simulate the issue that happened that is reported in #791 (comment) A user won't be able to do that, but in the past, we had a bug that caused this scenario. A user with an old device with this "wrong data" can end up with their podcasts out of their folders, which is exactly what happened for this user. |
@SergioEstevao could you please review again? I added a feature flag so we can toggle this behavior in case it's needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All working great, and FF doing it's job!
Fixes #791 (comment)
We recently had an issue where a user had all their podcasts removed from folders when signing back in on another device.
This happened because they had outdated information and the app marked it as unsynced.
To test
With
onlyMarkPodcastsUnsyncedForNewUsers
enabledonlyMarkPodcastsUnsyncedForNewUsers
With
onlyMarkPodcastsUnsyncedForNewUsers
disabledonlyMarkPodcastsUnsyncedForNewUsers
SyncSigninViewController.swift:337
Checklist
CHANGELOG.md
if necessary.