Fix relinking of imported attachment with external file

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;

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);
+	// If selected file isn't in the attachment's storage directory,
-	
+	// copy it in and use that one instead
-	try {
+	var storageDir = Zotero.Attachments.getStorageDirectory(this).path;
-		// If selected file isn't in the attachment's storage directory,
+	if (this.isImportedAttachment() && OS.Path.dirname(path) != storageDir) {
-		// copy it in and use that one instead
+		newPath = OS.Path.join(storageDir, newName);
-		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,
-			// If file with same name already exists in the storage directory,
+		// move it out of the way
-			// move it out of the way
+		let backupCreated = false;
-			let deleteBackup = false;;
+		if (yield OS.File.exists(newPath)) {
-			if (yield OS.File.exists(newPath)) {
+			backupCreated = true;
-				deleteBackup = true;
+			yield OS.File.move(newPath, newPath + ".bak");
-				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");
-			}
 		}
-		// Rename file to filtered name if necessary
+		// Create storage directory if necessary
-		else if (fileName != newName) {
+		else if (!(yield OS.File.exists(storageDir))) {
-			Zotero.debug("Renaming file '" + fileName + "' to '" + newName + "'");
+			Zotero.Attachments.createDirectoryForItem(this);
-			OS.File.move(path, newPath, { noOverwrite: true });
+		}
+		
+		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) {
+	// Rename file to filtered name if necessary
-		Zotero.logError(e);
+	else if (fileName != newName) {
-		return false;
+		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;
 });
 

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)) {
+			try {
-				Zotero.debug("File renamed without library access -- "
+				if (!Zotero.Items.isEditable(item)) {
-					+ "updating itemAttachments path", 3);
+					Zotero.debug("File renamed without library access -- "
-				yield item.relinkAttachmentFile(newPath, true);
+						+ "updating itemAttachments path", 3);
+					yield item.relinkAttachmentFile(newPath, true);
+				}
+				else {
+					yield item.relinkAttachmentFile(newPath);
+				}
 			}
-			else {
+			catch (e) {
-				yield item.relinkAttachmentFile(newPath);
+				Zotero.File.checkFileAccessError(e, path, 'update');
 			}
 			
 			path = newPath;

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.getFilePath();
-			var file = item.getFile(false, true);
+			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;
 			}
 			

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 = [