Add sync #6882

diracdeltas · 2017-01-27T00:34:35Z

replaces #6751

this adds basic support for browser sync, using https://github.com/brave/sync

Test Plan:

Setup

make sure automated tests related to sync are passing
make sure npm install has been run. it also downloads the sync library.
add the following to package.json to create a separate browser 'device' to sync to:

 "start2": "node ./tools/start.js --user-data-dir=brave-development-2 --debug=5859 --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck"

Test

npm start
go to about:preferences#sync
enable sync
click 'add device...'
npm run start2
in pyramid 2, go to about:preferences#sync > i have an existing code > enter the code from device 1
go to any website in either of the browsers. bookmark it, and make changes to the siteSettings, e.g. disable JS. add a bookmark folder and a bookmark into it.
in the other pyramid, go to another website, bookmark, etc
after a minute, the bookmarks, history, and site settings should show up in the other browser. both browsers should contain data from each other.

todos before public release (not necessarily before merge)

https://github.com/brave/sync/milestone/2

This reverts commit 23203e1.

@ayumi

auditors: @ayumi test plan: 1. start brave 2. bookmark any page 3. observe that the terminal console says that the bookmark is created with some objectId 4. un-bookmark the page 5. observe that the terminal console says that the bookmark is deleted with the same objectId

@ayumi

in case they changed while sync was not yet initialized. Also add a TODO for where Ayumi's sync resolution IPC will hook in. Auditors: @ayumi

so it doesn't error

'electron' could not be required in the site util test, so unittests had been failing.

@ayumi

This prevents the window from crashing on first init, which has been causing Travis to fail. Auditors: @ayumi

@ayumi

5da9ba5 removed RECEIVE_SYNC_RECORDS Auditors: @ayumi

@ayumi

also refactors reusable sync methods from app/sync.js to js/state/syncUtil.js Auditors: @ayumi Test Plan: 1. start Brave 2. go to bing.com 3. disable shields 4. you should see the console log a setting with 'shieldsUp: false', indicating the sync-client sent the record. 5. enable shields 6. same as 4 but with 'shieldsUp: true'

Adds a preference for sync and categories. Defaults to off. Test Plan: 1. go to about:preferences#sync and turn sync on 3. kill brave and start it again 4. verify in console that sync is running 5. go to brave.com, verify that no history is synced 6. go to about:preferences#sync, enable syncing history 7. go to brave.com, verify that history is synced this time 8. go to about:preferences#sync, disable sync 9. go to brave.com, bookmark it. verify that nothing is synced at all.

@diracdeltas

Auditors: @diracdeltas Test Plan: 1. Start Brave, go to Preferences #Sync and check out the sweet pyramid

@ayumi

7abab4b caused node to throw 'module not found' errors when siteUtil (which depended on syncUtil) was imported into renderer components, i think. This avoids the problem by not having siteUtil depend on syncUtil. also auto-added docs from the post-commit hook Auditors: @ayumi Test Plan: 1. npm start 2. you should not see any 'module not found' errors in the terminal

@diracdeltas

Auditors: @diracdeltas

@ayumi

This adds most of the UX for Brave sync setup in the case where this device is the first device in the sync profile. Auditors: @ayumi Test Plan: 1. npm run clean-session-store 2. npm start 3. go to about:preferences#sync 4. click the orange button to set up sync for the first time 5. name the device, then click the create button 6. shut down brave, run npm start again 7. you should see the device name from (5) in the terminal console 8. go to about:preferences#sync, click 'add new device' button 9. you should see a QR code

@ayumi

Auditors: @ayumi

@diracdeltas

Auditors: @diracdeltas

@diracdeltas

Auditors: @diracdeltas Test Plan: Prep: 0. Update sync lib to `brave/sync #fix/resolve-delete-nonexistant-props`. 1. Prepare 2 instances (pyramids) of Brave with Sync enabled. - Enable Sync and close Brave. - Copy `{userData}/brave-development` to `{userData}/brave-development-2`. - Edit `brave-development-2/session-store-1` `deviceId` to `1`. - To `browser-laptop` `package.json` add `"start2": "node ./tools/start.js --user-data-dir=brave-development-2 --debug=5859 --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",` 2. In `appConfig.js` `sync.fetchInterval` reduce to 5 seconds. Play: 3. Open both pyramid 1 and pyramid 2. 4. In pyramid 1 visit a webpage and open up the bravery panel. 5. In pyramid 2 visit the same page and open up the bravery panel. 6. In pyramid 2 toggle each available siteSetting. Observe it appears in pyramid 1 after 1–5s. 7. Try toggling multiple settings at once, and toggling different settings simulatenously on both pyramids. 8. In both go to Preferences #Shields. Clear siteSettings with the Clear links and the red X's. Observe they sync over.

by entering the 16 code words

@diracdeltas

Auditors: @diracdeltas Test Plan: 1. Have Sync disabled. 2. Browse a site and toggle a Bravery Panel setting. 3. Enable Sync and restart your pyramid. Note that siteSettings are sent. 4. Restart your pyramid again. Note this time siteSettings are *not* sent.

@diracdeltas

Fix brave/sync#44 Auditors: @diracdeltas Test Plan: 1. Start Pyramid 1 which has a bunch of syncable data. Sync should enabled. 2. Setup Pyramid 2 (add `"start2": "node ./tools/start.js --user-data-dir=brave-development-2 --debug=5859 --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",` to package.json) and specify the Pyramid 1 Sync credentials. 3. When Pyramid 2 restarts and begins syncing data, Brave should be somewhat usable (not completely frozen for 60s).

diracdeltas · 2017-01-27T00:49:28Z

The test plan previously worked against 0.13.1 but seems buggy with master (bookmark favicon is synced but not the actual bookmark location). Site settings sync seems to be working.

@diracdeltas

Also show device name Auditors: @diracdeltas

Test syncing can be turned off; private history does not sync

Syncing styling

@ayumi

Auditors: @ayumi Test Plan: 1. go to about:preferences#sync 2. click 'i have an existing sync code' 3. the setup button should be disabled 4. enter some text in the textbox 5. the setup button should be enabled

@ayumi

Auditors: @ayumi Test Plan: 1. enable sync for the first time 2. don't change the default device name 3. once sync is enabled, it should show 'Mac Laptop' as the device name or whatever your OS is

Test Plan: 1. Open Preferences > Sync. 2. With Sync enabled, the area with the toggle switch and device name should look ok.

alexwykoff · 2017-02-14T06:47:37Z

test/unit/state/siteUtilTest.js

        assert.deepEqual(processedSites.toJS(), expectedSites)
      })
+      it('sets an objectId when syncCallback is provided', function () {


This appears to be describing and update/edit case if I am not mistaken.

What I did not notice is a deletion test case.

alexwykoff · 2017-02-14T06:53:21Z

test/components/syncTest.js

+      .waitForUrl(this.page1Url)
+    yield bookmarkUrl(this.page2Url, this.page2Title)
+    yield Brave.app.client
+      .pause(1000) // XXX: Helps to correctly order Create and Update


Rather than using explicit pauses, is there no promise (or set of nested promises) we can create?

@ayumi put some effort into this but it seemed like not easily

in general i agree on removing explicit pauses; if i understand correctly, it is tricky here because this is waiting on the server to receive the records and process them, which is not something that can be detected client-side (ex: using a waitUntil for some appState change). since the server can't push notifications to the client, we would have to have the client poll the server periodically to detect when a sync operation has finished.

alexwykoff · 2017-02-14T07:05:52Z

test/components/syncTest.js

+  })
+})
+
+describe('Syncing history', function () {


What happens if the separate machines have overlapping timestamps for history? Do we have tiebreaker logic?

not sure what you mean by a 'tie'. if the same history entry (same location) has two sync records with the same timestamp, the one which is received later by the sync server wins.

alexwykoff · 2017-02-14T07:08:46Z

test/components/syncTest.js

+  })
+})
+
+describe('Syncing site settings', function () {


What happens for conflicting site preferences? How do we tiebreak?

i believe the record which is received later always wins. https://github.com/brave/sync/blob/staging/client/recordUtil.js#L103

Depends on brave/sync #fix/timestamp-ms to return S3 records in upload order. Test Plan: 1. `npm run test -- --grep='"^Syncing bookmarks from an"'`

Sync bookmarks in order during initial sync

Related: brave/sync#56

@diracdeltas

- Add download sync client to package.json postinstall - Fix sync endpoints to be prod for packaged apps Auditors: @diracdeltas @bsclifton Test Plan: - Remove `node_modules`. - `npm install`. - `CHANNEL=dev npm run build-package` / `CHANNEL=dev npm run build-installer`. - Open the built app. - Preferences > Sync should be present. - Enable Sync. - Sync should connect to `https://sync.brave.com` and `https://brave-sync.s3.dualstack.us-west-2.amazonaws.com`. (You could check this with Little Snitch for MacOS)

Prepare Sync for packaging

ayumi · 2017-02-15T03:12:33Z

Discussed with @bsclifton and @bbondy in slack and decided this is good to merge for MVP sync!

@alexwykoff we will follow up with polish right away.

luixxiul · 2017-02-15T03:31:00Z

@diracdeltas @ayumi Please update the test plan in the first post, if it is outdated. Also please do not hesitate to add anything which should be tested during QA tests, including edge cases. Thanks in advance!

ayumi · 2017-02-15T18:53:51Z

@luixxiul thanks, I updated the Test Plan above!

diracdeltas and others added 23 commits January 27, 2017 00:18

Revert "Convert background page to event page"

3d0fb35

This reverts commit 23203e1.

start integration with brave/sync/feature/syncing

abb9eea

Send site settings and bookmarks when sync starts up

0661451

in case they changed while sync was not yet initialized. Also add a TODO for where Ayumi's sync resolution IPC will hook in. Auditors: @ayumi

auto update to latest sync/staging

5778210

make travis download the sync client bundle

62e1d31

so it doesn't error

fix failing siteUtil unit test

3f426cb

'electron' could not be required in the site util test, so unittests had been failing.

Save sync init data as Immutable.List instead of Uint8Array

3587172

This prevents the window from crashing on first init, which has been causing Travis to fail. Auditors: @ayumi

change RECEIVE_SYNC_RECORDS to GET_EXISTING_OBJECTS

2405b52

5da9ba5 removed RECEIVE_SYNC_RECORDS Auditors: @ayumi

Sync: Update appState with Sync bookmark records

4f16d95

Add Sync logo

3284b51

Auditors: @diracdeltas Test Plan: 1. Start Brave, go to Preferences #Sync and check out the sweet pyramid

Fix SYNC_CLEAR_HISTORY

08c913a

Fix applying Create bookmark folders

6d31ee9

Auditors: @diracdeltas

Add DELETE_SYNC_SITE_SETTINGS, decrease sync interval

502ea23

Auditors: @ayumi

Fix syncing pinned sites

610ecbd

Auditors: @diracdeltas

allow browser-laptop to join an existing sync profile

934acb5

by entering the 16 code words

diracdeltas mentioned this pull request Jan 27, 2017

Add sync #6751

Closed

diracdeltas added this to the 0.13.2 milestone Jan 27, 2017

diracdeltas added feature/sync PR/needs-QA-attention ☕ labels Jan 27, 2017

diracdeltas modified the milestones: 0.13.1, 0.13.2 Jan 27, 2017

diracdeltas and others added 2 commits February 10, 2017 01:48

friendlier device name input UI

375ba5d

Syncing styling

7dad719

Also show device name Auditors: @diracdeltas

luixxiul mentioned this pull request Feb 10, 2017

Use <SettingTextbox> for the textbox on the dialog of enabling Sync #7164

Merged

ayumi and others added 8 commits February 10, 2017 09:55

Merge pull request #7145 from brave/feature/syncing-tests-3

2d43d47

Test syncing can be turned off; private history does not sync

Merge pull request #7152 from brave/feature/syncing-styling

04af333

Syncing styling

disable sync restore button until code words are entered

7f1e79f

Auditors: @ayumi Test Plan: 1. go to about:preferences#sync 2. click 'i have an existing sync code' 3. the setup button should be disabled 4. enter some text in the textbox 5. the setup button should be enabled

Merge remote-tracking branch 'origin/master' into feature/syncing

4cd74b0

fix issues related to merge and sync default naming

a66f921

Auditors: @ayumi Test Plan: 1. enable sync for the first time 2. don't change the default device name 3. once sync is enabled, it should show 'Mac Laptop' as the device name or whatever your OS is

Adjust width of sync toggle pref / device name

a80b05b

Test Plan: 1. Open Preferences > Sync. 2. With Sync enabled, the area with the toggle switch and device name should look ok.

Merge remote-tracking branch 'origin/master' into feature/syncing

7c59a85

guard against same objectId set on bookmark and history site

8c39e18

alexwykoff reviewed Feb 14, 2017

View reviewed changes

ayumi and others added 4 commits February 13, 2017 23:24

Sync bookmarks in order during initial sync

1a69b53

Depends on brave/sync #fix/timestamp-ms to return S3 records in upload order. Test Plan: 1. `npm run test -- --grep='"^Syncing bookmarks from an"'`

Merge pull request #7236 from brave/feature/syncing-bookmarks-existing

0239612

Sync bookmarks in order during initial sync

Merge remote-tracking branch 'origin/master' into feature/syncing

4984218

Remove Sync test pauses

2618188

ayumi removed the PR/needs-rebase ۞ label Feb 14, 2017

ayumi and others added 3 commits February 14, 2017 14:52

Remove deprecated sync record field Bookmark.index

d132bdf

Related: brave/sync#56

Merge pull request #7247 from brave/feature/syncing-auto-dl-lib

7809a0a

Prepare Sync for packaging

ayumi merged commit 0904162 into master Feb 15, 2017

ayumi deleted the feature/syncing branch February 15, 2017 03:12

luixxiul removed the PR/needs-QA-attention ☕ label Feb 15, 2017

scottstensland mentioned this pull request Feb 15, 2017

Implement sync #1854

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sync #6882

Add sync #6882

diracdeltas commented Jan 27, 2017 •

edited by ayumi

Loading

diracdeltas commented Jan 27, 2017

alexwykoff Feb 14, 2017

alexwykoff Feb 14, 2017

diracdeltas Feb 14, 2017

diracdeltas Feb 14, 2017

alexwykoff Feb 14, 2017

diracdeltas Feb 14, 2017

alexwykoff Feb 14, 2017

diracdeltas Feb 14, 2017

ayumi commented Feb 15, 2017

luixxiul commented Feb 15, 2017 •

edited

Loading

ayumi commented Feb 15, 2017

Add sync #6882

Add sync #6882

Conversation

diracdeltas commented Jan 27, 2017 • edited by ayumi Loading

Test Plan:

Setup

Test

todos before public release (not necessarily before merge)

diracdeltas commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayumi commented Feb 15, 2017

luixxiul commented Feb 15, 2017 • edited Loading

ayumi commented Feb 15, 2017

diracdeltas commented Jan 27, 2017 •

edited by ayumi

Loading

luixxiul commented Feb 15, 2017 •

edited

Loading