Skip to content
This repository has been archived by the owner. It is now read-only.

sync v2 #13197

Closed
wants to merge 16 commits into from

allow sync devices to be removed

fix #9254
fix #12356
  • Loading branch information
Cezar Augusto authored and cezaraugusto committed Mar 1, 2018
commit bf2990bfe2516a1a19ef0634b101511196eb7383
@@ -370,6 +370,7 @@ syncRemoveDevice.title=Remove this device
syncRemoveActiveDeviceWarning1=Local device data will remain intact on all devices. Other devices in this sync chain will remain sync'd.

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 9, 2018

Member

can we use synced instead of sync'd (which isn't a word)?

This comment has been minimized.

Copy link
@bradleyrichter

bradleyrichter Apr 10, 2018

Contributor

I hate the way the word looks but since it is more correct, let's use "synced".

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 10, 2018

Author Member

done in 63c57cd

syncRemoveActiveDeviceWarning2=To join a sync chain again, choose "Enter a sync chain code".
syncRemoveOtherDeviceWarning= Note: Removing this device from this sync chain does not clear previously sync'd data from the device.

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 9, 2018

Member

same as above

syncReset=Reset Sync
syncResetDataDisabled=This feature is only available when Sync is enabled.
syncRetryButton=Try again
syncScanMobile=Now, using Brave on your mobile device, scan this code
@@ -39,6 +39,8 @@ const syncComputerImage = require('../../../extensions/brave/img/sync/device_typ
const syncPlusImage = require('../../../extensions/brave/img/sync/add_device_titleicon.svg')
const syncCodeImage = require('../../../extensions/brave/img/sync/synccode_titleicon.svg')
const syncHandImage = require('../../../extensions/brave/img/sync/hand_image.png')
const syncRemoveImage = require('../../../extensions/brave/img/sync/remove_device_titleicon.svg')
const removeIcon = require('../../../extensions/brave/img/ledger/icon_remove.svg')
const syncPassphraseInputSize = 16

class SyncTab extends ImmutableComponent {
@@ -52,8 +54,12 @@ class SyncTab extends ImmutableComponent {
this.onCopy = this.copyPassphraseToClipboard.bind(this)
this.state = {
wordCount: 0,
deviceIdToRemove: '',
deviceNameToRemove: '',
isRemovingMainDevice: '',
currentDeviceOption: ''
}
this.onRemove = this.removeSyncDevice.bind(this)
}

get setupError () {
@@ -202,6 +208,17 @@ class SyncTab extends ImmutableComponent {
{
html: new Date(device.get('lastRecordTimestamp')).toLocaleDateString(),
value: device.get('lastRecordTimestamp')
},
{
html: <span
id={id}
data-main-device={device.get('mainDevice')}
data-device-name={device.get('name')}
data-l10n-id='syncRemoveDevice'
className={css(styles.actionIcons__icon, styles.actionIcons__icon_remove)}
onClick={this.onClickSyncRemoveButton.bind(this)}
/>,
value: ''
}
])
}
@@ -741,6 +758,42 @@ class SyncTab extends ImmutableComponent {
</section>
}

get removeOverlayContent () {
return (
<Grid gap={0} columns={1} padding='0 77px'>
<Column>
{
this.state.isRemovingMainDevice
? (
<div>
<p
className={css(styles.settingsListContainerMargin__bottom)} data-l10n-id='syncRemoveActiveDeviceWarning1'
/>
<p data-l10n-id='syncRemoveActiveDeviceWarning2' />
</div>
)
: <p data-l10n-id='syncRemoveOtherDeviceWarning' />
}
</Column>
</Grid>
)
}

get removeOverlayFooter () {
return <section>
<BrowserButton groupedItem secondaryColor
l10nId='cancel'
testId='cancelButton'
onClick={this.props.hideOverlay.bind(this, 'syncRemove')}
/>
<BrowserButton groupedItem primaryColor
l10nId='syncRemove'
testId='syncRemoveButton'
onClick={this.onRemove}
/>
</section>
}

enableRestore (e) {
if (e.target.value.length > 0) {
const wordCount = e.target.value
@@ -908,6 +961,18 @@ class SyncTab extends ImmutableComponent {
onHide={this.onHideAnyRemovalOverlay.bind(this)} />
: null
}
{
this.props.syncRemoveOverlayVisible
? <ModalOverlay
whiteOverlay
title={'syncRemoveDeviceModal'}
titleImage={syncRemoveImage}
titleArgs={{device: this.state.deviceNameToRemove}}
content={this.removeOverlayContent}
footer={this.removeOverlayFooter}
onHide={this.props.hideOverlay.bind(this, 'syncRemove')} />
: null
}
<section className={css(styles.settingsListContainerMargin__bottom)}>
{
this.setupError
@@ -444,6 +444,7 @@ class AboutPreferences extends React.Component {
syncChainCodeOverlayVisible: false,
syncQRPassphraseOverlayVisible: false,
syncResetOverlayVisible: false,
syncRemoveOverlayVisible: false,
syncRestoreEnabled: false,
preferenceTab: this.tabFromCurrentHash,
hintNumber: this.getNextHintNumber(),
@@ -668,6 +669,7 @@ class AboutPreferences extends React.Component {
syncChainCodeOverlayVisible={this.state.syncChainCodeOverlayVisible}
syncQRPassphraseOverlayVisible={this.state.syncQRPassphraseOverlayVisible}
syncResetOverlayVisible={this.state.syncResetOverlayVisible}
syncRemoveOverlayVisible={this.state.syncRemoveOverlayVisible}
/>
break
case preferenceTabs.SHIELDS:
@@ -1074,6 +1074,16 @@ const appActions = {
})
},

/**
* Dispatches a message to stop syncing the requested device.
*/
removeSyncDevice: function (deviceId) {
dispatch({
actionType: appConstants.APP_REMOVE_SYNC_DEVICE,
deviceId
})
},

/**
* Tells the store that user has finish setting up Sync
*/
@@ -104,6 +104,7 @@ const appConstants = {
APP_SAVE_SYNC_INIT_DATA: _,
APP_RESET_SYNC_DATA: _,
APP_SET_SYNC_SETUP_ERROR: _,
APP_REMOVE_SYNC_DEVICE: _,
APP_SETUP_SYNC_COMPLETED: _,
APP_ADD_NOSCRIPT_EXCEPTIONS: _,
APP_TAB_MESSAGE_BOX_SHOWN: _,
@@ -34,6 +34,7 @@ const {HrtimeLogger} = require('../../app/common/lib/logUtil')
const platformUtil = require('../../app/common/lib/platformUtil')
const urlUtil = require('../lib/urlutil')
const buildConfig = require('../constants/buildConfig')
const {getSetting} = require('../../js/settings')

// state helpers
const {makeImmutable, findNullKeyPaths} = require('../../app/common/state/immutableUtil')
@@ -555,6 +556,7 @@ const handleAppAction = (action) => {
}
break
case appConstants.APP_SAVE_SYNC_DEVICES:
const hasMainDevice = appState.getIn(['sync', 'devices']).some(device => device.get('mainDevice'))
for (let deviceId of Object.keys(action.devices)) {
const device = action.devices[deviceId]
if (device.lastRecordTimestamp) {
@@ -563,6 +565,9 @@ const handleAppAction = (action) => {
if (device.name) {
appState = appState.setIn(['sync', 'devices', deviceId, 'name'], device.name)
}
if (!hasMainDevice && getSetting(settings.SYNC_DEVICE_NAME) === device.name) {

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 11, 2018

Member

i think this line just sets the mainDevice to be true for any device with the same name as the current device, since SYNC_DEVICE_NAME is the name of the current device. is that the intended behavior?

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 11, 2018

Member

(as long as the current device doesn't know of any existing main devices)

appState = appState.setIn(['sync', 'devices', deviceId, 'mainDevice'], true)

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 9, 2018

Member

mainDevice should be added to docs/state.md

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 10, 2018

Author Member

done in 49113e5

}
}
break
case appConstants.APP_SAVE_SYNC_INIT_DATA:
@@ -609,6 +614,9 @@ const handleAppAction = (action) => {
appState = appState.setIn(['sync', 'devices'], {})
appState = appState.setIn(['sync', 'objectsById'], {})
break
case appConstants.APP_REMOVE_SYNC_DEVICE:
appState = appState.deleteIn(['sync', 'devices', action.deviceId])

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 9, 2018

Member

I don't know if this is the intended behavior, but since this does not delete the device seed, if you set up sync again after removing a device, it will re-join the old sync chain.

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 9, 2018

Member

this is probably not the right behavior because the text says 'Start a new sync chain' not 'Rejoin my old sync chain'

This comment has been minimized.

Copy link
@bradleyrichter

bradleyrichter Apr 10, 2018

Contributor

correct. It should start new, not rejoin.

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 10, 2018

Author Member

not sure if I get it but isn't the seed related to the main device only?

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 10, 2018

Author Member

so that option is for other devices (other than the main device). IIRC considering the above use case, if user setup sync again after removing a device, seed will be cleared given that removing the main device triggers RESET_SYNC.

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Apr 10, 2018

Member

@cezaraugusto

i think these were my test steps:

  1. join an existing sync chain (the one corresponding to bytes [0, 0, 0, 0...], so the current device is not the main device
  2. click Leave Sync Chain
  3. click Start a new sync chain
  4. click Computer
  5. it showed the old sync code

however i can't seem to repro this consistently

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 11, 2018

Author Member

sorry unable to repro but by clicking Leave Sync Chain you trigger DELETE_SYNC_USERwhich iirc clears all data in client. Maybe a response delay from the server?

Created #13799 for further investigation

break
case appConstants.APP_SETUP_SYNC_COMPLETED:
appState = appState.setIn(['sync', 'setupCompleted'], action.isCompleted)
break
@@ -71,6 +71,8 @@ describe('Preferences component unittest', function () {
mockery.registerMock('../../../extensions/brave/img/sync/add_device_titleicon.svg')
mockery.registerMock('../../../extensions/brave/img/sync/synccode_titleicon.svg')
mockery.registerMock('../../../extensions/brave/img/sync/hand_image.png')
mockery.registerMock('../../../extensions/brave/img/ledger/icon_remove.svg')
mockery.registerMock('../../../extensions/brave/img/sync/remove_device_titleicon.svg')

mockery.registerMock('electron', fakeElectron)

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.