From a9dee2f48795e20d344b63538ad835efbca2b103 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Mon, 17 Feb 2020 11:35:39 -0500
Subject: [PATCH] Item::fromJSON(): Remove invalid-for-type Type-mapped fields

"Type: [val]` in Extra means Item Type to citeproc-js, and Type values
from translators mostly aren't going to be useful if the item type
doesn't have a Type-mapped field.

https://forums.zotero.org/discussion/comment/348864/#Comment_348864
---
 chrome/content/zotero/xpcom/data/item.js | 32 ++++++----------
 test/tests/itemTest.js                   | 48 +-----------------------
 2 files changed, 13 insertions(+), 67 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 73873eb63c..9d608d8c7c 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -4408,27 +4408,17 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 			}
 		}
 		
-		// Special handling for Type fields, since Type isn't a base field and therefore isn't set
-		// in translators that are doing this
-		if (!extraFields.has('type')) {
-			let typeFieldNames = Zotero.ItemFields.getTypeFieldsFromBase('type', true)
-				// This is actually 'medium' but as of 2/2020 the Embedded Metadata translator
-				// assigns it along with the other 'type' fields.
-				.concat('audioFileType');
-			let typeValue = false;
-			for (let typeFieldName of typeFieldNames) {
-				let value = extraFields.get(typeFieldName);
-				if (value !== undefined) {
-					if (typeValue === false) {
-						typeValue = value;
-						extraFields.set('type', value);
-					}
-					if (typeValue == value) {
-						Zotero.warn(`Removing redundant Extra field '${typeFieldName}' for item `
-							+ this.libraryKey);
-						extraFields.delete(typeFieldName);
-					}
-				}
+		// Remove Type-mapped fields from Extra, since 'Type' is mapped to Item Type by citeproc-js
+		// and Type values mostly aren't going to be useful for item types without a Type-mapped field.
+		let typeFieldNames = Zotero.ItemFields.getTypeFieldsFromBase('type', true)
+			// This is actually 'medium' but as of 2/2020 the Embedded Metadata translator
+			// assigns it along with the other 'type' fields.
+			.concat('audioFileType');
+		for (let typeFieldName of typeFieldNames) {
+			if (extraFields.has(typeFieldName)) {
+				Zotero.warn(`Removing invalid-for-type Type field '${typeFieldName}' from Extra for item `
+					+ this.libraryKey);
+				extraFields.delete(typeFieldName);
 			}
 		}
 	}
diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js
index 47f0204a25..e3ee830ec0 100644
--- a/test/tests/itemTest.js
+++ b/test/tests/itemTest.js
@@ -1898,24 +1898,7 @@ describe("Zotero.Item", function () {
 				assert.equal(item.getField('extra'), 'Publisher: Foo');
 			});
 			
-			it("should deduplicate invalid-for-type base-mapped Type fields with same values and keep Type when storing in Extra", function () {
-				var json = {
-					itemType: "document",
-					type: "Foo", // Invalid base field
-					reportType: "Foo", // Invalid base-mapped field
-					websiteType: "Foo" // Invaid base-mapped field
-				};
-				// Confirm that 'type' is still invalid for 'document', in case this changes
-				assert.isFalse(Zotero.ItemFields.isValidForType(
-					Zotero.ItemFields.getID('type'),
-					Zotero.ItemTypes.getID('document')
-				));
-				var item = new Zotero.Item;
-				item.fromJSON(json);
-				assert.equal(item.getField('extra'), 'Type: Foo');
-			});
-			
-			it("should remove invalid-for-type base-mapped Type fields with same values and use Type if not present when storing in Extra", function () {
+			it("should remove invalid-for-type base-mapped Type fields when storing in Extra", function () {
 				var json = {
 					itemType: "document",
 					reportType: "Foo", // Invalid base-mapped field
@@ -1928,34 +1911,7 @@ describe("Zotero.Item", function () {
 				));
 				var item = new Zotero.Item;
 				item.fromJSON(json);
-				assert.equal(item.getField('extra'), 'Type: Foo');
-			});
-			
-			it("shouldn't deduplicate invalid-for-type base-mapped Type fields with different values when storing in Extra", function () {
-				var json = {
-					itemType: "document",
-					reportType: "Foo", // Invalid base-mapped field
-					type: "Foo", // Invalid base field
-					websiteType: "Bar" // Invaid base-mapped field with different value
-				};
-				var item = new Zotero.Item;
-				item.fromJSON(json);
-				assert.equal(item.getField('extra'), 'Type: Foo\nWebsite Type: Bar');
-			});
-			
-			it("shouldn't deduplicate invalid-for-type base-mapped Type fields with different values when storing in Extra", function () {
-				var json = {
-					itemType: "artwork",
-					publisher: "Foo", // Invalid base field
-					company: "Foo", // Invalid base-mapped field
-					label: "Bar" // Invaid base-mapped field with different value
-				};
-				var item = new Zotero.Item;
-				item.fromJSON(json);
-				var parts = item.getField('extra').split('\n');
-				assert.lengthOf(parts, 2);
-				assert.include(parts, 'Publisher: Foo');
-				assert.include(parts, 'Label: Bar');
+				assert.equal(item.getField('extra'), '');
 			});
 		});