Skip to content
This repository has been archived by the owner. It is now read-only.

send UPDATE instead of CREATE for sites #9614

Merged
merged 1 commit into from Jun 21, 2017
Merged

send UPDATE instead of CREATE for sites #9614

merged 1 commit into from Jun 21, 2017

Conversation

@diracdeltas
Copy link
Member

diracdeltas commented Jun 20, 2017

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.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header
@diracdeltas diracdeltas self-assigned this Jun 20, 2017
@diracdeltas diracdeltas requested review from ayumi and SergeyZhukovsky Jun 20, 2017
@diracdeltas diracdeltas added this to the 0.17.x (Beta Channel) milestone Jun 20, 2017
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 force-pushed the fix/sync-folder branch from 0721be0 to 6a418ea Jun 20, 2017
@alexwykoff
Copy link
Member

alexwykoff commented Jun 20, 2017

left-to-right order on initial sync didn't appear to propagate
(top os x, bottom linux)
screen shot 2017-06-20 at 6 06 29 pm

otherwise the rest of the steps seem fine. certainly fixes the primary case of managing folder structure and updating appropriately

@ayumi
ayumi approved these changes Jun 20, 2017
Copy link
Contributor

ayumi left a comment

sweet, test plan works and tests pass (npm run test -- --grep='^Syncing and clearing' is intermittent but i think it's in master as well.)

🌵

@diracdeltas
Copy link
Member Author

diracdeltas commented Jun 20, 2017

@alexwykoff as mentioned in slack and during the triage, order is not preserved during sync yet

Copy link
Member

SergeyZhukovsky left a comment

++

@bbondy bbondy merged commit 8fd0f34 into master Jun 21, 2017
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
bbondy added a commit that referenced this pull request Jun 21, 2017
send UPDATE instead of CREATE for sites
diracdeltas added a commit that referenced this pull request Jun 21, 2017
send UPDATE instead of CREATE for sites
@alexwykoff
Copy link
Member

alexwykoff commented Jun 21, 2017

@diracdeltas correct, just wanted to list it as observed so we don't need to re-test to see if we got a 2-for-1 😸

@cezaraugusto cezaraugusto deleted the fix/sync-folder branch Jun 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.