From 99eb39e288004f6b86eb7fd4e6fc10cc4f6feda0 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 20 May 2016 23:32:45 -0400 Subject: [PATCH] Always include 'contentType'/'charset'/'filename' in attachment JSON And omit in ZFS file sync requests The API previously didn't allow these properties to be set for group items, because they were set atomically during the file upload process, but 1) that's not really necessary (makes a little sense for 'filename', but not really a big deal if an old file is renamed on another computer before the new file is synced down) and 2) skipping them results in the properties getting erased after items are uploaded and the empty values returned by the server overwrite the local values. --- chrome/content/zotero/xpcom/data/item.js | 13 ++++--------- chrome/content/zotero/xpcom/storage/zfs.js | 16 ---------------- chrome/content/zotero/xpcom/sync/syncEngine.js | 8 ++++---- test/tests/itemTest.js | 16 ++++++++++++++++ test/tests/syncEngineTest.js | 10 +++++----- test/tests/zfsTest.js | 3 --- 6 files changed, 29 insertions(+), 37 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 70742f02cf..26ed1de4ac 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -4114,22 +4114,17 @@ Zotero.Item.prototype.toJSON = function (options = {}) { let linkMode = this.attachmentLinkMode; obj.linkMode = Zotero.Attachments.linkModeToName(linkMode); - let imported = this.isImportedAttachment(); - let skipFileProps = options.skipImportedFileProperties; - - if (!imported || !skipFileProps) { - obj.contentType = this.attachmentContentType; - obj.charset = this.attachmentCharset; - } + obj.contentType = this.attachmentContentType; + obj.charset = this.attachmentCharset; if (linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE) { obj.path = this.attachmentPath; } - else if (linkMode != Zotero.Attachments.LINK_MODE_LINKED_URL && !skipFileProps) { + else if (linkMode != Zotero.Attachments.LINK_MODE_LINKED_URL) { obj.filename = this.attachmentFilename; } - if (imported && !skipFileProps) { + if (this.isImportedAttachment() && !options.skipStorageProperties) { if (options.syncedStorageProperties) { obj.mtime = this.attachmentSyncedModificationTime; obj.md5 = this.attachmentSyncedHash; diff --git a/chrome/content/zotero/xpcom/storage/zfs.js b/chrome/content/zotero/xpcom/storage/zfs.js index 5d7828463b..a564530a5f 100644 --- a/chrome/content/zotero/xpcom/storage/zfs.js +++ b/chrome/content/zotero/xpcom/storage/zfs.js @@ -335,14 +335,6 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = { filename, size: file.fileSize }; - var charset = item.attachmentCharset; - var contentType = item.attachmentContentType; - if (charset) { - json.charset = charset; - } - if (contentType) { - json.contentType = contentType; - } if (zip) { json.zip = true; } @@ -372,14 +364,6 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = { filename, filesize: (yield OS.File.stat(uploadPath)).size }; - var charset = item.attachmentCharset; - var contentType = item.attachmentContentType; - if (charset) { - params.charset = charset; - } - if (contentType) { - params.contentType = contentType; - } if (zip) { params.zipMD5 = yield Zotero.Utilities.Internal.md5Async(uploadPath); params.zipFilename = OS.Path.basename(uploadPath); diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 88a5c335b3..cccf1e626a 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -878,8 +878,8 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func objectType, o.id, { - // Only include file properties ('contentType', etc.) for WebDAV files - skipImportedFileProperties: + // Only storage properties ('mtime', 'md5') for WebDAV files + skipStorageProperties: objectType == 'item' ? Zotero.Sync.Storage.Local.getModeForLibrary(this.library.libraryID) != 'webdav' @@ -1073,8 +1073,8 @@ Zotero.Sync.Data.Engine.prototype._getJSONForObject = function (objectType, id, includeKey: true, includeVersion: true, // DEBUG: remove? includeDate: true, - // Whether to skip 'contentType', 'filename', 'mtime', and 'md5' - skipImportedFileProperties: options.skipImportedFileProperties, + // Whether to skip 'mtime' and 'md5' + skipStorageProperties: options.skipStorageProperties, // Use last-synced mtime/md5 instead of current values from the file itself syncedStorageProperties: true, patchBase: cacheObj ? cacheObj.data : false diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index 8e5729bbd4..03cfe1f7ce 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1051,6 +1051,22 @@ describe("Zotero.Item", function () { assert.equal(json.md5, (yield item.attachmentHash)); }) + it("should omit storage values with .skipStorageProperties", function* () { + var file = getTestDataDirectory(); + file.append('test.png'); + var item = yield Zotero.Attachments.importFromFile({ file }); + + item.attachmentSyncedModificationTime = new Date().getTime(); + item.attachmentSyncedHash = 'b32e33f529942d73bea4ed112310f804'; + yield item.saveTx({ skipAll: true }); + + var json = item.toJSON({ + skipStorageProperties: true + }); + assert.isUndefined(json.mtime); + assert.isUndefined(json.md5); + }); + it("should output synced storage values with .syncedStorageProperties", function* () { var item = new Zotero.Item('attachment'); item.attachmentLinkMode = 'imported_file'; diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 1cfb98bbb5..db6abfb91a 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -787,7 +787,7 @@ describe("Zotero.Sync.Data.Engine", function () { }); - it("shouldn't include file properties for attachments in ZFS libraries", function* () { + it("shouldn't include storage properties for attachments in ZFS libraries", function* () { ({ engine, client, caller } = yield setup()); var libraryID = Zotero.Libraries.userLibraryID; @@ -812,9 +812,9 @@ describe("Zotero.Sync.Data.Engine", function () { let itemJSON = json[0]; assert.equal(itemJSON.key, item.key); assert.equal(itemJSON.version, 0); - assert.notProperty(itemJSON, "contentType"); - assert.notProperty(itemJSON, "charset"); - assert.notProperty(itemJSON, "filename"); + assert.property(itemJSON, "contentType"); + assert.property(itemJSON, "charset"); + assert.property(itemJSON, "filename"); assert.notProperty(itemJSON, "mtime"); assert.notProperty(itemJSON, "md5"); req.respond( @@ -840,7 +840,7 @@ describe("Zotero.Sync.Data.Engine", function () { }); - it("should include file properties for attachments in WebDAV libraries", function* () { + it("should include storage properties for attachments in WebDAV libraries", function* () { ({ engine, client, caller } = yield setup()); var libraryID = Zotero.Libraries.userLibraryID; diff --git a/test/tests/zfsTest.js b/test/tests/zfsTest.js index 1515be0075..14a7a5ce76 100644 --- a/test/tests/zfsTest.js +++ b/test/tests/zfsTest.js @@ -323,7 +323,6 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { assert.equal(params.mtime, mtime1); assert.equal(params.filename, filename1); assert.equal(params.filesize, size1); - assert.equal(params.contentType, contentType1); req.respond( 200, @@ -375,8 +374,6 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { assert.equal(params.filename, filename2); assert.equal(params.zipFilename, item2.key + ".zip"); assert.isTrue(parseInt(params.filesize) == params.filesize); - assert.equal(params.contentType, contentType2); - assert.equal(params.charset, charset2); req.respond( 200,