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

Fix record merging #84

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Fix record merging #84

merged 1 commit into from
Apr 27, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Apr 27, 2017

The previous way somtimes merged a record into record with a different objectId somehow. Using Object.values guarantees that the final number of merged records will be the same as the number of unique objectIds.

Fix brave/browser-laptop#8454, fix tests in brave/browser-laptop#8480

Test plan:

  1. Sync to the 'rummy tarp...' group sent by slack DM.
  2. You should see a bookmark folder 'f1' with two bookmarks in it; previously the bookmarks would be at the top level.
  3. checkout refactor sync to use appStore change listener browser-laptop#8480 and run "npm run test -- --grep='^Syncing bookmarks'". the tests should pass.

The previous way somtimes merged a record into record with a different objectId somehow. Using Object.values guarantees that the final number of merged records will be the same as the number of unique objectIds.

Test plan:
1. Sync to the 'rummy tarp...' group sent by slack DM.
2. You should see a bookmark folder 'f1' with two bookmarks in it; previously the bookmarks would be at the top level.
3. checkout brave/browser-laptop#8480 and run "npm run test -- --grep='^Syncing bookmarks'". the tests should pass.
@diracdeltas
Copy link
Member Author

a more clear test case:

  1. open pyramid 0 on brave/master, start a sync group
  2. bookmark a site
  3. create bookmark folder
  4. move the bookmark into the folder
  5. create another bookmark
  6. sync pyramid 1 to pyramid 0
  7. pyramid 1 should have the same bookmark hiearchy

Copy link
Contributor

@ayumi ayumi left a comment

Choose a reason for hiding this comment

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

tests, browser-laptop tests, and test plan success

@ayumi ayumi merged commit c775b0c into staging Apr 27, 2017
@ayumi ayumi deleted the fix/merge-records branch April 27, 2017 07:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync is not tracking & merging changes between pyramids which creates a loss of hierarchy
2 participants