Up Next Queue Syncing: Attempt to merge the local and remote device changes to prevent items from the queue being removed or cleared #661

emilylaguna · 2023-01-21T01:44:54Z

Part of #24

Note: The majority of the line changes are actually me adding a ton of documentation and logging.

Description

This attempts to solve the instances where a users up next queue can be incorrectly cleared, or replaced with the contents of an outdated account. For more info on the scenarios that this could occur see this comment.

In order to prevent a users queue from being removed we now apply different logic based on the context ("reason") that a sync is occurring.

Account Creation

If we are syncing because due to a new account being created, we treat the local queue as the source of truth and it is then persisted to the server.

Logging In

If we are syncing because the user is logging into an existing account we apply a non-destructive merge:

Retrieve the latest up next queue from the server
Add all the remote items to the users local queue
Ensure any local episodes that were not on the server are not removed
Persist the "merged" changes to the server

Everything else

We apply the same logic as we did before.

Known issues with this approach

Removed episodes reappearing

Since during a login we only add items to the queue, and never remove them a user who has previously removed episodes from their queue may have them added back in if they login on a device that is not up to date.

However, I think this is a worthy trade off to deleting or replacing users queues entirely.

Sort order not persisting for logged out users

During a login the latest server changes are applied to the episodes in the queue which means any local episodes that were added may appear out of their original order.

Again, I think this is an okay trade off.

Demo Videos

Installing the app from scratch and logging into an existing account that has a queue

✅ Expectation: The existing users queued will be persisted on the new installation.

new-install-logged-in.mov

Logged out user with queued items, logging into existing account with a queue

✅ Expectation: The logged out and existing queued items will be merged.

logged-out-to-in.mov

Logged out user with queued items, logging into an account that has no queue

✅ Expectation: The logged out queued items will appear on the account

logged-in-empty.mov

Logged out user with items that were removed on the other device

✅ Expectation: The deleted items will be restored when the logged out device logs back in

issue-logged-out-removed.mov

To test

Account Creation

Reinstall the app
Dismiss the login prompt
Add some items to your up next queue
Create a new account
✅ Verify your local queue remains the same as it was
Sign into the new account on another device
✅ Verify the correct queue is sync'd

Installing the app from scratch and logging into an existing account that has a queue

Sign into an account on device 1
Add some items to your up next queue and sync them
Reinstall the app on device 2
When prompted tap login
Sign into the device 1 account
✅ Verify the correct queue is added to your device
Go to Device 1, and perform a sync
✅ Verify the queue remains the same

Being a logged out user with items added to the up next, and logging into an account that has no items in the queue

Sign into an account on device 1
Clear your queue
Reinstall the app on device 2
Dismiss the login prompt
Add some items to the queue
Sign into the device 1 account
✅ Verify your local queue remains the same
Go to Device 1, and perform a sync
✅ Verify the device 2 queue is moved over

Being a logged out user with items added to the up next, and logging into an account that has existing items in the queue

Sign into an account on device 1
Add some items to your up next queue and sync them
Reinstall the app on device 2
Dismiss the login prompt
Add some items to the queue
Sign into the device 1 account
✅ Verify the queue consists of the items you added and the items from device 1
Go to Device 1, and perform a sync
✅ Verify you see both device 1 and 2's queue

Being a logged out user who removed items from their queue, and logging into an account where those changes weren't persisted

Sign into an account on device 1
Add some items to your up next queue and sync them
Reinstall the app on device 2
Sign into device 1's account
Then sign out
Remove some items from the queue
Sign back into device 1's account
✅ ⚠️ The items that you removed will be added back to device 2 (See known issues above)

Being a logged in user both devices and syncing changes

Sign into an account on device 1
Add some items to your up next queue and sync them
On Device 2, Sign into device 1's account
On Device 1, make some changes to the queue (add, remove, move, etc)
Sync the changes
On device 2, refresh
✅ The queue is updated correctly
On device 2, make some changes to the queue.
Sync them
On device 1, refresh
✅ Verify the changes are sync'd back correctly.

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.

…diately

…nges to the sync task

emilylaguna · 2023-01-21T01:48:27Z

Modules/Server/Sources/PocketCastsServer/Public/Sync/UpNextSyncTask.swift

+        // to us, and we will attempt to merge the changes in the `applyServerChanges` call below
+        //
+        if ServerSettings.syncReason != .login {
+            // replace action


The code block here is the same as it was before, it's just been wrapped in the if check

emilylaguna · 2023-01-21T01:55:05Z

podcasts/PlaybackQueue.swift

-        startSyncTimer()
+
+        // Persist the copy on the server immediately.
+        RefreshManager.shared.syncUpNext()


Normally this would fire a sync after a delay, however since this is only used while we're already syncing I think we should trigger it immediately.

emilylaguna · 2023-01-21T01:55:22Z

podcasts/AuthenticationHelper.swift

@@ -56,6 +56,7 @@ class AuthenticationHelper {
        // we've signed in, set all our existing podcasts to be non synced
        DataManager.sharedManager.markAllPodcastsUnsynced()

+        ServerSettings.syncReason = .login


Will need to update this for SSO new accounts / logged in accounts as well.

mchowning · 2023-01-23T15:45:23Z

Modules/Server/Sources/PocketCastsServer/Public/Sync/UpNextSyncTask.swift

+        // To prevent this, during a login we no longer send any local changes to the server, instead we pull the latest sync'd
+        // up next queue and attempt to merge the changes later in the "applyServerChanges" call.


...we no longer send any local changes to the server, instead we pull the latest sync'd up next queue and attempt to merge the changes

FWIW, it looks like Android already wasn't sending the local changes to the server until after it merged the queues, so this aligns well (Android's problem was that it was "merging" the server and local queues by removing any locally queued episodes that weren't in the server queue 😬 ).

leandroalonso

@emilylaguna I have a concern regarding the known issues. When we think about data loss, it indeed pays off. But what if those issues start happening much more than the current data loss issue?

I believe there's no way to know that and we'll need to test and see how it goes. In the meantime, it would be interesting to give support all this context beforehand.

If we start receiving complaints about those issues we might need to rethink this approach.

leandroalonso · 2023-01-23T16:12:52Z

Modules/Server/Sources/PocketCastsServer/Public/Sharing/Structs/ServerConstants.swift

+    public enum SyncingReason: String {
+        case accountCreated
+        case login
+    }


I don't think this belongs to ServerConstants. This is used only by the client, right?

This induces me to think that the sync reason is also given to the server, which is not true.

I believe we can declare that outside of the ServerConstants and stating that this is for internal use.

I moved it into the SyncManager here: 9ecdec1

I figured this was a better choice since contextually it goes hand in hand, and it already has a isFirstSyncInProgress helper function.

emilylaguna · 2023-01-24T00:51:05Z

When we think about data loss, it indeed pays off. But what if those issues start happening much more than the current data loss issue?

This is a a very valid point. I tried to mitigate this by reducing the instances where the merging takes place to just a login action. However like you said, it is impossible to know before hand how many people will encounter this and if it's more than the people who were losing their data.

Once this is merged in I'll write a P2 describing the issue, the fix, and make sure that support is also aware of this.

If we start receiving complaints about those issues we might need to rethink this approach.

100% agree.

…is pr

emilylaguna added 14 commits January 20, 2023 15:59

Saves the changes to the users up next queue even if they're logged out

a97fdd7

Add a way to track what kind of sync is being performed

c7878cd

Track the syncing reason when creating an account or logging in

ce3f192

Add some more detailed logs to the up next sync process

ac697b0

Add a helper variable to get the enum value for the UpNextChanges

a8930ee

Revert some unnecessary changes

23a284e

Remove the syncing reason user defaults persistence

41745d5

When persisting the local up next queue, trigger an up next sync imme…

8c6d664

…diately

When syncing the up next because of a login, don't send any local cha…

c8d42ca

…nges to the sync task

When creating a new account, always persist the local queue

84588b5

Fix some typos

0289f2d

Add missing commit lines

3cb152f

Update UpNextSyncTask.swift

fa3b85e

Send the merged changes to the server to be saved

870ed6d

emilylaguna added [Type] Bug Used for issues where something is not functioning as intended. Up Next issues related to the Up Next feature sync Issues related to sync between platforms labels Jan 21, 2023

emilylaguna requested a review from a team as a code owner January 21, 2023 01:44

Update CHANGELOG.md

5ff5e83

emilylaguna changed the title ~~Change how the up next queue~~ Up Next Queue Syncing: Attempt to merge the local and remote device changes to prevent items from the queue being removed or cleared Jan 21, 2023

Removed unused item

8d9bf34

emilylaguna commented Jan 21, 2023

View reviewed changes

Make sure the UI update notifications are posted when merging

6772b57

emilylaguna commented Jan 21, 2023

View reviewed changes

emilylaguna added this to the 7.31 milestone Jan 21, 2023

emilylaguna added 2 commits January 20, 2023 21:09

Update comment

04eb4c6

Update comments

4713f37

mchowning reviewed Jan 23, 2023

View reviewed changes

leandroalonso approved these changes Jan 23, 2023

View reviewed changes

emilylaguna changed the base branch from trunk to release/7.31 January 24, 2023 00:31

emilylaguna added 2 commits January 23, 2023 19:45

Move the sync reason to the SyncManager

9ecdec1

Merge branch 'release/7.31' into issue/24-fix-up-next-queue-clearing

df6f781

emilylaguna added 2 commits January 23, 2023 20:22

Improve wording of the account creation comment in the UpNextSyncTask

148f24d

Return to using the sync timer to prevent the amount of changes in th…

439227e

…is pr

emilylaguna merged commit f7e3cc4 into release/7.31 Jan 25, 2023

emilylaguna deleted the issue/24-fix-up-next-queue-clearing branch January 25, 2023 00:39

emilylaguna mentioned this pull request Feb 4, 2023

Up Next: Queue Emptied Unexpectedly #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Up Next Queue Syncing: Attempt to merge the local and remote device changes to prevent items from the queue being removed or cleared #661

Up Next Queue Syncing: Attempt to merge the local and remote device changes to prevent items from the queue being removed or cleared #661

emilylaguna commented Jan 21, 2023 •

edited

Loading

emilylaguna Jan 21, 2023

emilylaguna Jan 21, 2023

emilylaguna Jan 21, 2023

mchowning Jan 23, 2023 •

edited

Loading

leandroalonso left a comment

leandroalonso Jan 23, 2023

emilylaguna Jan 24, 2023

emilylaguna commented Jan 24, 2023

		// To prevent this, during a login we no longer send any local changes to the server, instead we pull the latest sync'd
		// up next queue and attempt to merge the changes later in the "applyServerChanges" call.

Up Next Queue Syncing: Attempt to merge the local and remote device changes to prevent items from the queue being removed or cleared #661

Up Next Queue Syncing: Attempt to merge the local and remote device changes to prevent items from the queue being removed or cleared #661

Conversation

emilylaguna commented Jan 21, 2023 • edited Loading

Description

Account Creation

Logging In

Everything else

Known issues with this approach

Removed episodes reappearing

Sort order not persisting for logged out users

Demo Videos

Installing the app from scratch and logging into an existing account that has a queue

Logged out user with queued items, logging into existing account with a queue

Logged out user with queued items, logging into an account that has no queue

Logged out user with items that were removed on the other device

To test

Account Creation

Installing the app from scratch and logging into an existing account that has a queue

Being a logged out user with items added to the up next, and logging into an account that has no items in the queue

Being a logged out user with items added to the up next, and logging into an account that has existing items in the queue

Being a logged out user who removed items from their queue, and logging into an account where those changes weren't persisted

Being a logged in user both devices and syncing changes

Checklist

emilylaguna Jan 21, 2023

Choose a reason for hiding this comment

emilylaguna Jan 21, 2023

Choose a reason for hiding this comment

emilylaguna Jan 21, 2023

Choose a reason for hiding this comment

mchowning Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

leandroalonso left a comment

Choose a reason for hiding this comment

leandroalonso Jan 23, 2023

Choose a reason for hiding this comment

emilylaguna Jan 24, 2023

Choose a reason for hiding this comment

emilylaguna commented Jan 24, 2023

emilylaguna commented Jan 21, 2023 •

edited

Loading

mchowning Jan 23, 2023 •

edited

Loading