Revert "handle empty objects in sync DELETEs"

This reverts commit 91ef559.

it also re-fixes #9308 after the revert and fixes handling of skipSync (#111)

Test Plan:
Test plan in brave/sync#111

app/browser/reducers/sitesReducer.js

@@ -9,6 +9,7 @@ const filtering = require('../../filtering')
 const siteCache = require('../../common/state/siteCache')
 const siteTags = require('../../../js/constants/siteTags')
 const siteUtil = require('../../../js/state/siteUtil')
+const syncActions = require('../../../js/actions/syncActions')
 const syncUtil = require('../../../js/state/syncUtil')
 const Immutable = require('immutable')
 const settings = require('../../../js/constants/settings')
@@ -83,7 +84,8 @@ const sitesReducer = (state, action, immutableAction) => {
       state = updateActiveTabBookmarked(state)
       break
     case appConstants.APP_REMOVE_SITE:
-      state = siteUtil.removeSite(state, action.siteDetail, action.tag, true)
+      const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
+      state = siteUtil.removeSite(state, action.siteDetail, action.tag, true, removeSiteSyncCallback)
       if (syncEnabled()) {
         state = syncUtil.updateSiteCache(state, action.siteDetail)
       }
@@ -100,7 +102,6 @@ const sitesReducer = (state, action, immutableAction) => {
       }
       break
     case appConstants.APP_APPLY_SITE_RECORDS:
-      // Applies site records fetched by sync
       let nextFolderId = siteUtil.getNextFolderId(state.get('sites'))
       // Ensure that all folders are assigned folderIds
       action.records.forEach((record, i) => {

app/sync.js

@@ -10,7 +10,6 @@ const qr = require('qr-image')
 const ipcMain = electron.ipcMain
 const locale = require('./locale')
 const messages = require('../js/constants/messages')
-const siteTags = require('../js/constants/siteTags')
 const syncMessages = require('../js/constants/sync/messages')
 const categories = require('../js/constants/sync/proto').categories
 const writeActions = require('../js/constants/sync/proto').actions
@@ -28,10 +27,11 @@ const extensions = require('./extensions')
 
 const CATEGORY_MAP = syncUtil.CATEGORY_MAP
 const CATEGORY_NAMES = Object.keys(categories)
+const SYNC_ACTIONS = Object.values(syncConstants)
 
 // Fields that should trigger a sync SEND when changed
 const SYNC_FIELDS = {
-  sites: ['location', 'customTitle', 'folderId', 'parentFolderId'],
+  sites: ['location', 'customTitle', 'folderId', 'parentFolderId', 'tags'],
   siteSettings: Object.keys(syncUtil.siteSettingDefaults)
 }
 
@@ -53,15 +53,12 @@ let pollIntervalId = null
 let deviceIdSent = false
 let bookmarksToolbarShown = false
 
-// Syncs state diffs to the sync server if needed
+// Determines what to sync
 const appStoreChangeCallback = function (diffs) {
   if (!backgroundSender || !diffs) {
     return
   }
 
-  // Site locations created by the diffs
-  let createdSiteLocations = []
-
   diffs.forEach((diff) => {
     if (!diff || !diff.path) {
       return
@@ -73,92 +70,54 @@ const appStoreChangeCallback = function (diffs) {
     }
 
     const type = path[1]
+    const field = path[3]
     const isSite = type === 'sites'
-    const siteLocation = isSite ? path[2].split('|')[0] : null
 
     const fieldsToPick = SYNC_FIELDS[type]
     if (!fieldsToPick) {
       return
     }
 
-    let action = null
-    let maybeSkipSync = false
-    if (siteLocation && diff.op !== 'remove') {
-      createdSiteLocations.push(siteLocation)
+    const statePath = path.slice(1, 3).map((item) => item.replace(/~1/g, '/'))
+    if (field === 'skipSync' && diff.value === true) {
+      // Remove the flag so that this gets synced next time it is updated
+      appActions.setSkipSync(statePath, false)
+      return
     }
 
-    // XXX: adding/removing a tag (e.g. 'bookmark') corresponds to adding/deleting a site record in sync
-    if (path.length === 3 || path[3] === 'tags') {
-      if (diff.op === 'add') {
-        maybeSkipSync = true
-        if (isSite) {
-          // Send UPDATE instead of CREATE for sites to work around sync#111
-          action = writeActions.UPDATE
-        } else {
-          action = writeActions.CREATE
-        }
-      } else if (diff.op === 'remove') {
-        if (path.length === 3 && createdSiteLocations.includes(siteLocation)) {
-          // We are removing a site that is created in the same diff set.
-          // This probably means we are moving a site, not actually deleting
-          // it, so ignore this update.
-          console.log('Skipping site deletion', siteLocation)
-          return
-        }
-        action = writeActions.DELETE
-      } else {
-        action = writeActions.UPDATE
+    const isInsert = diff.op === 'add' && path.length === 3
+    const isUpdate = fieldsToPick.includes(field) // Ignore insignicant updates
+
+    // DELETES are handled in appState because the old object is no longer
+    // available by the time emitChanges is received
+    if (isInsert || isUpdate) {
+      // Get the item's path and entry in appStore
+      const entry = AppStore.getState().getIn(statePath)
+      if (!entry || !entry.toJS) {
+        return
+      }
+      if (entry.get('skipSync')) {
+        // Remove the flag so that this gets synced next time it is updated
+        appActions.setSkipSync(statePath, false)
+        return
       }
-    } else if (fieldsToPick.includes(path[3])) {
-      action = writeActions.UPDATE
-    }
 
-    if (action === null) {
-      return
-    }
+      let action = null
 
-    const statePath = path.slice(1, 3).map((item) => item.replace(/~1/g, '/'))
-    const state = AppStore.getState()
-    const entry = state.getIn(statePath)
+      if (isInsert) {
+        // Send UPDATE instead of CREATE for sites to work around sync#111
+        action = isSite ? writeActions.UPDATE : writeActions.CREATE
+      } else if (isUpdate) {
+        action = writeActions.UPDATE
+      }
 
-    if (maybeSkipSync && entry && entry.get('skipSync')) {
-      // Don't re-create objects that were fetched by sync
-      return
-    }
+      if (action !== null) {
+        // Set the object ID if there is not already one
+        const entryJS = entry.toJS()
+        entryJS.objectId = entryJS.objectId || syncUtil.newObjectId(statePath)
 
-    if (isSite && action === writeActions.DELETE && !entry) {
-      // If we deleted the site, it is no longer availble in appState.
-      // Find the corresponding objectId using the sync cache
-      // and send this in the sync record
-      const objectId = syncUtil.siteKeyToObjectId(state, statePath[1])
-      if (objectId) {
-        // Delete the site from both history and bookmarks
-        sendSyncRecords(backgroundSender, action, [{
-          name: 'bookmark',
-          objectId,
-          value: {}
-        }])
-        sendSyncRecords(backgroundSender, action, [{
-          name: 'historySite',
-          objectId,
-          value: {}
-        }])
-      }
-    } else if (entry && entry.toJS) {
-      const entryJS = entry.toJS()
-      if (action === writeActions.DELETE && isSite) {
-        const tags = entryJS.tags || []
-        sendSyncRecords(backgroundSender, action, [{
-          objectId: entryJS.objectId,
-          value: {},
-          name: tags.includes(siteTags.BOOKMARK) || siteTags.includes(siteTags.BOOKMARK_FOLDER)
-            ? 'historySite' // if the site is still a bookmark, it must have been deleted from history
-            : 'bookmark'
-        }])
-      } else {
-        sendSyncRecords(backgroundSender, action, [
-          isSite ? syncUtil.createSiteData(entryJS, state) : syncUtil.createSiteSettingsData(statePath[1], entryJS)
-        ])
+        sendSyncRecords(backgroundSender, action,
+          [type === 'sites' ? syncUtil.createSiteData(entryJS) : syncUtil.createSiteSettingsData(statePath[1], entryJS)])
       }
     }
   })
@@ -168,7 +127,7 @@ const appStoreChangeCallback = function (diffs) {
  * Sends sync records of the same category to the sync server.
  * @param {event.sender} sender
  * @param {number} action
- * @param {Array.<{objectId: Array, name: string, value: Object}>} data
+ * @param {Array.<{name: string, value: Object}>} data
  */
 const sendSyncRecords = (sender, action, data) => {
   if (!deviceId) {
@@ -183,7 +142,7 @@ const sendSyncRecords = (sender, action, data) => {
     return
   }
   sender.send(syncMessages.SEND_SYNC_RECORDS, category.categoryName, data.map((item) => {
-    if (!item || !item.name || !item.value || !item.objectId) {
+    if (!item || !item.name || !item.value) {
       return
     }
     return {
@@ -195,6 +154,34 @@ const sendSyncRecords = (sender, action, data) => {
   }))
 }
 
+/**
+ * @param {Object} action
+ * @returns {boolean}
+ */
+const validateAction = (action) => {
+  const SYNC_ACTIONS_WITHOUT_ITEMS = [
+    syncConstants.SYNC_CLEAR_HISTORY,
+    syncConstants.SYNC_CLEAR_SITE_SETTINGS
+  ]
+  if (SYNC_ACTIONS.includes(action.actionType) !== true) {
+    return false
+  }
+
+  // If the action requires an item, validate the item.
+  if (SYNC_ACTIONS_WITHOUT_ITEMS.includes(action.actionType) !== true) {
+    if (!action.item || !action.item.toJS) {
+      log('Missing item!')
+      return false
+    }
+    // Only accept items who have an objectId set already
+    if (!action.item.get('objectId')) {
+      log(`Missing object ID! ${action.item.toJS()}`)
+      return false
+    }
+  }
+  return true
+}
+
 const dispatcherCallback = (action) => {
   if (!backgroundSender) {
     return
@@ -206,10 +193,14 @@ const dispatcherCallback = (action) => {
     }
   }
   // If sync is not enabled, the following actions should be ignored.
-  if (!syncEnabled() || backgroundSender.isDestroyed()) {
+  if (!syncEnabled() || validateAction(action) !== true || backgroundSender.isDestroyed()) {
     return
   }
   switch (action.actionType) {
+    case syncConstants.SYNC_REMOVE_SITE:
+      sendSyncRecords(backgroundSender, writeActions.DELETE,
+        [syncUtil.createSiteData(action.item.toJS())])
+      break
     case syncConstants.SYNC_CLEAR_HISTORY:
       backgroundSender.send(syncMessages.DELETE_SYNC_CATEGORY, CATEGORY_MAP.historySite.categoryName)
       break
@@ -343,6 +334,7 @@ module.exports.init = function (appState) {
   }
   // sent by about:preferences when sync should be reloaded
   ipcMain.on(messages.RELOAD_SYNC_EXTENSION, () => {
+    console.log('reloading sync')
     extensions.reloadExtension(syncExtensionId)
   })
   // sent by about:preferences when resetting sync

docs/appActions.md

@@ -393,6 +393,18 @@ Removes a site setting
 
 
 
+### setSkipSync(path, skipSync) 
+
+Changes the skipSync flag on an appState path.
+
+**Parameters**
+
+**path**: `Array.<string>`, Changes the skipSync flag on an appState path.
+
+**skipSync**: `boolean`, Changes the skipSync flag on an appState path.
+
+
+
 ### updateLedgerInfo(ledgerInfo) 
 
 Updates ledger information for the payments pane

js/actions/appActions.js

@@ -485,6 +485,19 @@ const appActions = {
     })
   },
 
+  /**
+   * Changes the skipSync flag on an appState path.
+   * @param {Array.<string>} path
+   * @param {boolean} skipSync
+   */
+  setSkipSync: function (path, skipSync) {
+    dispatch({
+      actionType: appConstants.APP_SET_SKIP_SYNC,
+      path,
+      skipSync
+    })
+  },
+
   /**
    * Updates ledger information for the payments pane
    * @param {object} ledgerInfo - the current ledger state

js/actions/syncActions.js

@@ -7,6 +7,13 @@ const AppDispatcher = require('../dispatcher/appDispatcher')
 const syncConstants = require('../constants/syncConstants')
 
 const syncActions = {
+  removeSite: function (item) {
+    AppDispatcher.dispatch({
+      actionType: syncConstants.SYNC_REMOVE_SITE,
+      item
+    })
+  },
+
   clearHistory: function () {
     AppDispatcher.dispatch({
       actionType: syncConstants.SYNC_CLEAR_HISTORY

js/constants/appConstants.js

@@ -135,7 +135,8 @@ const appConstants = {
   APP_UPDATE_LOG_OPENED: _,
   APP_URL_BAR_SELECTED_INDEX_CHANGED: _,
   APP_ON_TOGGLE_BROWSING_DATA: _,
-  APP_ON_CANCEL_BROWSING_DATA: _
+  APP_ON_CANCEL_BROWSING_DATA: _,
+  APP_SET_SKIP_SYNC: _
 }
 
 module.exports = mapValuesByKeys(appConstants)

js/constants/syncConstants.js

@@ -6,6 +6,7 @@ const mapValuesByKeys = require('../lib/functional').mapValuesByKeys
 
 const _ = null
 const syncConstants = {
+  SYNC_REMOVE_SITE: _,  /** @param {Immutable.Map} item */
   SYNC_CLEAR_HISTORY: _,
   SYNC_CLEAR_SITE_SETTINGS: _
 }

js/state/siteUtil.js

@@ -369,14 +369,18 @@ module.exports.removeSiteByObjectId = function (state, objectId, objectData) {
  * @param {Immutable.Map} siteDetail The siteDetail to remove a tag from
  * @param {string} tag
  * @param {boolean} reorder whether to reorder sites (default with reorder)
+ * @param {Function=} syncCallback
  * @return {Immutable.Map} The new state Immutable object
  */
-module.exports.removeSite = function (state, siteDetail, tag, reorder = true) {
+module.exports.removeSite = function (state, siteDetail, tag, reorder = true, syncCallback) {
   let sites = state.get('sites')
   const key = module.exports.getSiteKey(siteDetail)
   if (!key) {
     return state
   }
+  if (getSetting(settings.SYNC_ENABLED) === true && syncCallback) {
+    syncCallback(sites.getIn([key]))
+  }
 
   const tags = sites.getIn([key, 'tags'])
   if (isBookmarkFolder(tags)) {
@@ -385,7 +389,7 @@ module.exports.removeSite = function (state, siteDetail, tag, reorder = true) {
     childSites.forEach((site) => {
       const tags = site.get('tags')
       tags.forEach((tag) => {
-        state = module.exports.removeSite(state, site, tag, false)
+        state = module.exports.removeSite(state, site, tag, false, syncCallback)
       })
     })
   }