Skip to content

/ sync

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

Merge by id action (staging) #313

Merged
merged 6 commits into from May 17, 2019
Merged

Merge by id action (staging) #313

merged 6 commits into from May 17, 2019

Conversation

@AlexeyBarabash
Copy link
Contributor

AlexeyBarabash commented May 13, 2019

We met the situation when [CREATE, UPDATE] were merged to [CREATE], which was then ignored because local object had been existed.
This PR makes CREATE and DELETE records stays alone to prevent such ignoring. So only UPDATE records of the same object id are merged.

STR for brave-core brave-syncer branch:

  1. DeviceA: create folderA in bookmarks
  2. DeviceA: create bookmarks of brave://bookmarks, brave://sync, brave://inspect, all in Bookmarks Bar folder
  3. DeviceA: enable sync and copy code words
  4. DeviceB: join sync chain with words from pt.3
  5. DeviceB: As soon as bookmarks from DeviceA appear , move 3 bookmarks into folderA
  6. Actual on DeviceA: bookmark will not be moved
    Expected on DeviceA: bookmarks should be moved.
@AlexeyBarabash AlexeyBarabash self-assigned this May 13, 2019
@AlexeyBarabash AlexeyBarabash changed the title Merge by id action staging Merge by id action (staging) May 13, 2019
@darkdh
Copy link
Member

darkdh commented May 13, 2019

If you want to merge this into staging, you should remove brave-syncer specific commit "Add MetaInfo to Bookmark", otherwise you can merge into brave-syncer branch
@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented May 13, 2019

@darkdh yes, reopened PR for brave-syncer-staging branch
@AlexeyBarabash AlexeyBarabash changed the base branch from staging to brave-syncer-staging May 13, 2019
@darkdh
darkdh requested changes May 13, 2019
View changes
Copy link
Member

darkdh left a comment

and you should update the test
@AlexeyBarabash AlexeyBarabash force-pushed the brave-syncer-staging branch from a72d29c to 3c72f2b May 14, 2019
@AlexeyBarabash AlexeyBarabash force-pushed the merge_by_id_action_staging branch from 88f85d3 to e07de75 May 14, 2019
@AlexeyBarabash
Merge keeps alone CREATE and DELETE
20a74d0
@AlexeyBarabash AlexeyBarabash force-pushed the merge_by_id_action_staging branch from 39dad93 to 23bec26 May 14, 2019
@AlexeyBarabash
fixed 'resolves same data cross-platform on laptop and android' by ad…
89d5f8e 
…d missing 'parentFolderObjectId':[]
@AlexeyBarabash
Introduced a new testcase
Loading status checks…
4af5ad4 
Create + Update + Update of an existing object should resolve to a separate Update
@AlexeyBarabash AlexeyBarabash force-pushed the merge_by_id_action_staging branch from 23bec26 to 4af5ad4 May 15, 2019
@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented May 15, 2019

CI failed because npm ERR! audit Your configured registry (https://registry.npmjs.org/) does not support audit requests, or the audit endpoint is temporarily unavailable.
@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented May 15, 2019

reopened PR to trigger new CI run
@AlexeyBarabash AlexeyBarabash requested a review from darkdh May 15, 2019
@darkdh
darkdh reviewed May 16, 2019
View changes
Copy link
Member

darkdh left a comment

Why don't we have to update these two test cases?
4af5ad4#diff-dbbb41acf4ed22ffd4a6afc45647bb64R408
4af5ad4#diff-dbbb41acf4ed22ffd4a6afc45647bb64R429
test/client/recordUtil.js Show resolved Hide resolved
@AlexeyBarabash
Fixed 'Create + Update of a new object should resolve to a single Cre…
86d3026 
…ate'
@AlexeyBarabash
Fixed merge to have separate CREATE only when existingObject is not n…
Loading status checks…
81d04c1 
…ull. Can be squashed
@AlexeyBarabash
Added test for UPDATE,UPDATE with existing object; renamed another te…
Loading status checks…
cd79e42 
…st to 'sequential Updates should become no op if the merged result equals to existingObject'
@AlexeyBarabash AlexeyBarabash requested a review from darkdh May 16, 2019
@darkdh
darkdh approved these changes May 16, 2019
View changes
@darkdh darkdh merged commit 7a4f9c8 into brave-syncer-staging May 17, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@darkdh darkdh deleted the merge_by_id_action_staging branch May 17, 2019
darkdh added a commit that referenced this pull request May 17, 2019
@darkdh
Merge pull request #313 from brave/merge_by_id_action_staging
Verified
This commit was signed with a verified signature.
darkdh Anthony Tseng
GPG key ID: E14094A20D3AAEEC Learn about signing commits
Loading status checks…
fe74151 
Merge by id action (staging)
darkdh added a commit that referenced this pull request May 21, 2019
@darkdh
Merge pull request #313 from brave/merge_by_id_action_staging
Verified
This commit was signed with a verified signature.
darkdh Anthony Tseng
GPG key ID: E14094A20D3AAEEC Learn about signing commits
Loading status checks…
951e071 
Merge by id action (staging)
@AlexeyBarabash AlexeyBarabash mentioned this pull request May 30, 2019
Merged
This was referenced Oct 15, 2019
Closed
Closed
@AlexeyBarabash AlexeyBarabash restored the merge_by_id_action_staging branch Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh

@diracdeltas diracdeltas

@SergeyZhukovsky SergeyZhukovsky

Assignees

@AlexeyBarabash AlexeyBarabash

Labels
enhancement
Projects
Sync - All platforms
  
Awaiting triage
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@AlexeyBarabash @darkdh
You can’t perform that action at this time.