Attachment merge: Compare linkModes, improve logging

Using keys instead of IDs should make logs more useful.
Fixes #2527.

chrome/content/zotero/xpcom/data/items.js

@@ -1060,7 +1060,7 @@ Zotero.Items = function() {
 				}
 
 				if (!masterAttachmentID || mergedMasterAttachments.has(masterAttachmentID)) {
-					Zotero.debug(`No unmerged match for attachment ${otherAttachment.id} in master item - moving`);
+					Zotero.debug(`No unmerged match for attachment ${otherAttachment.key} in master item - moving`);
 					otherAttachment.parentItemID = item.id;
 					await otherAttachment.save();
 					continue;
@@ -1070,14 +1070,22 @@ Zotero.Items = function() {
 				let masterAttachment = await this.getAsync(masterAttachmentID);
 
 				if (masterAttachment.attachmentContentType !== otherAttachment.attachmentContentType) {
-					Zotero.debug(`Master attachment ${masterAttachmentID} matches ${otherAttachment.id}, `
+					Zotero.debug(`Master attachment ${masterAttachment.key} matches ${otherAttachment.key}, `
 						+ 'but content types differ - moving');
 					otherAttachment.parentItemID = item.id;
 					await otherAttachment.save();
 					continue;
 				}
 
-				Zotero.debug(`Master attachment ${masterAttachmentID} matches ${otherAttachment.id} - merging`);
+				if (masterAttachment.attachmentLinkMode !== otherAttachment.attachmentLinkMode) {
+					Zotero.debug(`Master attachment ${masterAttachment.key} matches ${otherAttachment.key}, `
+						+ 'but link modes differ - moving');
+					otherAttachment.parentItemID = item.id;
+					await otherAttachment.save();
+					continue;
+				}
+
+				Zotero.debug(`Master attachment ${masterAttachment.key} matches ${otherAttachment.key} - merging`);
 				await this.moveChildItems(otherAttachment, masterAttachment, true);
 				await this._moveEmbeddedNote(otherAttachment, masterAttachment);
 				await this._moveRelations(otherAttachment, masterAttachment);
@@ -1125,7 +1133,7 @@ Zotero.Items = function() {
 				);
 
 				if (!masterAttachment) {
-					Zotero.debug(`No match for web attachment ${otherAttachment.id} in master item - moving`);
+					Zotero.debug(`No match for web attachment ${otherAttachment.key} in master item - moving`);
 					otherAttachment.parentItemID = item.id;
 					await otherAttachment.save();
 					continue;

test/tests/itemsTest.js

@@ -819,6 +819,51 @@ describe("Zotero.Items", function () {
 			assert.lengthOf(rels, 2);
 			assert.sameMembers(rels, [attachment2URI, attachment3URI]);
 		});
+
+		it("should not merge attachments with different content types", async function () {
+			let item1 = await createDataObject('item', { setTitle: true });
+			let attachment1 = await importPDFAttachment(item1);
+
+			let item2 = item1.clone();
+			await item2.saveTx();
+			let attachment2 = await importPDFAttachment(item2);
+			attachment2.attachmentContentType = 'text/plain';
+			await attachment2.saveTx();
+
+			await Zotero.Items.merge(item1, [item2]);
+
+			assert.isFalse(item1.deleted);
+			assert.isFalse(attachment1.deleted);
+			assert.equal(item1.numAttachments(true), 2);
+			assert.isTrue(item2.deleted);
+			assert.isFalse(attachment2.deleted);
+			assert.equal(attachment2.parentItemID, item1.id);
+		});
+
+		it("should not merge attachments with different link modes", async function () {
+			let file = getTestDataDirectory();
+			file.append('test.pdf');
+
+			let item1 = await createDataObject('item', { setTitle: true });
+			let attachment1 = await Zotero.Attachments.linkFromFile({
+				file,
+				parentItemID: item1.id
+			});
+
+			let item2 = item1.clone();
+			await item2.saveTx();
+			let attachment2 = await importPDFAttachment(item2);
+
+			await Zotero.Items.merge(item1, [item2]);
+
+			assert.equal(await attachment1.attachmentHash, await attachment2.attachmentHash);
+			assert.isFalse(item1.deleted);
+			assert.isFalse(attachment1.deleted);
+			assert.equal(item1.numAttachments(true), 2);
+			assert.isTrue(item2.deleted);
+			assert.isFalse(attachment2.deleted);
+			assert.equal(attachment2.parentItemID, item1.id);
+		});
 	})