From 52d5b68564f1df7e6930edd8965c1ba9e5216103 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sun, 22 Mar 2020 15:19:24 -0400 Subject: [PATCH] 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 | 30 ++++++- chrome/content/zotero/xpcom/schema.js | 3 +- chrome/content/zotero/xpcom/utilities.js | 2 +- .../zotero/xpcom/utilities_internal.js | 87 +++++++++++-------- resource/schema/global | 2 +- test/tests/schemaTest.js | 55 ++++++++++++ test/tests/utilities_internalTest.js | 6 ++ 7 files changed, 145 insertions(+), 40 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 1903134941..7a5f0a3dd7 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/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); diff --git a/chrome/content/zotero/xpcom/schema.js b/chrome/content/zotero/xpcom/schema.js index 397bf59c77..81710957b4 100644 --- a/chrome/content/zotero/xpcom/schema.js +++ b/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; diff --git a/chrome/content/zotero/xpcom/utilities.js b/chrome/content/zotero/xpcom/utilities.js index 1a62611fb1..e21719bb8e 100644 --- a/chrome/content/zotero/xpcom/utilities.js +++ b/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"; diff --git a/chrome/content/zotero/xpcom/utilities_internal.js b/chrome/content/zotero/xpcom/utilities_internal.js index cabcd4f29c..4b1b9919a3 100644 --- a/chrome/content/zotero/xpcom/utilities_internal.js +++ b/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]; + }; + + // Extract item type from 'type:' lines + lines = lines.filter((line) => { + let [key, value] = getKeyAndValue(line); - if (key == 'type') { - let possibleType = itemTypes.get(value); - if (possibleType) { + 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') }; }, diff --git a/resource/schema/global b/resource/schema/global index 3bff7c83e6..6dedc4e940 160000 --- a/resource/schema/global +++ b/resource/schema/global @@ -1 +1 @@ -Subproject commit 3bff7c83e63e60aa91d5359fcf02dcfb0f2dc18f +Subproject commit 6dedc4e940ec10d622defd3b08978990e7646b10 diff --git a/test/tests/schemaTest.js b/test/tests/schemaTest.js index 075c0dbdb1..7c0b8d989f 100644 --- a/test/tests/schemaTest.js +++ b/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); diff --git a/test/tests/utilities_internalTest.js b/test/tests/utilities_internalTest.js index 4c56f94cdd..20e90ebf4e 100644 --- a/test/tests/utilities_internalTest.js +++ b/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}`;