Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

syncing site settings exception #8241

Closed
diracdeltas opened this issue Apr 11, 2017 · 3 comments · Fixed by #8242
Closed

syncing site settings exception #8241

diracdeltas opened this issue Apr 11, 2017 · 3 comments · Fixed by #8242
Assignees
@diracdeltas
Labels
feature/sync needs-info Another team member needs information from the PR/issue opener. QA/checked-Linux QA/checked-macOS QA/checked-Win32 QA/checked-Win64 QA/test-plan-specified release-notes/exclude
Milestone
0.15.0

Comments

@diracdeltas
Copy link
Member

diracdeltas commented Apr 11, 2017 •
edited by bsclifton
Loading

Test plan

  1. 'Syncing' tests should pass
  2. Set isProduction to true in js/constants/appConfig
  3. Sync to the profile starting with "duality" (sent via slack DM)
  4. You should not see any console errors
  5. Go http://expectnothing.com/ and verify that shields are down

Original issue description

syncing to serg's profile on the production sync server throws this error:

An uncaught exception occurred in the main process Uncaught Exception:
TypeError: Cannot read property '1' of undefined
    at applySiteSettingRecord (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:156:48)
    at applySyncRecord (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:201:7)
    at Immediate.setImmediate (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:180:5)
    at runCallback (timers.js:651:20)
    at tryOnImmediate (timers.js:624:5)
    at processImmediate [as _immediateCallback] (timers.js:596:5)
@diracdeltas diracdeltas added this to the 0.14.2 milestone Apr 11, 2017
@diracdeltas diracdeltas self-assigned this Apr 11, 2017
diracdeltas added a commit that referenced this issue Apr 12, 2017
@diracdeltas
fix applySiteSettingRecord error, remove unneeded setObjectId calls
7008144 
fix #8241

applySiteSettingRecord should not need to check if there is a diff between
the existing record and the new record; this is the job of reducers.

Auditors: @ayumi

Test Plan:
1. 'Syncing' tests should pass
2. Set isProduction to true in js/constants/appConfig
3. Sync to the profile starting with "duality" (sent via slack DM)
4. You should not see any console errors
5. Go http://expectnothing.com/ and verify that shields are down
@diracdeltas diracdeltas mentioned this issue Apr 12, 2017
Merged
4 tasks
bsclifton pushed a commit that referenced this issue Apr 12, 2017
@diracdeltas @bsclifton
fix applySiteSettingRecord error, remove unneeded setObjectId calls
707fcf2 
fix #8241

applySiteSettingRecord should not need to check if there is a diff between
the existing record and the new record; this is the job of reducers.

Auditors: @ayumi

Test Plan:
1. 'Syncing' tests should pass
2. Set isProduction to true in js/constants/appConfig
3. Sync to the profile starting with "duality" (sent via slack DM)
4. You should not see any console errors
5. Go http://expectnothing.com/ and verify that shields are down
This was referenced Apr 13, 2017
Closed
Closed
Closed
Closed
This was referenced Apr 21, 2017
Closed
Closed
Closed
@cezaraugusto
Copy link
Contributor

@diracdeltas is there a member-only area where I could get this profile?

@alexwykoff
Copy link
Contributor

While trying this test I found that about:preferences#security per site settings (like fullscreen for youtube) would not sync, but about:preferences#shields would sync.

So while there is no exception, it's unclear if the issue is fully resolved.

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Apr 24, 2017
@luixxiul luixxiul mentioned this issue Apr 25, 2017
Closed
102 tasks
This was referenced Apr 25, 2017
Closed
Closed
Closed
@diracdeltas
Copy link
Member Author

it's just for shields

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@diracdeltas diracdeltas

Labels
feature/sync needs-info Another team member needs information from the PR/issue opener. QA/checked-Linux QA/checked-macOS QA/checked-Win32 QA/checked-Win64 QA/test-plan-specified release-notes/exclude
Projects
None yet
Milestone
0.15.0
Development

Successfully merging a pull request may close this issue.

fix applySiteSettingRecord error, remove unneeded setObjectId calls brave/browser-laptop
5 participants
@diracdeltas @alexwykoff @luixxiul @cezaraugusto @bsclifton