Skip to content
This repository has been archived by the owner on Jul 31, 2020. It is now read-only.

moving bookmark to a different folder on laptop is not synced to mobile devices #111

Closed
alexwykoff opened this issue Jun 19, 2017 · 5 comments · Fixed by brave/browser-laptop#9614
Closed

moving bookmark to a different folder on laptop is not synced to mobile devices #111

alexwykoff opened this issue Jun 19, 2017 · 5 comments · Fixed by brave/browser-laptop#9614
Assignees
@diracdeltas
Labels
browser-laptop
Milestone
pyramid 2

Comments

@alexwykoff
Copy link

  • Did you search for similar issues before submitting this one?
    Yes

  • Describe the issue you encountered:
    When bookmark is moved from one folder to another on the laptop, the change is not synced to Android or iOS.

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    MacOS

  • Brave Version (revision SHA):
    Brave 0.17.3
    rev 02a7878
    Muon 4.0.3

  • Steps to reproduce:

    1. Create folders and bookmarks on laptop.
    2. Create folders and bookmarks on another device (I used Android and iOS devices)
    3. Create sync group on laptop and join on other devices.
    4. On laptop, navigate to a site that is not already bookmarked.
    5. Select the star in the URL bar.
    6. On the popup, select Done. You will see the bookmark added to the Bookmarks Toolbar. Wait and verify this is synced to Android and iOS devices.
    7. Drag and drop the just added bookmark from the Bookmarks Toolbar to a folder on Bookmarks Toolbar. Wait and verify this is synced to Android and iOS devices.
    8. Drag and drop the same bookmark to a different folder on the Bookmarks Toolbar. This action is not synced to either Android or iOS.
    9. From the Bookmarks Toolbar, right click on the Bookmark and select Edit. Change the folder and select Done. This action was also not synced to Android or iOS.

  • Actual result:
    Bookmark on synced devices still shows in original folder, not in updated folder.

  • Expected result:
    Bookmark should be displayed in updated folder on the synced devices.

  • Will the steps above reproduce in a fresh profile? If not what other info can be added?
    Yes

  • Is this an issue in the currently released version?
    mobile sync not available in released version

  • Can this issue be consistently reproduced?
    Yes

  • Extra QA steps:
    1.
    2.
    3.

  • Screenshot if needed:

Here's what was displayed in the dev tools console log while performing the steps to reproduce -
fetching BOOKMARKS,PREFERENCES records after 1497639813
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497639813
sync.js:2 got 0 decrypted records in PREFERENCES after 1497639813
sync.js:2 sending record: {"action":1,"bookmark":{"hideInToolbar":false,"isFolder":false,"site":{"creationTime":0,"customTitle":"","favicon":"https://www.fsu.edu/favicon.ico","lastAccessedTime":1497639621943,"location":"https://www.fsu.edu/","title":"Florida State University"}},"deviceId":{"0":0},"objectId":{"0":13,"1":125,"2":221,"3":119,"4":245,"5":137,"6":243,"7":112,"8":126,"9":232,"10":189,"11":108,"12":108,"13":21,"14":248,"15":193}}
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497639873
sync.js:2 got 1 decrypted records in BOOKMARKS after 1497639873
sync.js:1 Ignoring UPDATE of object 13,125,221,119,245,137,243,112,126,232,189,108,108,21,248,193.
sync.js:2 resolved 1 BOOKMARKS -> 0
sync.js:2 got 0 decrypted records in PREFERENCES after 1497639873
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497639933
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497639933
sync.js:2 got 0 decrypted records in PREFERENCES after 1497639933
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497639993
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497639993
sync.js:2 got 0 decrypted records in PREFERENCES after 1497639993
sync.js:2 sending record: {"action":0,"bookmark":{"hideInToolbar":false,"isFolder":false,"parentFolderObjectId":{"0":108,"1":158,"2":128,"3":120,"4":198,"5":251,"6":55,"7":182,"8":5,"9":197,"10":73,"11":195,"12":176,"13":107,"14":102,"15":240},"site":{"creationTime":0,"customTitle":"","favicon":"https://www.fsu.edu/favicon.ico","lastAccessedTime":1497639621943,"location":"https://www.fsu.edu/","title":"Florida State University"}},"deviceId":{"0":0},"objectId":{"0":13,"1":125,"2":221,"3":119,"4":245,"5":137,"6":243,"7":112,"8":126,"9":232,"10":189,"11":108,"12":108,"13":21,"14":248,"15":193}}
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640053
sync.js:2 got 1 decrypted records in BOOKMARKS after 1497640053
sync.js:2 resolved 1 BOOKMARKS -> 1
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640053
sync.js:2 sending record: {"action":1,"bookmark":{"hideInToolbar":false,"isFolder":false,"parentFolderObjectId":{"0":108,"1":158,"2":128,"3":120,"4":198,"5":251,"6":55,"7":182,"8":5,"9":197,"10":73,"11":195,"12":176,"13":107,"14":102,"15":240},"site":{"creationTime":0,"customTitle":"","favicon":"https://www.fsu.edu/favicon.ico","lastAccessedTime":1497639621943,"location":"https://www.fsu.edu/","title":"Florida State University"}},"deviceId":{"0":0},"objectId":{"0":13,"1":125,"2":221,"3":119,"4":245,"5":137,"6":243,"7":112,"8":126,"9":232,"10":189,"11":108,"12":108,"13":21,"14":248,"15":193}}
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640113
sync.js:2 got 1 decrypted records in BOOKMARKS after 1497640113
sync.js:1 Ignoring UPDATE of object 13,125,221,119,245,137,243,112,126,232,189,108,108,21,248,193.
sync.js:2 resolved 1 BOOKMARKS -> 0
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640113
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640173
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640173
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497640173
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640233
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640233
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497640233
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640293
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497640293
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640293
sync.js:2 sending record: {"action":0,"bookmark":{"hideInToolbar":false,"isFolder":false,"parentFolderObjectId":{"0":165,"1":201,"2":4,"3":209,"4":62,"5":33,"6":78,"7":142,"8":174,"9":240,"10":178,"11":139,"12":150,"13":136,"14":41,"15":39},"site":{"creationTime":0,"customTitle":"","favicon":"https://www.fsu.edu/favicon.ico","lastAccessedTime":1497639621943,"location":"https://www.fsu.edu/","title":"Florida State University"}},"deviceId":{"0":0},"objectId":{"0":13,"1":125,"2":221,"3":119,"4":245,"5":137,"6":243,"7":112,"8":126,"9":232,"10":189,"11":108,"12":108,"13":21,"14":248,"15":193}}
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640353
sync.js:2 got 1 decrypted records in BOOKMARKS after 1497640353
sync.js:1 Ignoring CREATE of object 13,125,221,119,245,137,243,112,126,232,189,108,108,21,248,193.
sync.js:2 resolved 1 BOOKMARKS -> 0
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640353
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640413
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497640413
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640413
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640473
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497640473
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640473
sync.js:2 fetching BOOKMARKS,PREFERENCES records after 1497640533
sync.js:2 got 0 decrypted records in BOOKMARKS after 1497640533
sync.js:2 got 0 decrypted records in PREFERENCES after 1497640533

  • Any related issues:
@alexwykoff alexwykoff mentioned this issue Jun 19, 2017
Closed
@alexwykoff alexwykoff added this to the pyramid 2 milestone Jun 19, 2017
@diracdeltas diracdeltas self-assigned this Jun 19, 2017
@LaurenWags LaurenWags mentioned this issue Jun 19, 2017
Closed
34 tasks
@diracdeltas
Copy link
Member

i can repro this when both devices are laptops. it is happening because for some reason, browser-laptop deletes the site and then re-adds it when the site changes folder. this resolves to a CREATE, which is a no-op when the site already exists on the receiver.

diracdeltas added a commit to brave/browser-laptop that referenced this issue Jun 20, 2017
@diracdeltas
send UPDATE instead of CREATE for sites
0721be0 
see brave/sync#111 (comment)

fix brave/sync#111

test plan:
1. create some folders and bookmarks in pyramid 0
2. create some folders and bookmarks in pyramid 1
3. create sync group in pyramid 0 and join it on pyramid 1
4. on pyramid 0, bookmark a new site. wait for it to sync over.
5. drag-and-drop the new bookmark into a folder. verify that this is synced.
6. right-click to edit an existing bookmark and change the parent folder. verify this is synced.
@diracdeltas diracdeltas mentioned this issue Jun 20, 2017
Merged
8 tasks
@diracdeltas
Copy link
Member

diracdeltas commented Jun 20, 2017 •
edited
Loading

i think the deletion happens because changing the parent folder changes the siteKey. this creates a separate issue where sometimes when bookmark changes folders and browser-laptop creates a 'remove' event, the bookmark is either moved or completely DELETED depending on which change is sent first.

diracdeltas added a commit to brave/browser-laptop that referenced this issue Jun 20, 2017
@diracdeltas
send UPDATE instead of CREATE for sites
6a418ea 
see brave/sync#111 (comment)

fix brave/sync#111

test plan:
1. create some folders and bookmarks in pyramid 0
2. create some folders and bookmarks in pyramid 1
3. create sync group in pyramid 0 and join it on pyramid 1
4. on pyramid 0, bookmark a new site. wait for it to sync over.
5. drag-and-drop the new bookmark into a folder. verify that this is synced.
6. right-click to edit an existing bookmark and change the parent folder. verify this is synced.
@ayumi ayumi mentioned this issue Jun 20, 2017
Open
@srirambv srirambv mentioned this issue Jun 21, 2017
Closed
@LaurenWags
Copy link
Member

I retested today with 0.17.7 on MacOS, 1.0.24(beta) on Android and 1.4 (17.06.21.14) on iOS. Steps 1-7 were as expected, but step 8 failed. recording below shows what I did for step 8.

111_start_at_step_8

@LaurenWags LaurenWags reopened this Jun 23, 2017
@alexwykoff alexwykoff mentioned this issue Jun 23, 2017
Closed
diracdeltas added a commit to brave/browser-laptop that referenced this issue Jun 24, 2017
@diracdeltas
Fix handling of skipSync
9860e13 
It should be removed from an object after the object is updated for the first time after it is fetched; otherwise future 'add' operations to the object will not be synced. It turns out that moving a bookmark from one folder to another is an 'add' operation which wasn't being synced before.

Fix brave/sync#111

Auditors: @ayumi

Test Plan:
Test plan in brave/sync#111
@bsclifton
Copy link
Member

I believe this was fixed with brave/browser-laptop@9860e13

@diracdeltas does this need to be merged to 0.18.x or master? (I don't think it does, but wanted to ask 😄 )

@diracdeltas
Copy link
Member

it may be an issue on master; i haven't checked. the sync code there is different enough that it's probably easier to fix separately than to merge this commit.

diracdeltas added a commit to brave/browser-laptop that referenced this issue Jun 26, 2017
@diracdeltas
Fix handling of skipSync
783ec1a 
It should be removed from an object after the object is updated for the first time after it is fetched; otherwise future 'add' operations to the object will not be synced. It turns out that moving a bookmark from one folder to another is an 'add' operation which wasn't being synced before.

Fix brave/sync#111

Test Plan:
Test plan in brave/sync#111
diracdeltas added a commit to brave/browser-laptop that referenced this issue Jun 26, 2017
@diracdeltas
Revert "handle empty objects in sync DELETEs"
720ecc4 
This reverts commit 91ef559.

it also re-fixes #9308 after the revert and fixes handling of skipSync (#111)

Test Plan:
Test plan in brave/sync#111
@diracdeltas diracdeltas mentioned this issue Jun 26, 2017
Merged
8 tasks
@LaurenWags LaurenWags mentioned this issue Jul 11, 2017
Closed
34 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@diracdeltas diracdeltas

Labels
browser-laptop
Projects
None yet
Milestone
pyramid 2
Development

Successfully merging a pull request may close this issue.

send UPDATE instead of CREATE for sites brave/browser-laptop
4 participants
@diracdeltas @alexwykoff @bsclifton @LaurenWags