Update 'type:' handling when migrating Extra lines

- When changing type based on 'type:' line, move existing fields that
  are no longer valid to Extra
- Remove 'type:' line with CSL type if the item's existing type is one
  of the types mapped to it

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

@@ -4634,10 +4634,10 @@ Zotero.Item.prototype.migrateExtraFields = function () {
 		if (itemType) {
 			Zotero.debug("Item Type: " + itemType);
 		}
-		if (fields.size) {
+		if (fields && fields.size) {
 			Zotero.debug("Fields:\n\n" + Array.from(fields.entries()).map(x => `${x[0]}: ${x[1]}`).join("\n"));
 		}
-		if (creators.length) {
+		if (creators && creators.length) {
 			Zotero.debug("Creators:");
 			Zotero.debug(creators);
 		}
@@ -4651,7 +4651,33 @@ Zotero.Item.prototype.migrateExtraFields = function () {
 			originalExtra, this
 		);
 		if (itemType) {
+			let originalType = this.itemTypeID;
+			let preJSON = this.toJSON();
+			let preKeys = Object.keys(preJSON);
+			
 			this.setType(Zotero.ItemTypes.getID(itemType));
+			
+			// Move any fields that were removed by the item type switch to Extra
+			let postJSON = this.toJSON();
+			let postKeys = Object.keys(postJSON)
+			let removedKeys = Zotero.Utilities.arrayDiff(preKeys, postKeys);
+			let addToExtra = [];
+			for (let key of removedKeys) {
+				// Follow base-field mappings
+				let baseFieldID = Zotero.ItemFields.getBaseIDFromTypeAndField(originalType, key);
+				let newField = baseFieldID
+					? Zotero.ItemFields.getFieldIDFromTypeAndBase(itemType, baseFieldID)
+					: null;
+				if (!newField) {
+					// "numPages" → "Num Pages"
+					let formattedKey = key[0].toUpperCase()
+						+ key.substr(1).replace(/([a-z])([A-Z])/, '$1 $2');
+					addToExtra.push(formattedKey + ': ' + preJSON[key]);
+				}
+			}
+			if (addToExtra.length) {
+				extra = (addToExtra.join('\n') + '\n' + extra).trim();
+			}
 		}
 		for (let [field, value] of fields) {
 			this.setField(field, value);

chrome/content/zotero/xpcom/schema.js

@@ -576,7 +576,8 @@ Zotero.Schema = new function(){
 			for (let zoteroType of data.csl.types[cslType]) {
 				Zotero.Schema.CSL_TYPE_MAPPINGS[zoteroType] = cslType;
 			}
-			Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[cslType] = data.csl.types[cslType][0];
+			// Add the first mapped Zotero type
+			Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[cslType] = [...data.csl.types[cslType]];
 		}
 		Zotero.Schema.CSL_TEXT_MAPPINGS = data.csl.fields.text;
 		Zotero.Schema.CSL_DATE_MAPPINGS = data.csl.fields.date;

chrome/content/zotero/xpcom/utilities.js

@@ -1654,7 +1654,7 @@ Zotero.Utilities = {
 			zoteroType = 'videoRecording';
 		}
 		else {
-			zoteroType = Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[cslItem.type];
+			zoteroType = Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[cslItem.type][0];
 		}
 		
 		if(!zoteroType) zoteroType = "document";

chrome/content/zotero/xpcom/utilities_internal.js

@@ -955,14 +955,6 @@ Zotero.Utilities.Internal = {
 		// Build `Map`s of normalized types/fields, including CSL variables, to built-in types/fields
 		//
 		
-		// Built-in item types
-		var itemTypes = new Map(Zotero.ItemTypes.getAll().map(x => [this._normalizeExtraKey(x.name), x.name]));
-		// CSL types
-		for (let i in Zotero.Schema.CSL_TYPE_MAPPINGS) {
-			let cslType = Zotero.Schema.CSL_TYPE_MAPPINGS[i];
-			itemTypes.set(cslType.toLowerCase(), i);
-		}
-		
 		// For fields we use arrays, because there can be multiple possibilities
 		//
 		// Built-in fields
@@ -990,40 +982,65 @@ Zotero.Utilities.Internal = {
 		var keepLines = [];
 		var skipKeys = new Set();
 		var lines = extra.split(/\n/g);
-		for (let line of lines) {
+		
+		var getKeyAndValue = (line) => {
 			let parts = line.match(/^([a-z][a-z -_]+):(.+)/i);
 			// Old citeproc.js cheater syntax;
 			if (!parts) {
 				parts = line.match(/^{:([a-z -_]+):(.+)}/i);
 			}
 			if (!parts) {
-				keepLines.push(line);
-				continue;
+				return [null, null];
 			}
 			let [_, originalField, value] = parts;
-			
 			let key = this._normalizeExtraKey(originalField);
-			if (skipKeys.has(key)) {
-				keepLines.push(line);
-				continue;
-			}
 			value = value.trim();
+			return [key, value];
+		};
 		
-			if (key == 'type') {
-				let possibleType = itemTypes.get(value);
-				if (possibleType) {
+		// Extract item type from 'type:' lines
+		lines = lines.filter((line) => {
+			let [key, value] = getKeyAndValue(line);
+			
+			if (!key
+					|| key != 'type'
+					|| skipKeys.has(key)
 					// Ignore 'type: note' and 'type: attachment'
-					if (['note', 'attachment'].includes(possibleType)) {
-						keepLines.push(line);
-						continue;
-					}
-					// Ignore item type that's the same as the item
-					if (!item || possibleType != Zotero.ItemTypes.getName(itemTypeID)) {
-						itemType = possibleType;
-						skipKeys.add(key);
-						continue;
+					|| ['note', 'attachment'].includes(value)) {
+				return true;
+			}
+			
+			// See if it's a Zotero type
+			let possibleType = Zotero.ItemTypes.getName(value);
+			
+			// If not, see if it's a CSL type
+			if (!possibleType && Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[value]) {
+				if (item) {
+					let currentType = Zotero.ItemTypes.getName(itemTypeID);
+					// If the current item type is valid for the given CSL type, remove the line
+					if (Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[value].includes(currentType)) {
+						return false;
 					}
 				}
+				// Use first mapped Zotero type for CSL type
+				possibleType = Zotero.Schema.CSL_TYPE_MAPPINGS_REVERSE[value][0];
+			}
+			
+			if (possibleType) {
+				itemType = possibleType;
+				itemTypeID = Zotero.ItemTypes.getID(itemType);
+				skipKeys.add(key);
+				return false;
+			}
+			
+			return true;
+		});
+		
+		lines = lines.filter((line) => {
+			let [key, value] = getKeyAndValue(line);
+			
+			if (!key || skipKeys.has(key)) {
+				return true;
 			}
 			
 			// Fields
@@ -1039,7 +1056,7 @@ Zotero.Utilities.Internal = {
 						if (!Zotero.ItemFields.isValidForType(fieldID, itemTypeID)
 								|| item.getField(fieldID)
 								|| additionalFields.has(possibleField)) {
-							continue;
+							return true;
 						}
 					}
 					fields.set(possibleField, value);
@@ -1052,7 +1069,7 @@ Zotero.Utilities.Internal = {
 				}
 				if (added) {
 					skipKeys.add(key);
-					continue;
+					return false;
 				}
 			}
 			
@@ -1076,24 +1093,24 @@ Zotero.Utilities.Internal = {
 							// to follow citeproc-js behavior
 							&& !item.getCreators().some(x => x.creatorType == possibleCreatorType)) {
 						creators.push(c);
-						continue;
+						return false;
 					}
 				}
 				else {
 					creators.push(c);
-					continue;
+					return false;
 				}
 			}
 			
 			// We didn't find anything, so keep the line in Extra
-			keepLines.push(line);
-		}
+			return true;
+		});
 		
 		return {
 			itemType,
 			fields,
 			creators,
-			extra: keepLines.join('\n')
+			extra: lines.join('\n')
 		};
 	},
 	

resource/schema/global

@@ -1 +1 @@
-Subproject commit 3bff7c83e63e60aa91d5359fcf02dcfb0f2dc18f
+Subproject commit 6dedc4e940ec10d622defd3b08978990e7646b10

test/tests/schemaTest.js

@@ -149,6 +149,61 @@ describe("Zotero.Schema", function() {
 				assert.isTrue(item.synced);
 			});
 			
+			it("should change item type if 'type:' is defined", async function () {
+				var item = await createDataObject('item', { itemType: 'document' });
+				item.setField('extra', 'type: personal_communication');
+				item.synced = true;
+				await item.saveTx();
+				
+				await migrate();
+				
+				assert.equal(item.itemTypeID, Zotero.ItemTypes.getID('letter'));
+				assert.equal(item.getField('extra'), '');
+				assert.isFalse(item.synced);
+			});
+			
+			it("should remove 'type:' line for CSL type if item is the first mapped Zotero type", async function () {
+				var item = await createDataObject('item', { itemType: 'letter' });
+				item.setField('extra', 'type: personal_communication');
+				item.synced = true;
+				await item.saveTx();
+				
+				await migrate();
+				
+				assert.equal(item.itemTypeID, Zotero.ItemTypes.getID('letter'));
+				assert.equal(item.getField('extra'), '');
+				assert.isFalse(item.synced);
+			});
+			
+			it("should remove 'type:' line for CSL type if item is a non-primary mapped Zotero type", async function () {
+				var item = await createDataObject('item', { itemType: 'instantMessage' });
+				item.setField('extra', 'type: personal_communication');
+				item.synced = true;
+				await item.saveTx();
+				
+				await migrate();
+				
+				assert.equal(item.itemTypeID, Zotero.ItemTypes.getID('instantMessage'));
+				assert.equal(item.getField('extra'), '');
+				assert.isFalse(item.synced);
+			});
+			
+			it("should move existing fields that would be invalid in the new 'type:' type to Extra", async function () {
+				var item = await createDataObject('item', { itemType: 'book' });
+				item.setField('numPages', '123');
+				item.setField('extra', 'type: article-journal\nJournal Abbreviation: abc.\nnumPages: 234');
+				item.synced = true;
+				await item.saveTx();
+				
+				await migrate();
+				
+				assert.equal(item.itemTypeID, Zotero.ItemTypes.getID('journalArticle'));
+				assert.equal(item.getField('journalAbbreviation'), 'abc.');
+				// Migrated real field should be placed at beginning, followed by unused line from Extra
+				assert.equal(item.getField('extra'), 'Num Pages: 123\nnumPages: 234');
+				assert.isFalse(item.synced);
+			});
+			
 			it("shouldn't migrate invalid item type", async function () {
 				var item = await createDataObject('item', { itemType: 'book' });
 				item.setField('numPages', 30);

test/tests/utilities_internalTest.js

@@ -129,6 +129,12 @@ describe("Zotero.Utilities.Internal", function () {
 			assert.strictEqual(extra, '');
 		});
 		
+		it("should use the first mapped Zotero type for a CSL type", function () {
+			var str = 'type: personal_communication';
+			var { itemType, fields, extra } = Zotero.Utilities.Internal.extractExtraFields(str);
+			assert.equal(itemType, 'letter');
+		});
+		
 		it("should extract a field", function () {
 			var val = '10.1234/abcdef';
 			var str = `DOI: ${val}`;