diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js index c9a2a03a46..3206640195 100644 --- a/chrome/content/zotero/xpcom/attachments.js +++ b/chrome/content/zotero/xpcom/attachments.js @@ -61,62 +61,61 @@ Zotero.Attachments = new function(){ } var attachmentItem, itemID, newFile, contentType; - return Zotero.DB.executeTransaction(function* () { - // Create a new attachment - attachmentItem = new Zotero.Item('attachment'); - if (parentItemID) { - let {libraryID: parentLibraryID, key: parentKey} = - Zotero.Items.getLibraryAndKeyFromID(parentItemID); - attachmentItem.libraryID = parentLibraryID; - } - else if (libraryID) { - attachmentItem.libraryID = libraryID; - } - attachmentItem.setField('title', newName); - attachmentItem.parentID = parentItemID; - attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE; - if (collections) { - attachmentItem.setCollections(collections); - } - yield attachmentItem.save(saveOptions); - - // Create directory for attachment files within storage directory - destDir = yield this.createDirectoryForItem(attachmentItem); - - // Point to copied file - newFile = destDir.clone(); - newFile.append(newName); - - // Copy file to unique filename, which automatically shortens long filenames - newFile = Zotero.File.copyToUnique(file, newFile); - - contentType = yield Zotero.MIME.getMIMETypeFromFile(newFile); - - attachmentItem.attachmentContentType = contentType; - attachmentItem.attachmentPath = newFile.path; - yield attachmentItem.save(saveOptions); - }.bind(this)) - .then(function () { - return _postProcessFile(attachmentItem, newFile, contentType); - }) - .catch(function (e) { + try { + yield Zotero.DB.executeTransaction(function* () { + // Create a new attachment + attachmentItem = new Zotero.Item('attachment'); + if (parentItemID) { + let {libraryID: parentLibraryID, key: parentKey} = + Zotero.Items.getLibraryAndKeyFromID(parentItemID); + attachmentItem.libraryID = parentLibraryID; + } + else if (libraryID) { + attachmentItem.libraryID = libraryID; + } + attachmentItem.setField('title', newName); + attachmentItem.parentID = parentItemID; + attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE; + if (collections) { + attachmentItem.setCollections(collections); + } + yield attachmentItem.save(saveOptions); + + // Create directory for attachment files within storage directory + destDir = yield this.createDirectoryForItem(attachmentItem); + + // Point to copied file + newFile = OS.Path.join(destDir, newName); + + // Copy file to unique filename, which automatically shortens long filenames + newFile = Zotero.File.copyToUnique(file, newFile); + + contentType = yield Zotero.MIME.getMIMETypeFromFile(newFile); + + attachmentItem.attachmentContentType = contentType; + attachmentItem.attachmentPath = newFile.path; + yield attachmentItem.save(saveOptions); + }.bind(this)); + yield _postProcessFile(attachmentItem, newFile, contentType); + } + catch (e) { Zotero.logError(e); - var msg = "Failed importing file " + file.path; - Zotero.logError(msg); + Zotero.logError("Failed importing file " + file.path); // Clean up try { - if (destDir && destDir.exists()) { - destDir.remove(true); + if (destDir && (yield OS.File.exists(destDir))) { + yield OS.File.removeDir(destDir); } } catch (e) { Zotero.logError(e); } - }.bind(this)) - .then(function () { - return attachmentItem; - }); + + throw e; + } + + return attachmentItem; }); @@ -174,39 +173,39 @@ Zotero.Attachments = new function(){ } var attachmentItem, itemID, destDir, newFile; - return Zotero.DB.executeTransaction(function* () { - // Create a new attachment - attachmentItem = new Zotero.Item('attachment'); - let {libraryID, key: parentKey} = Zotero.Items.getLibraryAndKeyFromID(parentItemID); - attachmentItem.libraryID = libraryID; - attachmentItem.setField('title', title); - attachmentItem.setField('url', url); - attachmentItem.parentID = parentItemID; - attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL; - attachmentItem.attachmentContentType = contentType; - attachmentItem.attachmentCharset = charset; - - // DEBUG: this should probably insert access date too so as to - // create a proper item, but at the moment this is only called by - // translate.js, which sets the metadata fields itself - itemID = yield attachmentItem.save(); - - var storageDir = Zotero.getStorageDirectory(); - destDir = this.getStorageDirectory(attachmentItem); - yield _moveOrphanedDirectory(destDir); - file.parent.copyTo(storageDir, destDir.leafName); - - // Point to copied file - newFile = destDir.clone(); - newFile.append(file.leafName); - - attachmentItem.attachmentPath = newFile.path; - yield attachmentItem.save(); - }.bind(this)) - .then(function () { - return _postProcessFile(attachmentItem, newFile, contentType, charset); - }) - .catch(function (e) { + try { + yield Zotero.DB.executeTransaction(function* () { + // Create a new attachment + attachmentItem = new Zotero.Item('attachment'); + let {libraryID, key: parentKey} = Zotero.Items.getLibraryAndKeyFromID(parentItemID); + attachmentItem.libraryID = libraryID; + attachmentItem.setField('title', title); + attachmentItem.setField('url', url); + attachmentItem.parentID = parentItemID; + attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL; + attachmentItem.attachmentContentType = contentType; + attachmentItem.attachmentCharset = charset; + + // DEBUG: this should probably insert access date too so as to + // create a proper item, but at the moment this is only called by + // translate.js, which sets the metadata fields itself + itemID = yield attachmentItem.save(); + + var storageDir = Zotero.getStorageDirectory(); + destDir = this.getStorageDirectory(attachmentItem); + yield OS.File.removeDir(destDir.path); + file.parent.copyTo(storageDir, destDir.leafName); + + // Point to copied file + newFile = destDir.clone(); + newFile.append(file.leafName); + + attachmentItem.attachmentPath = newFile.path; + yield attachmentItem.save(); + }.bind(this)); + yield _postProcessFile(attachmentItem, newFile, contentType, charset); + } + catch (e) { Zotero.logError(e); // Clean up @@ -218,10 +217,10 @@ Zotero.Attachments = new function(){ catch (e) { Zotero.logError(e, 1); } - }.bind(this)) - .then(function () { - return attachmentItem; - }); + + throw e; + } + return attachmentItem; }); @@ -787,21 +786,18 @@ Zotero.Attachments = new function(){ /** * Create directory for attachment files within storage directory * - * If a directory exists with the same name, move it to orphaned-files + * If a directory exists, delete and recreate * * @param {Number} itemID - Item id - * @return {Promise} + * @return {Promise} - Path of new directory */ this.createDirectoryForItem = Zotero.Promise.coroutine(function* (item) { if (!(item instanceof Zotero.Item)) { throw new Error("'item' must be a Zotero.Item"); } - 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); - } + var dir = this.getStorageDirectory(item).path; + yield OS.File.removeDir(dir, { ignoreAbsent: true }); + yield Zotero.File.createDirectoryIfMissingAsync(dir); return dir; }); @@ -1160,55 +1156,6 @@ Zotero.Attachments = new function(){ } - /** - * If directory exists and is non-empty, move it to orphaned-files directory - * - * If empty, just remove it - */ - var _moveOrphanedDirectory = Zotero.Promise.coroutine(function* (dir) { - if (!dir.exists()) { - return; - } - - dir = dir.clone(); - - // If directory is empty or has only hidden files, delete it - var files = dir.directoryEntries; - files.QueryInterface(Components.interfaces.nsIDirectoryEnumerator); - var empty = true; - while (files.hasMoreElements()) { - var file = files.getNext(); - file.QueryInterface(Components.interfaces.nsIFile); - if (file.leafName[0] == '.') { - continue; - } - empty = false; - break; - } - files.close(); - if (empty) { - dir.remove(true); - return; - } - - // Create orphaned-files directory if it doesn't exist - var orphaned = OS.Path.join(Zotero.DataDirectory.dir, 'orphaned-files'); - yield Zotero.File.createDirectoryIfMissingAsync(orphaned); - - // Find unique filename for orphaned file - var orphanTarget = OS.Path.join(orphaned, dir.leafName); - var newName = null; - if (yield OS.File.exists(orphanTarget)) { - let newFile = yield OS.File.openUnique(orphanTarget, { humanReadable: true }) - newName = OS.Path.basename(newFile.path); - newFile.file.close(); - } - - // Move target to orphaned files directory - dir.moveTo(Zotero.File.pathToFile(orphaned), newName); - }); - - /** * Create a new item of type 'attachment' and add to the itemAttachments table * diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 8965469544..33ac2074bf 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -2536,7 +2536,7 @@ Zotero.Item.prototype.relinkAttachmentFile = Zotero.Promise.coroutine(function* } // Create storage directory if necessary else if (!(yield OS.File.exists(storageDir))) { - Zotero.Attachments.createDirectoryForItem(this); + yield Zotero.Attachments.createDirectoryForItem(this); } let newFile; diff --git a/chrome/content/zotero/xpcom/fulltext.js b/chrome/content/zotero/xpcom/fulltext.js index f3441be448..897bf0e588 100644 --- a/chrome/content/zotero/xpcom/fulltext.js +++ b/chrome/content/zotero/xpcom/fulltext.js @@ -659,7 +659,15 @@ Zotero.Fulltext = Zotero.FullText = new function(){ // If file is stored outside of Zotero, create a directory for the item // in the storage directory and save the cache file there if (linkMode == Zotero.Attachments.LINK_MODE_LINKED_FILE) { - var parentDirPath = (yield Zotero.Attachments.createDirectoryForItem(item)).path; + let path = item.getFilePath(); + if (!path) { + Zotero.debug("Invalid path for item " + itemID); + return false; + } + var parentDirPath = OS.Path.dirname(path); + if (!(yield OS.File.exists(parentDirPath))) { + yield Zotero.Attachments.createDirectoryForItem(item); + } } else { var parentDirPath = OS.Path.dirname(filePath); diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js index a8ad04739e..48b6d2032f 100644 --- a/chrome/content/zotero/xpcom/storage/storageLocal.js +++ b/chrome/content/zotero/xpcom/storage/storageLocal.js @@ -626,12 +626,7 @@ Zotero.Sync.Storage.Local = { throw new Error("Downloaded file not found"); } - var parentDirPath = Zotero.Attachments.getStorageDirectory(item).path; - if (!(yield OS.File.exists(parentDirPath))) { - yield Zotero.Attachments.createDirectoryForItem(item); - } - - yield this._deleteExistingAttachmentFiles(item); + yield Zotero.Attachments.createDirectoryForItem(item); var path = item.getFilePath(); if (!path) { @@ -741,16 +736,12 @@ Zotero.Sync.Storage.Local = { } var parentDir = Zotero.Attachments.getStorageDirectory(item).path; - if (!(yield OS.File.exists(parentDir))) { - yield Zotero.Attachments.createDirectoryForItem(item); - } - try { - yield this._deleteExistingAttachmentFiles(item); + yield Zotero.Attachments.createDirectoryForItem(item); } catch (e) { zipReader.close(); - throw (e); + throw e; } var returnFile = null; @@ -906,17 +897,6 @@ Zotero.Sync.Storage.Local = { }), - _deleteExistingAttachmentFiles: Zotero.Promise.method(function (item) { - var parentDir = Zotero.Attachments.getStorageDirectory(item).path; - // OS.File.DirectoryIterator, used by OS.File.removeDir(), isn't reliable on Travis, - // returning entry.isDir == false for subdirectories, so use nsIFile instead - if (Zotero.automatedTest) { - Zotero.File.pathToFile(parentDir).remove(true); - } - return OS.File.removeDir(parentDir); - }), - - /** * @return {Promise} - A promise for an array of conflict objects */ diff --git a/test/tests/attachmentsTest.js b/test/tests/attachmentsTest.js index 50f11114b8..ce2a9fffbb 100644 --- a/test/tests/attachmentsTest.js +++ b/test/tests/attachmentsTest.js @@ -274,5 +274,36 @@ describe("Zotero.Attachments", function() { assert.isTrue(yield Zotero.Attachments.hasMultipleFiles(item)); assert.equal((yield Zotero.Attachments.getNumFiles(item)), 2); }) - }) + }); + + describe("#createDirectoryForItem()", function () { + it("should create missing directory", function* () { + var item = yield importFileAttachment('test.png'); + var path = OS.Path.dirname(item.getFilePath()); + yield OS.File.removeDir(path); + yield Zotero.Attachments.createDirectoryForItem(item); + assert.isTrue(yield OS.File.exists(path)); + }); + + it("should delete all existing files", function* () { + var item = yield importFileAttachment('test.html'); + var path = OS.Path.dirname(item.getFilePath()); + var files = ['a', 'b', 'c', 'd']; + for (let file of files) { + yield Zotero.File.putContentsAsync(OS.Path.join(path, file), file); + } + yield Zotero.Attachments.createDirectoryForItem(item); + assert.isTrue(yield Zotero.File.directoryIsEmpty(path)); + assert.isTrue(yield OS.File.exists(path)); + }); + + it("should handle empty directory", function* () { + var item = yield importFileAttachment('test.png'); + var file = item.getFilePath(); + var dir = OS.Path.dirname(item.getFilePath()); + yield OS.File.remove(file); + yield Zotero.Attachments.createDirectoryForItem(item); + assert.isTrue(yield OS.File.exists(dir)); + }); + }); }) diff --git a/test/tests/storageLocalTest.js b/test/tests/storageLocalTest.js index 6472ea81f1..024e55e7f5 100644 --- a/test/tests/storageLocalTest.js +++ b/test/tests/storageLocalTest.js @@ -218,24 +218,6 @@ describe("Zotero.Sync.Storage.Local", function () { }) }) - describe("#_deleteExistingAttachmentFiles()", function () { - it("should delete all files", function* () { - var item = yield importFileAttachment('test.html'); - var path = OS.Path.dirname(item.getFilePath()); - var files = ['a', 'b', 'c', 'd']; - for (let file of files) { - yield Zotero.File.putContentsAsync(OS.Path.join(path, file), file); - } - yield Zotero.Sync.Storage.Local._deleteExistingAttachmentFiles(item); - for (let file of files) { - assert.isFalse( - (yield OS.File.exists(OS.Path.join(path, file))), - `File '${file}' doesn't exist` - ); - } - }) - }) - describe("#getConflicts()", function () { it("should return an array of objects for attachments in conflict", function* () { var libraryID = Zotero.Libraries.userLibraryID;