From 79baac3158b8f45b138370ae3aa0fcdceb558e3a Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sat, 22 Oct 2016 15:02:15 -0400 Subject: [PATCH] Fix relinking of imported attachment with external file --- chrome/content/zotero/xpcom/attachments.js | 1 + chrome/content/zotero/xpcom/data/item.js | 71 +++++++++++-------- .../zotero/xpcom/storage/storageLocal.js | 17 +++-- chrome/content/zotero/zoteroPane.js | 13 ++-- test/tests/itemTest.js | 24 +++++++ 5 files changed, 86 insertions(+), 40 deletions(-) diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js index 2720261a8..de425e27d 100644 --- a/chrome/content/zotero/xpcom/attachments.js +++ b/chrome/content/zotero/xpcom/attachments.js @@ -797,6 +797,7 @@ Zotero.Attachments = new function(){ var dir = this.getStorageDirectory(item); yield _moveOrphanedDirectory(dir); if (!dir.exists()) { + Zotero.debug("Creating directory " + dir.path); dir.create(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0755); } return dir; diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 311678138..4dd8a8ce1 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -2513,43 +2513,53 @@ Zotero.Item.prototype.relinkAttachmentFile = Zotero.Promise.coroutine(function* if (fileName.endsWith(".lnk")) { throw new Error("Cannot relink to Windows shortcut"); } + var newPath; var newName = Zotero.File.getValidFileName(fileName); if (!newName) { throw new Error("No valid characters in filename after filtering"); } - var newPath = OS.Path.join(OS.Path.dirname(path), newName); - - try { - // If selected file isn't in the attachment's storage directory, - // copy it in and use that one instead - var storageDir = Zotero.Attachments.getStorageDirectory(this).path; - if (this.isImportedAttachment() && OS.Path.dirname(path) != storageDir) { - // If file with same name already exists in the storage directory, - // move it out of the way - let deleteBackup = false;; - if (yield OS.File.exists(newPath)) { - deleteBackup = true; - yield OS.File.move(newPath, newPath + ".bak"); - } - - let newFile = Zotero.File.copyToUnique(path, newPath); - newPath = newFile.path; - - // Delete backup file - if (deleteBackup) { - yield OS.File.remove(newPath + ".bak"); - } + // If selected file isn't in the attachment's storage directory, + // copy it in and use that one instead + var storageDir = Zotero.Attachments.getStorageDirectory(this).path; + if (this.isImportedAttachment() && OS.Path.dirname(path) != storageDir) { + newPath = OS.Path.join(storageDir, newName); + + // If file with same name already exists in the storage directory, + // move it out of the way + let backupCreated = false; + if (yield OS.File.exists(newPath)) { + backupCreated = true; + yield OS.File.move(newPath, newPath + ".bak"); } - // Rename file to filtered name if necessary - else if (fileName != newName) { - Zotero.debug("Renaming file '" + fileName + "' to '" + newName + "'"); - OS.File.move(path, newPath, { noOverwrite: true }); + // Create storage directory if necessary + else if (!(yield OS.File.exists(storageDir))) { + Zotero.Attachments.createDirectoryForItem(this); + } + + let newFile; + try { + newFile = Zotero.File.copyToUnique(path, newPath); + } + catch (e) { + // Restore backup file if copying failed + if (backupCreated) { + yield OS.File.move(newPath + ".bak", newPath); + } + throw e; + } + newPath = newFile.path; + + // Delete backup file + if (backupCreated) { + yield OS.File.remove(newPath + ".bak"); } } - catch (e) { - Zotero.logError(e); - return false; + // Rename file to filtered name if necessary + else if (fileName != newName) { + newPath = OS.Path.join(OS.Path.dirname(path), newName); + Zotero.debug("Renaming file '" + fileName + "' to '" + newName + "'"); + OS.File.move(path, newPath, { noOverwrite: true }); } this.attachmentPath = newPath; @@ -2560,6 +2570,9 @@ Zotero.Item.prototype.relinkAttachmentFile = Zotero.Promise.coroutine(function* skipEditCheck: skipItemUpdate }); + this._updateAttachmentStates(true); + yield Zotero.Notifier.trigger('refresh', 'item', this.id); + return true; }); diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js index e2c83d303..992dc1d12 100644 --- a/chrome/content/zotero/xpcom/storage/storageLocal.js +++ b/chrome/content/zotero/xpcom/storage/storageLocal.js @@ -566,13 +566,18 @@ Zotero.Sync.Storage.Local = { // If library isn't editable but filename was changed, update // database without updating the item's mod time, which would result // in a library access error - if (!Zotero.Items.isEditable(item)) { - Zotero.debug("File renamed without library access -- " - + "updating itemAttachments path", 3); - yield item.relinkAttachmentFile(newPath, true); + try { + if (!Zotero.Items.isEditable(item)) { + Zotero.debug("File renamed without library access -- " + + "updating itemAttachments path", 3); + yield item.relinkAttachmentFile(newPath, true); + } + else { + yield item.relinkAttachmentFile(newPath); + } } - else { - yield item.relinkAttachmentFile(newPath); + catch (e) { + Zotero.File.checkFileAccessError(e, path, 'update'); } path = newPath; diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 518262b80..3677360cc 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -4512,9 +4512,9 @@ var ZoteroPane = new function() return; } - var item = yield Zotero.Items.getAsync(itemID); + var item = Zotero.Items.get(itemID); if (!item) { - throw('Item ' + itemID + ' not found in ZoteroPane_Local.relinkAttachment()'); + throw new Error('Item ' + itemID + ' not found in ZoteroPane_Local.relinkAttachment()'); } while (true) { @@ -4523,8 +4523,11 @@ var ZoteroPane = new function() .createInstance(nsIFilePicker); fp.init(window, Zotero.getString('pane.item.attachments.select'), nsIFilePicker.modeOpen); - - var file = item.getFile(false, true); + var file = item.getFilePath(); + if (!file) { + Zotero.debug("Invalid path", 2); + break; + } var dir = Zotero.File.getClosestDirectory(file); if (dir) { dir.QueryInterface(Components.interfaces.nsILocalFile); @@ -4549,7 +4552,7 @@ var ZoteroPane = new function() continue; } - item.relinkAttachmentFile(file); + yield item.relinkAttachmentFile(file.path); break; } diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index c6cbf5ac3..97f913d70 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -799,6 +799,30 @@ describe("Zotero.Item", function () { }) + describe("#relinkAttachmentFile", function () { + it("should copy a file elsewhere into the storage directory", function* () { + var filename = 'test.png'; + var file = getTestDataDirectory(); + file.append(filename); + var tmpDir = yield getTempDirectory(); + var tmpFile = OS.Path.join(tmpDir, filename); + yield OS.File.copy(file.path, tmpFile); + file = OS.Path.join(tmpDir, filename); + + var item = yield Zotero.Attachments.importFromFile({ file }); + let path = yield item.getFilePathAsync(); + yield OS.File.remove(path); + yield OS.File.removeEmptyDir(OS.Path.dirname(path)); + + assert.isFalse(yield item.fileExists()); + yield item.relinkAttachmentFile(file); + assert.isTrue(yield item.fileExists()); + + assert.isTrue(yield OS.File.exists(tmpFile)); + }); + }); + + describe("#setTags", function () { it("should save an array of tags in API JSON format", function* () { var tags = [