Fix replacement of existing item storage directories

- Make Zotero.Attachments.createDirectoryForItem() delete existing
  directory instead of moving it to orphaned-files; also now returns a
  string path instead of an nsIFile
- Use above function during file sync instead of
  _deleteExistingAttachmentFiles(), which was partly broken
- Fix throwing on errors when saving some attachment types

chrome/content/zotero/xpcom/attachments.js

@@ -61,62 +61,61 @@ Zotero.Attachments = new function(){
 		}
 		
 		var attachmentItem, itemID, newFile, contentType;
-		return Zotero.DB.executeTransaction(function* () {
+		try {
-			// Create a new attachment
+			yield Zotero.DB.executeTransaction(function* () {
-			attachmentItem = new Zotero.Item('attachment');
+				// Create a new attachment
-			if (parentItemID) {
+				attachmentItem = new Zotero.Item('attachment');
-				let {libraryID: parentLibraryID, key: parentKey} =
+				if (parentItemID) {
-					Zotero.Items.getLibraryAndKeyFromID(parentItemID);
+					let {libraryID: parentLibraryID, key: parentKey} =
-				attachmentItem.libraryID = parentLibraryID;
+						Zotero.Items.getLibraryAndKeyFromID(parentItemID);
-			}
+					attachmentItem.libraryID = parentLibraryID;
-			else if (libraryID) {
+				}
-				attachmentItem.libraryID = libraryID;
+				else if (libraryID) {
-			}
+					attachmentItem.libraryID = libraryID;
-			attachmentItem.setField('title', newName);
+				}
-			attachmentItem.parentID = parentItemID;
+				attachmentItem.setField('title', newName);
-			attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE;
+				attachmentItem.parentID = parentItemID;
-			if (collections) {
+				attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_FILE;
-				attachmentItem.setCollections(collections);
+				if (collections) {
-			}
+					attachmentItem.setCollections(collections);
-			yield attachmentItem.save(saveOptions);
+				}
-			
+				yield attachmentItem.save(saveOptions);
-			// Create directory for attachment files within storage directory
+				
-			destDir = yield this.createDirectoryForItem(attachmentItem);
+				// Create directory for attachment files within storage directory
-			
+				destDir = yield this.createDirectoryForItem(attachmentItem);
-			// Point to copied file
+				
-			newFile = destDir.clone();
+				// Point to copied file
-			newFile.append(newName);
+				newFile = OS.Path.join(destDir, newName);
-			
+				
-			// Copy file to unique filename, which automatically shortens long filenames
+				// Copy file to unique filename, which automatically shortens long filenames
-			newFile = Zotero.File.copyToUnique(file, newFile);
+				newFile = Zotero.File.copyToUnique(file, newFile);
-			
+				
-			contentType = yield Zotero.MIME.getMIMETypeFromFile(newFile);
+				contentType = yield Zotero.MIME.getMIMETypeFromFile(newFile);
-			
+				
-			attachmentItem.attachmentContentType = contentType;
+				attachmentItem.attachmentContentType = contentType;
-			attachmentItem.attachmentPath = newFile.path;
+				attachmentItem.attachmentPath = newFile.path;
-			yield attachmentItem.save(saveOptions);
+				yield attachmentItem.save(saveOptions);
-		}.bind(this))
+			}.bind(this));
-		.then(function () {
+			yield _postProcessFile(attachmentItem, newFile, contentType);
-			return _postProcessFile(attachmentItem, newFile, contentType);
+		}
-		})
+		catch (e) {
-		.catch(function (e) {
 			Zotero.logError(e);
-			var msg = "Failed importing file " + file.path;
+			Zotero.logError("Failed importing file " + file.path);
-			Zotero.logError(msg);
 			
 			// Clean up
 			try {
-				if (destDir && destDir.exists()) {
+				if (destDir && (yield OS.File.exists(destDir))) {
-					destDir.remove(true);
+					yield OS.File.removeDir(destDir);
 				}
 			}
 			catch (e) {
 				Zotero.logError(e);
 			}
-		}.bind(this))
+			
-		.then(function () {
+			throw e;
-			return attachmentItem;
+		}
-		});
+		
+		return attachmentItem;
 	});
 	
 	
@@ -174,39 +173,39 @@ Zotero.Attachments = new function(){
 		}
 		
 		var attachmentItem, itemID, destDir, newFile;
-		return Zotero.DB.executeTransaction(function* () {
+		try {
-			// Create a new attachment
+			yield Zotero.DB.executeTransaction(function* () {
-			attachmentItem = new Zotero.Item('attachment');
+				// Create a new attachment
-			let {libraryID, key: parentKey} = Zotero.Items.getLibraryAndKeyFromID(parentItemID);
+				attachmentItem = new Zotero.Item('attachment');
-			attachmentItem.libraryID = libraryID;
+				let {libraryID, key: parentKey} = Zotero.Items.getLibraryAndKeyFromID(parentItemID);
-			attachmentItem.setField('title', title);
+				attachmentItem.libraryID = libraryID;
-			attachmentItem.setField('url', url);
+				attachmentItem.setField('title', title);
-			attachmentItem.parentID = parentItemID;
+				attachmentItem.setField('url', url);
-			attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL;
+				attachmentItem.parentID = parentItemID;
-			attachmentItem.attachmentContentType = contentType;
+				attachmentItem.attachmentLinkMode = this.LINK_MODE_IMPORTED_URL;
-			attachmentItem.attachmentCharset = charset;
+				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
+				// DEBUG: this should probably insert access date too so as to
-			// translate.js, which sets the metadata fields itself
+				// create a proper item, but at the moment this is only called by
-			itemID = yield attachmentItem.save();
+				// translate.js, which sets the metadata fields itself
-			
+				itemID = yield attachmentItem.save();
-			var storageDir = Zotero.getStorageDirectory();
+				
-			destDir = this.getStorageDirectory(attachmentItem);
+				var storageDir = Zotero.getStorageDirectory();
-			yield _moveOrphanedDirectory(destDir);
+				destDir = this.getStorageDirectory(attachmentItem);
-			file.parent.copyTo(storageDir, destDir.leafName);
+				yield OS.File.removeDir(destDir.path);
-			
+				file.parent.copyTo(storageDir, destDir.leafName);
-			// Point to copied file
+				
-			newFile = destDir.clone();
+				// Point to copied file
-			newFile.append(file.leafName);
+				newFile = destDir.clone();
-			
+				newFile.append(file.leafName);
-			attachmentItem.attachmentPath = newFile.path;
+				
-			yield attachmentItem.save();
+				attachmentItem.attachmentPath = newFile.path;
-		}.bind(this))
+				yield attachmentItem.save();
-		.then(function () {
+			}.bind(this));
-			return _postProcessFile(attachmentItem, newFile, contentType, charset);
+			yield _postProcessFile(attachmentItem, newFile, contentType, charset);
-		})
+		}
-		.catch(function (e) {
+		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 () {
+			throw e;
-			return attachmentItem;
+		}
-		});
+		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<nsIFile>}
+	 * @return {Promise<String>} - 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);
+		var dir = this.getStorageDirectory(item).path;
-		yield _moveOrphanedDirectory(dir);
+		yield OS.File.removeDir(dir, { ignoreAbsent: true });
-		if (!dir.exists()) {
+		yield Zotero.File.createDirectoryIfMissingAsync(dir);
-			Zotero.debug("Creating directory " + dir.path);
-			dir.create(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0755);
-		}
 		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
 	 *

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;

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);

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;
+		yield Zotero.Attachments.createDirectoryForItem(item);
-		if (!(yield OS.File.exists(parentDirPath))) {
-			yield Zotero.Attachments.createDirectoryForItem(item);
-		}
-		
-		yield this._deleteExistingAttachmentFiles(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<Object[]>} - A promise for an array of conflict objects
 	 */

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));
+		});
+	});
 })

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;