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

batch synced site creation #7309

Merged
merged 2 commits into from
Feb 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/browser/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,13 @@ const doAction = (action) => {
}
})
break
case appConstants.APP_APPLY_SITE_RECORDS:
if (action.records.find((record) => record.objectData === 'bookmark')) {
appDispatcher.waitFor([appStore.dispatchToken], () => {
createMenu()
})
}
break
case appConstants.APP_ADD_SITE:
if (action.tag === siteTags.BOOKMARK || action.tag === siteTags.BOOKMARK_FOLDER) {
appDispatcher.waitFor([appStore.dispatchToken], () => {
Expand Down
12 changes: 12 additions & 0 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,18 @@ Dispatches a message when sync init data needs to be saved



### applySiteRecords(records)

Dispatches a message to apply a batch of site records from Brave Sync
TODO: Refactor this to merge it into addSite/removeSite

**Parameters**

**records**: `Array.<Object>`, Dispatches a message to apply a batch of site records from Brave Sync
TODO: Refactor this to merge it into addSite/removeSite



### resetSyncData()

Dispatches a message to delete sync data.
Expand Down
12 changes: 12 additions & 0 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,18 @@ const appActions = {
})
},

/**
* Dispatches a message to apply a batch of site records from Brave Sync
* TODO: Refactor this to merge it into addSite/removeSite
* @param {Array.<Object>} records
*/
applySiteRecords: function (records) {
AppDispatcher.dispatch({
actionType: appConstants.APP_APPLY_SITE_RECORDS,
records
})
},

/**
* Dispatches a message to delete sync data.
*/
Expand Down
1 change: 1 addition & 0 deletions js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const appConstants = {
APP_SET_STATE: _,
APP_REMOVE_SITE: _,
APP_MOVE_SITE: _,
APP_APPLY_SITE_RECORDS: _,
APP_MERGE_DOWNLOAD_DETAIL: _, // Sets an individual download detail
APP_CLEAR_COMPLETED_DOWNLOADS: _, // Removes all completed downloads
APP_ADD_PASSWORD: _, /** @param {Object} passwordDetail */
Expand Down
111 changes: 71 additions & 40 deletions js/state/syncUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

const Immutable = require('immutable')
const writeActions = require('../constants/sync/proto').actions
const siteTags = require('../constants/siteTags')
const siteUtil = require('./siteUtil')

const CATEGORY_MAP = {
Expand Down Expand Up @@ -51,20 +52,26 @@ const pickFields = (object, fields) => {
}, {})
}

// Cache of bookmark folder object IDs mapped to folder IDs
let folderIdMap = new Immutable.Map()

/**
* Apply a bookmark or historySite SyncRecord to the browser data store.
* Converts sync records into a form that can be consumed by AppStore.
* @param {Object} record
* @param {Immutable.Map} siteDetail
* @param {Immutable.Map} appState
*/
const applySiteRecord = (record) => {
const appActions = require('../actions/appActions')
const siteTags = require('../constants/siteTags')
module.exports.getSiteDataFromRecord = (record, appState) => {
const objectId = new Immutable.List(record.objectId)
const category = CATEGORY_MAP[record.objectData].categoryName
const existingObject = module.exports.getObjectById(objectId, category)
const existingObjectData = existingObject && existingObject[1]
let tag
let existingObjectData

if (record.action !== writeActions.CREATE) {
const existingObject = module.exports.getObjectById(objectId, category,
appState)
existingObjectData = existingObject && existingObject[1]
}

let tag
let siteProps = Object.assign(
{},
existingObjectData && existingObjectData.toJS(),
Expand All @@ -87,22 +94,11 @@ const applySiteRecord = (record) => {
const parentFolderObjectId = siteProps.parentFolderObjectId
if (parentFolderObjectId && parentFolderObjectId.length > 0) {
siteProps.parentFolderId =
getFolderIdByObjectId(new Immutable.List(parentFolderObjectId))
getFolderIdByObjectId(new Immutable.List(parentFolderObjectId), appState)
}
}
const siteDetail = new Immutable.Map(pickFields(siteProps, SITE_FIELDS))

switch (record.action) {
case writeActions.CREATE:
appActions.addSite(siteDetail, tag, undefined, undefined, true)
break
case writeActions.UPDATE:
appActions.addSite(siteDetail, tag, existingObjectData, null, true)
break
case writeActions.DELETE:
appActions.removeSite(siteDetail, tag, true)
break
}
return {siteDetail, tag, existingObjectData}
}

const applySiteSettingRecord = (record) => {
Expand Down Expand Up @@ -163,19 +159,29 @@ const applySiteSettingRecord = (record) => {
}
}

const applyNonBatchedRecords = (records) => {
if (!records.length) { return }
setImmediate(() => {
const record = records.shift()
applySyncRecord(record)
applyNonBatchedRecords(records)
})
}

/**
* Given a SyncRecord, apply it to the browser data store.
* @param {Object} record
*/
module.exports.applySyncRecord = (record) => {
const applySyncRecord = (record) => {
if (!record || !record.objectData) {
console.log(`Warning: Can't apply empty record: ${record}`)
return
}
switch (record.objectData) {
case 'bookmark':
case 'historySite':
applySiteRecord(record)
// these are handled in batches now
console.log(`Warning: Skipping unexpected site record: ${record}`)
break
case 'siteSetting':
applySiteSettingRecord(record)
Expand All @@ -194,11 +200,21 @@ module.exports.applySyncRecord = (record) => {
*/
module.exports.applySyncRecords = (records) => {
if (!records || records.length === 0) { return }
setImmediate(() => {
const record = records.shift()
module.exports.applySyncRecord(record)
module.exports.applySyncRecords(records)
const siteRecords = []
const otherRecords = []
records.forEach((record) => {
if (record && ['bookmark', 'historySite'].includes(record.objectData)) {
siteRecords.push(record)
} else {
otherRecords.push(record)
}
})
applyNonBatchedRecords(otherRecords)
if (siteRecords.length) {
setImmediate(() => {
require('../actions/appActions').applySiteRecords(new Immutable.List(siteRecords))
})
}
}

/**
Expand All @@ -212,15 +228,15 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
const AppStore = require('../stores/appStore')
const appState = AppStore.getState()
const objectId = new Immutable.List(syncRecord.objectId)
const existingObject = module.exports.getObjectById(objectId, categoryName)
const existingObject = module.exports.getObjectById(objectId, categoryName, appState)
if (!existingObject) { return null }

const existingObjectData = existingObject[1].toJS()
let item
switch (categoryName) {
case 'BOOKMARKS':
case 'HISTORY_SITES':
item = module.exports.createSiteData(existingObjectData)
item = module.exports.createSiteData(existingObjectData, appState)
break
case 'PREFERENCES':
const hostPattern = existingObject[0]
Expand All @@ -245,15 +261,18 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
* Given an objectId and category, return the matching browser object.
* @param {Immutable.List} objectId
* @param {string} category
* @param {Immutable.Map=} appState
* @returns {Array} [<Array>, <Immutable.Map>] array is AppStore searchKeyPath e.g. ['sites', 10] for use with updateIn
*/
module.exports.getObjectById = (objectId, category) => {
module.exports.getObjectById = (objectId, category, appState) => {
if (!(objectId instanceof Immutable.List)) {
throw new Error('objectId must be an Immutable.List')
}

const AppStore = require('../stores/appStore')
const appState = AppStore.getState()
if (!appState) {
const AppStore = require('../stores/appStore')
appState = AppStore.getState()
}
switch (category) {
case 'BOOKMARKS':
return appState.get('sites').findEntry((site, index) => {
Expand All @@ -280,12 +299,18 @@ module.exports.getObjectById = (objectId, category) => {
/**
* Given an bookmark folder objectId, find the folder and return its folderId.
* @param {Immutable.List} objectId
* @param {Immutable.Map=} appState
* @returns {number|undefined}
*/
const getFolderIdByObjectId = (objectId) => {
const entry = module.exports.getObjectById(objectId, 'BOOKMARKS')
const getFolderIdByObjectId = (objectId, appState) => {
if (folderIdMap.has(objectId)) {
return folderIdMap.get(objectId)
}
const entry = module.exports.getObjectById(objectId, 'BOOKMARKS', appState)
if (!entry) { return undefined }
return entry[1].get('folderId')
const folderId = entry[1].get('folderId')
folderIdMap = folderIdMap.set(objectId, folderId)
return folderId
}

/**
Expand Down Expand Up @@ -332,12 +357,16 @@ module.exports.newObjectId = (objectPath) => {
/**
* Given a bookmark folder's folderId, get or set its object ID.
* @param {number} folderId
* @param {Immutable.Map} appState
* @returns {Array.<number>}
*/
module.exports.findOrCreateFolderObjectId = (folderId) => {
const findOrCreateFolderObjectId = (folderId, appState) => {
if (typeof folderId !== 'number') { return undefined }
const AppStore = require('../stores/appStore')
const folder = AppStore.getState().getIn(['sites', folderId.toString()])
if (!appState) {
const AppStore = require('../stores/appStore')
appState = AppStore.getState()
}
const folder = appState.getIn(['sites', folderId.toString()])
if (!folder) { return undefined }
const objectId = folder.get('objectId')
if (objectId) {
Expand All @@ -350,9 +379,10 @@ module.exports.findOrCreateFolderObjectId = (folderId) => {
/**
* Converts a site object into input for sendSyncRecords
* @param {Object} site
* @param {Immutable.Map} appState
* @returns {{name: string, value: object, objectId: Array.<number>}}
*/
module.exports.createSiteData = (site) => {
module.exports.createSiteData = (site, appState) => {
const siteData = {
location: '',
title: '',
Expand All @@ -372,7 +402,8 @@ module.exports.createSiteData = (site) => {
}
if (module.exports.isSyncable('bookmark', Immutable.fromJS(site))) {
const objectId = site.objectId || module.exports.newObjectId(['sites', siteKey])
const parentFolderObjectId = site.parentFolderObjectId || (site.parentFolderId && module.exports.findOrCreateFolderObjectId(site.parentFolderId))
const parentFolderObjectId = site.parentFolderObjectId ||
(site.parentFolderId && findOrCreateFolderObjectId(site.parentFolderId, appState))
return {
name: 'bookmark',
objectId,
Expand Down
30 changes: 30 additions & 0 deletions js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ const ExtensionConstants = require('../../app/common/constants/extensionConstant
const AppDispatcher = require('../dispatcher/appDispatcher')
const appConfig = require('../constants/appConfig')
const settings = require('../constants/settings')
const writeActions = require('../constants/sync/proto').actions
const siteUtil = require('../state/siteUtil')
const syncUtil = require('../state/syncUtil')
const siteSettings = require('../state/siteSettings')
const appUrlUtil = require('../lib/appUrlUtil')
const electron = require('electron')
Expand Down Expand Up @@ -485,6 +487,34 @@ const handleAppAction = (action) => {
case appConstants.APP_DATA_URL_COPIED:
nativeImage.copyDataURL(action.dataURL, action.html, action.text)
break
case appConstants.APP_APPLY_SITE_RECORDS:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we leave a comment here about refactoring this into a single action with APP_ADD_SITE? When we do that we should probably pull it out of appStore anyway and create a siteReducer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe the comment should go on the action. Either way is fine, just want to capture it in the code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

action.records.forEach((record) => {
const siteData = syncUtil.getSiteDataFromRecord(record, appState)
const tag = siteData.tag
let siteDetail = siteData.siteDetail
const sites = appState.get('sites')
if (record.action !== writeActions.DELETE &&
!siteDetail.get('folderId') && siteUtil.isFolder(siteDetail)) {
siteDetail = siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
}
switch (record.action) {
case writeActions.CREATE:
appState = appState.set('sites',
siteUtil.addSite(sites, siteDetail, tag))
break
case writeActions.UPDATE:
appState = appState.set('sites',
siteUtil.addSite(sites, siteDetail, tag, siteData.existingObjectData))
break
case writeActions.DELETE:
appState = appState.set('sites',
siteUtil.removeSite(sites, siteDetail, tag))
break
}
})
appState = aboutNewTabState.setSites(appState)
appState = aboutHistoryState.setHistory(appState)
break
case appConstants.APP_ADD_SITE:
const oldSiteSize = appState.get('sites').size
const addSiteSyncCallback = action.skipSync ? undefined : syncActions.updateSite
Expand Down