diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index e45609098a..73873eb63c 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -4362,7 +4362,7 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { // try to save multiple versions of base-mapped fields, which they shouldn't need to do. // // https://github.com/zotero/zotero/issues/1504#issuecomment-572415083 - if (extraFields.size) { + if (!strict && extraFields.size) { for (let field of setFields.keys()) { let baseField; if (Zotero.ItemFields.isBaseField(field)) { @@ -4385,6 +4385,52 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { } } } + + // + // Deduplicate remaining Extra fields + // + // For each invalid-for-type base field, remove any mapped fields with the same value + let baseFields = []; + for (let field of extraFields.keys()) { + if (Zotero.ItemFields.getID(field) && Zotero.ItemFields.isBaseField(field)) { + baseFields.push(field); + } + } + for (let baseField of baseFields) { + let value = extraFields.get(baseField); + let mappedFieldNames = Zotero.ItemFields.getTypeFieldsFromBase(baseField, true); + for (let mappedField of mappedFieldNames) { + if (extraFields.has(mappedField) && extraFields.get(mappedField) === value) { + Zotero.warn(`Removing redundant Extra field '${mappedField}' for item ` + + this.libraryKey); + extraFields.delete(mappedField); + } + } + } + + // 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); + } + } + } + } } this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields)); diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index 6c5daae97d..47f0204a25 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1885,6 +1885,78 @@ describe("Zotero.Item", function () { assert.equal(item.getField('label'), 'B'); assert.equal(item.getField('extra'), 'Foo: D'); }); + + it("should remove invalid-for-type base-mapped fields with same values and use base field if not present when storing in Extra", function () { + var json = { + itemType: "artwork", + publisher: "Foo", // Invalid base field + company: "Foo", // Invalid base-mapped field + label: "Foo" // Invaid base-mapped field + }; + var item = new Zotero.Item; + item.fromJSON(json); + 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 () { + var json = { + itemType: "document", + 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("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'); + }); }); describe("strict mode", function () {