Skip to content
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

Get and update sync code only after sync chain is created #6182

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jul 22, 2020

Otherwise, we end up caching wrong sync code when page is loaded and it
will only be updated after a page refresh

Resolves brave/brave-browser#10857

Submitter Checklist:

Test Plan:

  1. STR in [Desktop] Wrong code words are shown in View Sync Code after sync creation brave-browser#10857
  2. Reset sync chain
  3. Create a new sync chain and remember the sync code
  4. After sync chain created, click View Sync Code
  5. Code in step 3 & 4 should be the same

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@darkdh darkdh requested a review from petemill July 22, 2020 23:15
@darkdh darkdh self-assigned this Jul 22, 2020
@darkdh darkdh force-pushed the sync-words-mismatch branch from 954147e to 631263a Compare July 22, 2020 23:19
@darkdh
Copy link
Member Author

darkdh commented Jul 23, 2020

restart CI for window only

@darkdh darkdh added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 and removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 23, 2020
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Just 1 minor issue

@@ -82,11 +78,20 @@ Polymer({
return displayDate.toDateString()
},

onViewSyncCode_: function() {
getSyncCode_: async function() {
Copy link
Member

Choose a reason for hiding this comment

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

My preference would be to call this ensureSetSyncCode or at least not have get in it, since it's confusing when reading the calls to it as getXYZ implies returning a value.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@petemill
Copy link
Member

@darkdh was this issue caused because the 'configure' component is not removed and re-attached when finishing setting up the new sync chain and exiting the sync section?
If there's still a timing issue with this fix in the future, the proper fix is probably to have the syncCode bubble down from the main sync page component.

@darkdh
Copy link
Member Author

darkdh commented Jul 27, 2020

@darkdh was this issue caused because the 'configure' component is not removed and re-attached when finishing setting up the new sync chain and exiting the sync section?
If there's still a timing issue with this fix in the future, the proper fix is probably to have the syncCode bubble down from the main sync page component.

No, it is actually caused by user navigate to sync page for the first time without any sync chain setup. At that point, it tries to get sync code from pref but it shouldn't

Otherwise, we end up caching wrong sync code when page is loaded and it
will only be updated after a page refresh
@darkdh darkdh force-pushed the sync-words-mismatch branch from 631263a to 4a22990 Compare July 27, 2020 21:16
@darkdh darkdh requested review from bsclifton and petemill July 27, 2020 21:17
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Works perfectly! 👍 Thanks for adding the comment and fixing the reset sync chain case 😄

Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

LGTM

@bsclifton bsclifton merged commit 9632691 into master Jul 28, 2020
@bsclifton bsclifton deleted the sync-words-mismatch branch July 28, 2020 04:06
brave-builds pushed a commit that referenced this pull request Jul 28, 2020
@btlechowski
Copy link

LGTM

Verification passed on

Brave 1.13.50 Chromium: 84.0.4147.105 (Official Build) nightly (64-bit)
Revision a6b12dfad6663f13a7e16e9a42a6a4975374096b-refs/branch-heads/4147@{#943}
OS Ubuntu 18.04 LTS

Verified test plan from #6182 (comment).
Verified test case from brave/brave-browser#10857 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Desktop] Wrong code words are shown in View Sync Code after sync creation
5 participants