From c5fa1303e39e720c20a901d8263a148d518c9a85 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 26 Jan 2018 03:36:30 -0500 Subject: [PATCH] Prompt to reset local group files on 403 for file attachment upload And reset modified file attachments when resetting files --- .../content/zotero/xpcom/sync/syncEngine.js | 15 +++ chrome/content/zotero/xpcom/sync/syncLocal.js | 96 +++++++++++-------- test/tests/syncEngineTest.js | 61 ++++++++++++ test/tests/syncLocalTest.js | 71 +++++++++++++- 4 files changed, 199 insertions(+), 44 deletions(-) diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index ced55ca7fa..4d4b6a9ef7 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -1983,6 +1983,21 @@ Zotero.Sync.Data.Engine.prototype._checkObjectUploadError = Zotero.Promise.corou } } } + else if (code == 403) { + // Prompt to reset local group files on 403 for file attachment upload + if (objectType == 'item') { + let item = Zotero.Items.getByLibraryAndKey(this.libraryID, key); + if (this.library.libraryType == 'group' && item.isFileAttachment()) { + let index = Zotero.Sync.Storage.Utilities.showFileWriteAccessLostPrompt( + null, this.library + ); + if (index === 0) { + yield Zotero.Sync.Data.Local.resetUnsyncedLibraryFiles(this.libraryID); + } + return false; + } + } + } // This shouldn't happen, because the upload request includes a library version and should // prevent an outdated upload before the object version is checked. If it does, we need to // do a full sync. This error is checked in handleUploadError(). diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 21763a9152..14043fd486 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -239,6 +239,8 @@ Zotero.Sync.Data.Local = { _libraryHasUnsyncedFiles: Zotero.Promise.coroutine(function* (libraryID) { + // TODO: Check for modified file attachment items, which also can't be uploaded + // (and which are corrected by resetUnsyncedLibraryFiles()) yield Zotero.Sync.Storage.Local.checkForUpdatedFiles(libraryID); return !!(yield Zotero.Sync.Storage.Local.getFilesToUpload(libraryID)).length; }), @@ -253,42 +255,9 @@ Zotero.Sync.Data.Local = { } for (let objectType of Zotero.DataObjectUtilities.getTypesForLibrary(libraryID)) { - let objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); - let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); - // New/modified objects - let ids = yield Zotero.Sync.Data.Local.getUnsynced(objectType, libraryID); - let keys = ids.map(id => objectsClass.getLibraryAndKeyFromID(id).key); - let cacheVersions = yield this.getLatestCacheObjectVersions(objectType, libraryID, keys); - let toDelete = []; - for (let key of keys) { - let obj = objectsClass.getByLibraryAndKey(libraryID, key); - - // If object is in cache, overwrite with pristine data - if (cacheVersions[key]) { - let json = yield this.getCacheObject(objectType, libraryID, key, cacheVersions[key]); - yield Zotero.DB.executeTransaction(function* () { - yield this._saveObjectFromJSON(obj, json, {}); - }.bind(this)); - } - // Otherwise, erase - else { - toDelete.push(objectsClass.getIDFromLibraryAndKey(libraryID, key)); - } - } - if (toDelete.length) { - yield objectsClass.erase( - toDelete, - { - skipEditCheck: true, - skipDeleteLog: true - } - ); - } - - // Deleted objects - keys = yield Zotero.Sync.Data.Local.getDeleted(objectType, libraryID); - yield this.removeObjectsFromDeleteLog(objectType, libraryID, keys); + let ids = yield this.getUnsynced(objectType, libraryID); + yield this._resetObjects(libraryID, objectType, ids); } // Mark library for full sync @@ -305,13 +274,62 @@ Zotero.Sync.Data.Local = { * * _libraryHasUnsyncedFiles(), which checks for updated files, must be called first. */ - resetUnsyncedLibraryFiles: Zotero.Promise.coroutine(function* (libraryID) { - var itemIDs = yield Zotero.Sync.Storage.Local.getFilesToUpload(libraryID); + resetUnsyncedLibraryFiles: async function (libraryID) { + // Reset unsynced file attachments + var itemIDs = await Zotero.Sync.Data.Local.getUnsynced('item', libraryID); + var toReset = []; for (let itemID of itemIDs) { let item = Zotero.Items.get(itemID); - yield item.deleteAttachmentFile(); + if (item.isFileAttachment()) { + toReset.push(item.id); + } } - }), + await this._resetObjects(libraryID, 'item', toReset); + + // Delete unsynced files + var itemIDs = await Zotero.Sync.Storage.Local.getFilesToUpload(libraryID); + for (let itemID of itemIDs) { + let item = Zotero.Items.get(itemID); + await item.deleteAttachmentFile(); + } + }, + + + _resetObjects: async function (libraryID, objectType, ids) { + var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); + + var keys = ids.map(id => objectsClass.getLibraryAndKeyFromID(id).key); + var cacheVersions = await this.getLatestCacheObjectVersions(objectType, libraryID, keys); + var toDelete = []; + for (let key of keys) { + let obj = objectsClass.getByLibraryAndKey(libraryID, key); + + // If object is in cache, overwrite with pristine data + if (cacheVersions[key]) { + let json = await this.getCacheObject(objectType, libraryID, key, cacheVersions[key]); + await Zotero.DB.executeTransaction(async function () { + await this._saveObjectFromJSON(obj, json, {}); + }.bind(this)); + } + // Otherwise, erase + else { + toDelete.push(objectsClass.getIDFromLibraryAndKey(libraryID, key)); + } + } + if (toDelete.length) { + await objectsClass.erase( + toDelete, + { + skipEditCheck: true, + skipDeleteLog: true + } + ); + } + + // Deleted objects + keys = await Zotero.Sync.Data.Local.getDeleted(objectType, libraryID); + await this.removeObjectsFromDeleteLog(objectType, libraryID, keys); + }, getSkippedLibraries: function () { diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index ff5df058c3..dff8bf9b05 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -3024,6 +3024,67 @@ describe("Zotero.Sync.Data.Engine", function () { assert.equal(called, 1); assert.equal(spy.callCount, 2); }); + + + it("should show file-write-access-lost dialog on 403 for attachment upload in group", async function () { + var group = await createGroup(); + var libraryID = group.libraryID; + var libraryVersion = 5; + group.libraryVersion = libraryVersion; + await group.saveTx(); + + ({ engine, client, caller } = await setup({ + libraryID, + stopOnError: false + })); + + var item1 = await createDataObject('item', { libraryID }); + var item2 = await importFileAttachment( + 'test.png', + { + libraryID, + parentID: item1.id, + version: 5 + } + ); + + var called = 0; + server.respond(function (req) { + let requestJSON = JSON.parse(req.requestBody); + if (called == 0) { + req.respond( + 200, + { + "Last-Modified-Version": ++libraryVersion + }, + JSON.stringify({ + successful: { + 0: item1.toResponseJSON({ version: libraryVersion }) + }, + unchanged: {}, + failed: { + 1: { + code: 403, + message: "File editing access denied" + } + } + }) + ); + } + called++; + }); + + var promise = waitForDialog(); + var spy = sinon.spy(engine, "onError"); + var result = await engine._startUpload(); + assert.isTrue(promise.isResolved()); + assert.equal(result, engine.UPLOAD_RESULT_SUCCESS); + assert.equal(called, 1); + assert.equal(spy.callCount, 1); + + assert.ok(Zotero.Items.get(item1.id)); + assert.isFalse(Zotero.Items.get(item2.id)); + }); }); diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 9b835cb0e0..030ee9d90f 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -313,27 +313,88 @@ describe("Zotero.Sync.Data.Local", function() { }); var libraryID = group.libraryID; + // File attachment that's totally in sync -- leave alone var attachment1 = yield importFileAttachment('test.png', { libraryID }); attachment1.attachmentSyncState = "in_sync"; - attachment1.attachmentSyncedModificationTime = 1234567890000; - attachment1.attachmentSyncedHash = "8caf2ee22919d6725eb0648b98ef6bad"; - var attachment2 = yield importFileAttachment('test.pdf', { libraryID }); + attachment1.attachmentSyncedModificationTime = yield attachment1.attachmentModificationTime; + attachment1.attachmentSyncedHash = yield attachment1.attachmentHash; + attachment1.synced = true; + yield attachment1.saveTx({ + skipSyncedUpdate: true + }); + + // File attachment that's in sync with changed file -- delete file and mark for download + var attachment2 = yield importFileAttachment('test.png', { libraryID }); + attachment2.synced = true; + yield attachment2.saveTx({ + skipSyncedUpdate: true + }); + + // File attachment that's unsynced -- delete item and file + var attachment3 = yield importFileAttachment('test.pdf', { libraryID }); // Has to be called before resetUnsyncedLibraryFiles() assert.isTrue(yield Zotero.Sync.Data.Local._libraryHasUnsyncedFiles(libraryID)); yield Zotero.Sync.Data.Local.resetUnsyncedLibraryFiles(libraryID); - assert.isFalse(yield attachment1.fileExists()); + assert.isTrue(yield attachment1.fileExists()); assert.isFalse(yield attachment2.fileExists()); + assert.isFalse(yield attachment3.fileExists()); assert.equal( - attachment1.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD + attachment1.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_IN_SYNC ); assert.equal( attachment2.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD ); + assert.isFalse(Zotero.Items.get(attachment3.id)); }); }); + + it("should revert modified file attachment item", async function () { + var group = await createGroup({ + version: 1, + libraryVersion: 2 + }); + var libraryID = group.libraryID; + + // File attachment that's changed but file is in sync -- reset item, keep file + var attachment = await importFileAttachment('test.png', { libraryID }); + var originalTitle = attachment.getField('title'); + attachment.attachmentSyncedModificationTime = await attachment.attachmentModificationTime; + attachment.attachmentSyncedHash = await attachment.attachmentHash; + attachment.attachmentSyncState = "in_sync"; + attachment.synced = true; + attachment.version = 2; + await attachment.saveTx({ + skipSyncedUpdate: true + }); + // Save original in cache + await Zotero.Sync.Data.Local.saveCacheObject( + 'item', + libraryID, + Object.assign( + attachment.toJSON(), + // TEMP: md5 and mtime aren't currently included in JSON, and without it the + // file gets marked for download when the item gets reset from the cache + { + md5: attachment.attachmentHash, + mtime: attachment.attachmentSyncedModificationTime + } + ) + ); + // Modify title + attachment.setField('title', "New Title"); + await attachment.saveTx(); + + await Zotero.Sync.Data.Local.resetUnsyncedLibraryFiles(libraryID); + + assert.isTrue(await attachment.fileExists()); + assert.equal(attachment.getField('title'), originalTitle); + assert.equal( + attachment.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_IN_SYNC + ); + }); });