Merge pull request #1109 from brave/sync-v2-flow-fix

Fix UI initialization flow and reset when devices < 2 from native side

components/brave_sync/brave_sync_service_impl.cc

@@ -469,6 +469,7 @@ BraveSyncServiceImpl::PrepareResolvedPreferences(const RecordsList& records) {
 void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
   const std::string this_device_id = sync_prefs_->GetThisDeviceId();
   bool this_device_deleted = false;
+  bool contains_only_one_device = false;
 
   auto sync_devices = sync_prefs_->GetSyncDevices();
   for (const auto &record : records) {
@@ -486,12 +487,14 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) {
         (record->deviceId == this_device_id &&
           record->action == jslib::SyncRecord::Action::A_DELETE &&
           actually_merged);
+      contains_only_one_device = sync_devices->size() < 2 &&
+        record->action == jslib::SyncRecord::Action::A_DELETE;
     }
   } // for each device
 
   sync_prefs_->SetSyncDevices(*sync_devices);
 
-  if (this_device_deleted)
+  if (this_device_deleted || contains_only_one_device)
     OnResetSync();
 }
 

components/brave_sync/brave_sync_service_impl.h

@@ -20,6 +20,8 @@ FORWARD_DECLARE_TEST(BraveSyncServiceTest, BookmarkDeleted);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, GetSyncWords);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, GetSeed);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnDeleteDevice);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice);
+FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnResetSync);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, ClientOnGetInitData);
 FORWARD_DECLARE_TEST(BraveSyncServiceTest, OnGetInitData);
@@ -94,6 +96,9 @@ class BraveSyncServiceImpl
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, GetSyncWords);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, GetSeed);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnDeleteDevice);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice);
+  FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest,
+                           OnDeleteDeviceWhenSelfDeleted);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnResetSync);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, ClientOnGetInitData);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, OnSaveBookmarksBaseOrder);

components/brave_sync/brave_sync_service_unittest.cc

@@ -323,7 +323,7 @@ TEST_F(BraveSyncServiceTest, GetSettingsAndDevices) {
             EXPECT_FALSE(settings->sync_bookmarks_);
             EXPECT_FALSE(settings->sync_settings_);
             EXPECT_FALSE(settings->sync_history_);
-            EXPECT_EQ(devices->devices_.size(), 0u);
+            EXPECT_EQ(devices->size(), 0u);
         }
   );
   sync_service()->GetSettingsAndDevices(callback1);
@@ -425,24 +425,88 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) {
   EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES",
       ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device3"))).Times(1);
   sync_service()->OnDeleteDevice("3");
+
+  RecordsList resolved_records;
+  auto resolved_record = SyncRecord::Clone(*records.at(2));
+  resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
+  resolved_records.push_back(std::move(resolved_record));
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
+  sync_service()->OnResolvedPreferences(resolved_records);
+
+  auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
+  EXPECT_TRUE(DevicesContains(devices_final.get(), "1", "device1"));
+  EXPECT_TRUE(DevicesContains(devices_final.get(), "2", "device2"));
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "3", "device3"));
+}
+
+TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) {
+  RecordsList records;
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "1", "device1"));
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "2", "device2"));
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
+  sync_service()->OnResolvedPreferences(records);
+
+  auto devices = sync_service()->sync_prefs_->GetSyncDevices();
+
+  EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "2", "device2"));
+
+  using brave_sync::jslib::SyncRecord;
   EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES",
       ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device2"))).Times(1);
   sync_service()->OnDeleteDevice("2");
 
   RecordsList resolved_records;
-  auto resolved_record1 = SyncRecord::Clone(*records.at(2));
-  resolved_record1->action = jslib::SyncRecord::Action::A_DELETE;
-  resolved_records.push_back(std::move(resolved_record1));
-  auto resolved_record2 = SyncRecord::Clone(*records.at(1));
-  resolved_record2->action = jslib::SyncRecord::Action::A_DELETE;
-  resolved_records.push_back(std::move(resolved_record2));
+  auto resolved_record = SyncRecord::Clone(*records.at(1));
+  resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
+  resolved_records.push_back(std::move(resolved_record));
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(3);
+  sync_service()->OnResolvedPreferences(resolved_records);
+
+  auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1"));
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2"));
+
+  EXPECT_FALSE(sync_service()->IsSyncConfigured());
+}
+
+TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) {
+  RecordsList records;
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "1", "device1"));
+  records.push_back(SimpleDeviceRecord(
+      jslib::SyncRecord::Action::A_CREATE,
+      "2", "device2"));
   EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1);
+  sync_service()->OnResolvedPreferences(records);
+
+  auto devices = sync_service()->sync_prefs_->GetSyncDevices();
+
+  EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1"));
+  EXPECT_TRUE(DevicesContains(devices.get(), "2", "device2"));
+
+  using brave_sync::jslib::SyncRecord;
+  EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES",
+      ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device1"))).Times(1);
+  sync_service()->OnDeleteDevice("1");
+
+  RecordsList resolved_records;
+  auto resolved_record = SyncRecord::Clone(*records.at(0));
+  resolved_record->action = jslib::SyncRecord::Action::A_DELETE;
+  resolved_records.push_back(std::move(resolved_record));
+  EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(3);
   sync_service()->OnResolvedPreferences(resolved_records);
 
   auto devices_final = sync_service()->sync_prefs_->GetSyncDevices();
-  EXPECT_TRUE(DevicesContains(devices_final.get(), "1", "device1"));
+  EXPECT_FALSE(DevicesContains(devices_final.get(), "1", "device1"));
   EXPECT_FALSE(DevicesContains(devices_final.get(), "2", "device2"));
-  EXPECT_FALSE(DevicesContains(devices_final.get(), "3", "device3"));
+
+  EXPECT_FALSE(sync_service()->IsSyncConfigured());
 }
 
 TEST_F(BraveSyncServiceTest, OnResetSync) {

components/brave_sync/sync_devices.h

@@ -42,6 +42,7 @@ class SyncDevices {
    std::unique_ptr<base::Value> ToValue() const;
    std::unique_ptr<base::Value> ToValueArrOnly() const;
    std::string ToJson() const;
+   size_t size() const { return devices_.size(); }
    void FromJson(const std::string &str_json);
    void Merge(const SyncDevice& device, int action, bool* actually_merged);
 

components/brave_sync/ui/actions/sync_actions.ts

@@ -50,22 +50,9 @@ export const onSetupNewToSync = (thisDeviceName: string) => {
   return action(types.SYNC_ON_SETUP_NEW_TO_SYNC, { thisDeviceName })
 }
 
-/**
- * Dispatches a message telling the back-end that the user requested the QR code
- */
-export const onRequestQRCode = () => {
-  return action(types.SYNC_ON_REQUEST_QR_CODE)
-}
-
 export const onGenerateQRCodeImageSource = (imageSource: string) => {
   return action(types.SYNC_ON_GENERATE_QR_CODE_IMAGE_SOURCE, { imageSource })
 }
-/**
- * Dispatches a message telling the back-end that the user requested the sync words
- */
-export const onRequestSyncWords = () => {
-  return action(types.SYNC_ON_REQUEST_SYNC_WORDS)
-}
 
 /**
  * Dispatches a message telling the back-end that the user removed a given device

components/brave_sync/ui/components/enabledContent.tsx

@@ -60,17 +60,6 @@ export default class SyncEnabledContent extends React.PureComponent<Props, State
     }
   }
 
-   componentDidUpdate () {
-    // immediately request qr code and sync words
-    // in case they aren't already. this could happen if user
-    // had the sync word where the requests are stopped due to sync reset
-    const { seedQRImageSource, syncWords } = this.props.syncData
-    if (!seedQRImageSource && !syncWords) {
-       this.props.actions.onRequestQRCode()
-       this.props.actions.onRequestSyncWords()
-    }
-  }
-
   getRows = (devices?: any): Row[] | undefined => {
     if (!devices) {
       return

components/brave_sync/ui/components/modals/addNewChainCameraOption.tsx

@@ -105,9 +105,9 @@ export default class AddNewChainCameraOptionModal extends React.PureComponent<Pr
               type='accent'
               size='medium'
               onClick={onClose}
-              disabled={syncData.devices.length < 2}
+              disabled={!syncData.isSyncConfigured}
               text={
-                syncData.devices.length < 2
+                !syncData.isSyncConfigured
                 ? getLocale('lookingForDevice')
                 : getLocale('ok')
               }

components/brave_sync/ui/components/modals/deviceType.tsx

@@ -58,30 +58,18 @@ export default class DeviceTypeModal extends React.PureComponent<Props, State> {
     }
   }
 
-   componentDidUpdate () {
-    // once this screen is rendered and component is updated,
-    // request sync qr code and words so it can be seen immediately
-    const { seedQRImageSource, syncWords } = this.props.syncData
-    if (!syncWords) {
-      this.props.actions.onRequestSyncWords()
-    }
-    if (!seedQRImageSource) {
-      this.props.actions.onRequestQRCode()
-    }
-  }
-
   onUserNoticedError = () => {
     this.props.actions.resetSyncSetupError()
     this.props.onClose()
   }
 
   onClickClose = () => {
-    const { devices } = this.props.syncData
+    const { devices, isSyncConfigured } = this.props.syncData
     // sync is enabled when at least 2 devices are in the chain.
     // this modal works both with sync enabled and disabled states.
     // in case user opens it in the enabled content screen,
     // check there are 2 devices in chain before reset
-    if (devices.length < 2) {
+    if (isSyncConfigured && devices.length < 2) {
       this.props.actions.onSyncReset()
     }
     this.props.onClose()

components/brave_sync/ui/components/modals/scanCode.tsx

@@ -96,9 +96,9 @@ export default class ScanCodeModal extends React.PureComponent<Props, State> {
               type='accent'
               size='medium'
               onClick={onClose}
-              disabled={syncData.devices.length < 2}
+              disabled={!syncData.isSyncConfigured}
               text={
-                syncData.devices.length < 2
+                !syncData.isSyncConfigured
                 ? getLocale('lookingForDevice')
                 : getLocale('ok')
               }

components/brave_sync/ui/constants/sync_types.ts

@@ -8,9 +8,7 @@ export const enum types {
   SYNC_ON_HAVE_SYNC_WORDS = '@@sync/SYNC_ON_HAVE_SYNC_WORDS',
   SYNC_ON_HAVE_SEED_FOR_QR_CODE = '@@sync/SYNC_ON_HAVE_SEED_FOR_QR_CODE',
   SYNC_ON_SETUP_NEW_TO_SYNC = '@@sync/SYNC_ON_SETUP_NEW_TO_SYNC',
-  SYNC_ON_REQUEST_QR_CODE = '@@sync/SYNC_ON_REQUEST_QR_CODE',
   SYNC_ON_GENERATE_QR_CODE_IMAGE_SOURCE = '@@sync/SYNC_ON_GENERATE_QR_CODE_IMAGE_SOURCE',
-  SYNC_ON_REQUEST_SYNC_WORDS = '@@sync/SYNC_ON_REQUEST_SYNC_WORDS',
   SYNC_ON_REMOVE_DEVICE = '@@sync/SYNC_ON_REMOVE_DEVICE',
   SYNC_ON_RESET = '@@sync/SYNC_ON_RESET',
   SYNC_BOOKMARKS = '@@sync/SYNC_BOOKMARKS',

components/brave_sync/ui/reducers/sync_reducer.ts

@@ -44,6 +44,20 @@ const syncReducer: Reducer<Sync.State | undefined> = (state: Sync.State | undefi
         syncSavedSiteSettings: payload.settings.sync_settings,
         syncBrowsingHistory: payload.settings.sync_history
       }
+
+      if (state.isSyncConfigured && !state.syncWords) {
+        chrome.send('needSyncWords')
+      }
+      if (state.isSyncConfigured && !state.seedQRImageSource) {
+        chrome.send('needSyncQRcode')
+      }
+
+      // if this device is deleted or only one device on sync chain,
+      // BraveSyncService will do reset but we must clear
+      // previous sync state
+      if (!state.isSyncConfigured) {
+        state = { ...storage.defaultState }
+      }
       break
 
     case types.SYNC_ON_HAVE_SEED_FOR_QR_CODE:
@@ -68,14 +82,6 @@ const syncReducer: Reducer<Sync.State | undefined> = (state: Sync.State | undefi
       chrome.send('setupSyncNewToSync', [payload.thisDeviceName])
       break
 
-    case types.SYNC_ON_REQUEST_QR_CODE:
-      chrome.send('needSyncQRcode')
-      break
-
-    case types.SYNC_ON_REQUEST_SYNC_WORDS:
-      chrome.send('needSyncWords')
-      break
-
     case types.SYNC_ON_RESET:
       chrome.send('resetSync')
       // sync is reset. clear all data
@@ -86,18 +92,6 @@ const syncReducer: Reducer<Sync.State | undefined> = (state: Sync.State | undefi
       if (typeof payload.id === 'undefined' || typeof payload.deviceName === 'undefined') {
         break
       }
-
-      // if the device removed is the this device, reset sync
-      if (payload.id === state.thisDeviceId) {
-        state = { ...storage.defaultState }
-        chrome.send('resetSync')
-        break
-      }
-
-      state = {
-        ...state,
-        devices: [ ...state.devices.filter((device: Sync.Devices) => device.id !== payload.id) ]
-      }
       chrome.send('deleteDevice', [payload.id])
       break
 

components/test/brave_sync_ui/actions/sync_actions_test.ts

@@ -58,10 +58,6 @@ describe('sync_actions', () => {
     })
   })
 
-  it('onRequestQRCode', () => {
-    expect(actions.onRequestQRCode).toEqual(expect.any(Function))
-  })
-
   it('onGenerateQRCodeImageSource', () => {
     expect(actions.onGenerateQRCodeImageSource('someImageConvertedToBase64')).toEqual({
       type: types.SYNC_ON_GENERATE_QR_CODE_IMAGE_SOURCE,
@@ -70,10 +66,6 @@ describe('sync_actions', () => {
     })
   })
 
-  it('onRequestSyncWords', () => {
-    expect(actions.onRequestSyncWords).toEqual(expect.any(Function))
-  })
-
   it('onRemoveDevice', () => {
     expect(actions.onRemoveDevice(1337)).toEqual({
       type: types.SYNC_ON_REMOVE_DEVICE,

components/test/brave_sync_ui/reducers/sync_reducer_test.ts

@@ -31,14 +31,6 @@ describe.skip('syncReducer', () => {
     // TODO
   })
 
-  describe('SYNC_ON_REQUEST_QR_CODE', () => {
-    // TODO
-  })
-
-  describe('SYNC_ON_REQUEST_SYNC_WORDS', () => {
-    // TODO
-  })
-
   describe('SYNC_ON_RESET', () => {
     // TODO
   })