From 47741e75fa865cdd8f908a2aa32154e833f98520 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sun, 18 Jun 2017 05:49:25 -0400 Subject: [PATCH] Restore locally deleted collections and searches that changed remotely Also restore items that were in the collections --- .../content/zotero/xpcom/sync/syncEngine.js | 98 +++++++++++++++++-- chrome/content/zotero/xpcom/sync/syncLocal.js | 10 +- test/content/support.js | 2 +- test/tests/syncEngineTest.js | 93 ++++++++++++++++++ test/tests/syncLocalTest.js | 32 ++++++ 5 files changed, 222 insertions(+), 13 deletions(-) diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 34b0fc81e9..6f6042c04c 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -463,7 +463,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou * * @return {Promise} - A download result code (this.DOWNLOAD_RESULT_*) */ -Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) { +Zotero.Sync.Data.Engine.prototype._downloadObjects = async function (objectType, keys) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); var remainingKeys = [...keys]; @@ -506,12 +506,13 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu ); var conflicts = []; + var restored = []; var num = 0; // Process batches of object data as they're available, one at a time - yield Zotero.Promise.map( + await Zotero.Promise.map( json, - Zotero.Promise.coroutine(function* (batch) { + async function (batch) { this._failedCheck(); Zotero.debug(`Processing batch of downloaded ${objectTypePlural} in ${this.library.name}`); @@ -527,7 +528,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu }); // Process objects - let results = yield Zotero.Sync.Data.Local.processObjectsFromJSON( + let results = await Zotero.Sync.Data.Local.processObjectsFromJSON( objectType, this.libraryID, batch, @@ -563,6 +564,11 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu // If data was processed, remove JSON if (x.processed) { delete objectData[x.key]; + + // We'll need to add items back to restored collections + if (x.restored) { + restored.push(x.key); + } } // If object shouldn't be retried, mark as processed if (x.processed || !x.retry) { @@ -574,12 +580,18 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu }); remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, processedKeys); conflicts.push(...conflictResults); - }.bind(this)), + }.bind(this), { concurrency: 1 } ); + // If any locally deleted collections were restored, either add them back to the collection + // (if the items still exist) or remove them from the delete log and add them to the sync queue + if (restored.length && objectType == 'collection') { + await this._restoreRestoredCollectionItems(restored); + } + this._failedCheck(); // If all requests were successful, such that we had a chance to see all keys, remove keys we @@ -590,7 +602,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu Zotero.debug(`Removing ${missingKeys.length} missing ` + Zotero.Utilities.pluralize(missingKeys.length, [objectType, objectTypePlural]) + " from sync queue"); - yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, missingKeys); + await Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, missingKeys); remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, missingKeys); } } @@ -602,7 +614,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu Zotero.debug(`Queueing ${failedKeys.length} failed ` + Zotero.Utilities.pluralize(failedKeys.length, [objectType, objectTypePlural]) + " for later", 2); - yield Zotero.Sync.Data.Local.addObjectsToSyncQueue( + await Zotero.Sync.Data.Local.addObjectsToSyncQueue( objectType, this.libraryID, failedKeys ); @@ -629,7 +641,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu // Show conflict resolution window if (conflicts.length) { - let results = yield Zotero.Sync.Data.Local.processConflicts( + let results = await Zotero.Sync.Data.Local.processConflicts( objectType, this.libraryID, conflicts, this._getOptions() ); // Keys can be unprocessed if conflict resolution is cancelled @@ -637,11 +649,77 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu if (!keys.length) { throw new Zotero.Sync.UserCancelledException(); } - yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, keys); + await Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, keys); } return this.DOWNLOAD_RESULT_CONTINUE; -}); +}; + + +/** + * If a collection is deleted locally but modified remotely between syncs, the local collection is + * restored, but collection membership is a property of items, the local items that were previously + * in that collection won't be any longer (or they might have been deleted along with the collection), + * so we have to get the current collection items from the API and either add them back + * (if they exist) or clear them from the delete log and mark them for download. + * + * Remote items in the trash aren't currently restored and will be removed from the collection when the + * local collection-item removal syncs up. + */ +Zotero.Sync.Data.Engine.prototype._restoreRestoredCollectionItems = async function (collectionKeys) { + for (let collectionKey of collectionKeys) { + let { keys: itemKeys } = await this.apiClient.getKeys( + this.library.libraryType, + this.libraryTypeID, + { + target: `collections/${collectionKey}/items/top`, + format: 'keys' + } + ); + + if (itemKeys.length) { + let collection = Zotero.Collections.getByLibraryAndKey(this.libraryID, collectionKey); + let addToCollection = []; + let addToQueue = []; + for (let itemKey of itemKeys) { + let o = Zotero.Items.getByLibraryAndKey(this.libraryID, itemKey); + if (o) { + addToCollection.push(o.id); + // Remove item from trash if it's there, since it's not in the trash remotely. + // (This would happen if items were moved to the trash along with the collection + // deletion.) + if (o.deleted) { + o.deleted = false + await o.saveTx(); + } + } + else { + addToQueue.push(itemKey); + } + } + if (addToCollection.length) { + Zotero.debug(`Restoring ${addToCollection.length} ` + + `${Zotero.Utilities.pluralize(addToCollection.length, ['item', 'items'])} ` + + `to restored collection ${collection.libraryKey}`); + await Zotero.DB.executeTransaction(function* () { + yield collection.addItems(addToCollection); + }.bind(this)); + } + if (addToQueue.length) { + Zotero.debug(`Restoring ${addToQueue.length} deleted ` + + `${Zotero.Utilities.pluralize(addToQueue.length, ['item', 'items'])} ` + + `in restored collection ${collection.libraryKey}`); + await Zotero.Sync.Data.Local.removeObjectsFromDeleteLog( + 'item', this.libraryID, addToQueue + ); + await Zotero.Sync.Data.Local.addObjectsToSyncQueue( + 'item', this.libraryID, addToQueue + ); + } + } + } +}; + /** diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index a08d784e98..91d52e4add 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -665,6 +665,7 @@ Zotero.Sync.Data.Local = { * {Boolean} processed * {Object} [error] * {Boolean} [retry] + * {Boolean} [restored=false] - Locally deleted object was added back * {Boolean} [conflict=false] * {Object} [left] - Local JSON data for conflict (or .deleted and .dateDeleted) * {Object} [right] - Remote JSON data for conflict @@ -783,6 +784,7 @@ Zotero.Sync.Data.Local = { let obj = yield objectsClass.getByLibraryAndKeyAsync( libraryID, objectKey, { noCache: true } ); + let restored = false; if (obj) { Zotero.debug("Matching local " + objectType + " exists", 4); @@ -921,13 +923,14 @@ Zotero.Sync.Data.Local = { // Auto-restore some locally deleted objects that have changed remotely case 'collection': case 'search': + Zotero.debug(`${ObjectType} ${objectKey} was modified remotely ` + + '-- restoring'); yield this.removeObjectsFromDeleteLog( objectType, libraryID, [objectKey] ); - - throw new Error("Unimplemented"); + restored = true; break; default: @@ -946,6 +949,9 @@ Zotero.Sync.Data.Local = { } let saveResults = yield this._saveObjectFromJSON(obj, jsonObject, saveOptions); + if (restored) { + saveResults.restored = true; + } results.push(saveResults); if (!saveResults.processed) { throw saveResults.error; diff --git a/test/content/support.js b/test/content/support.js index 002e62df68..0fad22b671 100644 --- a/test/content/support.js +++ b/test/content/support.js @@ -393,7 +393,7 @@ function createUnsavedDataObject(objectType, params = {}) { var itemType; if (objectType == 'item' || objectType == 'feedItem') { itemType = params.itemType || 'book'; - allowedParams.push('dateAdded', 'dateModified'); + allowedParams.push('deleted', 'dateAdded', 'dateModified'); } if (objectType == 'item') { allowedParams.push('inPublications'); diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 831d73050f..855c1a2617 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -2188,6 +2188,99 @@ describe("Zotero.Sync.Data.Engine", function () { var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, libraryID); assert.sameMembers(keys, ['BBBBBBBB']); }); + + + it("should add items that exist remotely in a locally deleted, remotely modified collection back to collection", async function () { + ({ engine, client, caller } = await setup({ + stopOnError: false + })); + var libraryID = Zotero.Libraries.userLibraryID; + + var collection = await createDataObject('collection'); + var collectionKey = collection.key; + await collection.eraseTx(); + var item1 = await createDataObject('item'); + var item2 = await createDataObject('item', { deleted: true }); + + var headers = { + "Last-Modified-Version": 5 + }; + setResponse({ + method: "GET", + url: `users/1/collections?format=json&collectionKey=${collectionKey}`, + status: 200, + headers, + json: [ + makeCollectionJSON({ + key: collectionKey, + version: 5, + name: "A" + }) + ] + }); + setResponse({ + method: "GET", + url: `users/1/collections/${collectionKey}/items/top?format=keys`, + status: 200, + headers, + text: item1.key + "\n" + item2.key + "\n" + }); + await engine._downloadObjects('collection', [collectionKey]); + + var collection = Zotero.Collections.getByLibraryAndKey(libraryID, collectionKey); + assert.sameMembers(collection.getChildItems(true), [item1.id, item2.id]); + // Item should be removed from trash + assert.isFalse(item2.deleted); + }); + + + it("should add locally deleted items that exist remotely in a locally deleted, remotely modified collection to sync queue and remove from delete log", async function () { + ({ engine, client, caller } = await setup({ + stopOnError: false + })); + var libraryID = Zotero.Libraries.userLibraryID; + + var collection = await createDataObject('collection'); + var collectionKey = collection.key; + await collection.eraseTx(); + var item = await createDataObject('item'); + await item.eraseTx(); + + var headers = { + "Last-Modified-Version": 5 + }; + setResponse({ + method: "GET", + url: `users/1/collections?format=json&collectionKey=${collectionKey}`, + status: 200, + headers, + json: [ + makeCollectionJSON({ + key: collectionKey, + version: 5, + name: "A" + }) + ] + }); + setResponse({ + method: "GET", + url: `users/1/collections/${collectionKey}/items/top?format=keys`, + status: 200, + headers, + text: item.key + "\n" + }); + await engine._downloadObjects('collection', [collectionKey]); + + var collection = Zotero.Collections.getByLibraryAndKey(libraryID, collectionKey); + + assert.sameMembers( + await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID), + [item.key] + ); + assert.isFalse( + await Zotero.Sync.Data.Local.getDateDeleted('item', libraryID, item.key) + ); + }); }); diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index dbc0e4d403..7366d69f15 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -494,6 +494,38 @@ describe("Zotero.Sync.Data.Local", function() { assert.isFalse(obj.synced); }); + it("should restore locally deleted collections and searches that changed remotely", async function () { + var libraryID = Zotero.Libraries.userLibraryID; + + for (let type of ['collection', 'search']) { + let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type); + let obj = await createDataObject(type, { version: 1 }); + let data = obj.toJSON(); + + await obj.eraseTx(); + + data.key = obj.key; + data.version = 2; + let json = { + key: obj.key, + version: 2, + data + }; + let results = await Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, [json], { stopOnError: true } + ); + assert.isTrue(results[0].processed); + assert.notOk(results[0].conflict); + assert.isTrue(results[0].restored); + assert.isUndefined(results[0].changes); + assert.isUndefined(results[0].conflicts); + obj = objectsClass.getByLibraryAndKey(libraryID, data.key); + assert.equal(obj.version, 2); + assert.isTrue(obj.synced); + assert.isFalse(await Zotero.Sync.Data.Local.getDateDeleted(type, libraryID, data.key)); + } + }); + it("should delete older versions in sync cache after processing", function* () { var libraryID = Zotero.Libraries.userLibraryID;