diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 451018d6f2..346ece00cd 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -4208,11 +4208,11 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { var isValidForType = {}; var setFields = new Set(); - /*var { fields: extraFields, creators: extraCreators, extra } = Zotero.Utilities.Internal.extractExtraFields( + var { fields: extraFields, creators: extraCreators, extra } = Zotero.Utilities.Internal.extractExtraFields( json.extra !== undefined ? json.extra : '', this, Object.keys(json) - );*/ + ); // Transfer valid fields from Extra to regular fields // Currently disabled @@ -4319,12 +4319,11 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { throw e; } // Otherwise store in Extra - // TEMP: Disabled for now, along with tests in itemTest.js - /*if (typeof val == 'string') { + if (typeof val == 'string') { Zotero.warn(`Storing unknown field '${field}' in Extra for item ${this.libraryKey}`); extraFields.set(field, val); break; - }*/ + } Zotero.warn(`Discarding unknown JSON ${typeof val} '${field}' for item ${this.libraryKey}`); continue; } @@ -4346,12 +4345,9 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { throw e; } // Otherwise store in Extra - // TEMP: Disabled for now, since imports can assign values to multiple versions of - // fields - // https://groups.google.com/d/msg/zotero-dev/a1IPUJ2m_3s/hfmdK2P3BwAJ - /*Zotero.warn(`Storing invalid field '${origField}' for type ${type} in Extra for ` + Zotero.warn(`Storing invalid field '${origField}' for type ${type} in Extra for ` + `item ${this.libraryKey}`); - extraFields.set(field, val);*/ + extraFields.set(field, val); continue; } this.setField(field, json[origField]); @@ -4359,8 +4355,37 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { } } - //this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields)); - this.setField('extra', json.extra !== undefined ? json.extra : ''); + // If one of the valid fields is a base field or a base-mapped field, remove all other + // associated fields from Extra. This could be removed if we made sure that translators didn't + // 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) { + for (let field of setFields.keys()) { + let baseField; + if (Zotero.ItemFields.isBaseField(field)) { + baseField = field; + } + else { + let baseFieldID = Zotero.ItemFields.getBaseIDFromTypeAndField(itemTypeID, field); + if (baseFieldID) { + baseField = baseFieldID; + } + } + if (baseField) { + let mappedFieldNames = Zotero.ItemFields.getTypeFieldsFromBase(baseField, true); + for (let mappedField of mappedFieldNames) { + if (extraFields.has(mappedField)) { + Zotero.warn(`Removing redundant Extra field '${mappedField}' for item ` + + this.libraryKey); + extraFields.delete(mappedField); + } + } + } + } + } + + this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields)); if (json.collections || this._collections.length) { this.setCollections(json.collections); diff --git a/chrome/content/zotero/xpcom/utilities_internal.js b/chrome/content/zotero/xpcom/utilities_internal.js index f2a6439e08..f6eb79a2d3 100644 --- a/chrome/content/zotero/xpcom/utilities_internal.js +++ b/chrome/content/zotero/xpcom/utilities_internal.js @@ -1131,7 +1131,7 @@ Zotero.Utilities.Internal = { } } var fieldPairs = Array.from(fields.entries()) - .map(x => x[0] + ': ' + x[1]); + .map(x => this.camelToTitleCase(x[0]) + ': ' + x[1]); fieldPairs.sort(); return fieldPairs.join('\n') + ((fieldPairs.length && keepLines.length) ? "\n" : "") @@ -1382,7 +1382,7 @@ Zotero.Utilities.Internal = { camelToTitleCase: function (str) { - str = str.replace(/([A-Z])/g, " $1"); + str = str.replace(/([a-z])([A-Z])/g, "$1 $2"); return str.charAt(0).toUpperCase() + str.slice(1); }, diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index 3df03e0430..2d22dcb7f4 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1754,6 +1754,18 @@ describe("Zotero.Item", function () { assert.isFalse(item.inPublications); }); + it("should handle Extra in non-strict mode", function () { + var json = { + itemType: "journalArticle", + title: "Test", + extra: "Here's some extra text" + }; + var item = new Zotero.Item(); + item.fromJSON(json); + assert.equal(item.getField('extra'), json.extra); + }); + + // Not currently following this behavior /*it("should move valid field in Extra to field if not set", function () { var doi = '10.1234/abcd'; @@ -1799,19 +1811,20 @@ describe("Zotero.Item", function () { assert.equal(item.getField('extra'), `doi: ${doi2}`); });*/ - it.skip("should store unknown field in Extra in non-strict mode", function () { + it("should store unknown fields in Extra in non-strict mode", function () { var json = { itemType: "journalArticle", title: "Test", - foo: "Bar" + fooBar: "123", + testField: "test value" }; var item = new Zotero.Item; item.fromJSON(json); assert.equal(item.getField('title'), 'Test'); - assert.equal(item.getField('extra'), 'foo: Bar'); + assert.equal(item.getField('extra'), 'Foo Bar: 123\nTest Field: test value'); }); - it.skip("should replace unknown field in Extra in non-strict mode", function () { + it("should replace unknown field in Extra in non-strict mode", function () { var json = { itemType: "journalArticle", title: "Test", @@ -1824,15 +1837,42 @@ describe("Zotero.Item", function () { assert.equal(item.getField('extra'), 'Foo: BBB\nBar: CCC'); }); - it("should handle Extra in non-strict mode", function () { + it("should store invalid-for-type field in Extra in non-strict mode", function () { var json = { itemType: "journalArticle", title: "Test", - extra: "Here's some extra text" + medium: "123" }; - var item = new Zotero.Item(); + var item = new Zotero.Item; item.fromJSON(json); - assert.equal(item.getField('extra'), json.extra); + assert.equal(item.getField('title'), 'Test'); + assert.equal(item.getField('extra'), 'Medium: 123'); + }); + + it("should ignore invalid-for-type base-mapped field if valid-for-type base field is set in Extra in non-strict mode", function () { + var json = { + itemType: "document", + publisher: "Foo", // Valid for 'document' + company: "Bar" // Not invalid for 'document', but mapped to base field 'publisher' + }; + var item = new Zotero.Item; + item.fromJSON(json); + assert.equal(item.getField('publisher'), 'Foo'); + assert.equal(item.getField('extra'), ''); + }); + + it("shouldn't include base field or invalid base-mapped field in Extra if valid base-mapped field is set in non-strict mode", function () { + var json = { + itemType: "audioRecording", + publisher: "A", // Base field, which will be overwritten by the valid base-mapped field + label: "B", // Valid base-mapped field, which should be stored + company: "C", // Invalid base-mapped field, which should be ignored + foo: "D" // Invalid other field, which should be added to Extra + }; + var item = new Zotero.Item; + item.fromJSON(json); + assert.equal(item.getField('label'), 'B'); + assert.equal(item.getField('extra'), 'Foo: D'); }); it("should throw on unknown field in strict mode", function () { diff --git a/test/tests/utilities_internalTest.js b/test/tests/utilities_internalTest.js index 73191131d1..b4db6f431b 100644 --- a/test/tests/utilities_internalTest.js +++ b/test/tests/utilities_internalTest.js @@ -253,7 +253,7 @@ describe("Zotero.Utilities.Internal", function () { fieldMap.set('originalDate', originalDate); fieldMap.set('publicationPlace', publicationPlace); fieldMap.set('DOI', doi); - var fieldStr = `DOI: ${doi}\noriginalDate: ${originalDate}\npublicationPlace: ${publicationPlace}`; + var fieldStr = `DOI: ${doi}\nOriginal Date: ${originalDate}\nPublication Place: ${publicationPlace}`; it("should create 'field: value' pairs from field map", function () { var extra = ""; @@ -272,7 +272,7 @@ describe("Zotero.Utilities.Internal", function () { var newExtra = ZUI.combineExtraFields(extra, fieldMap); assert.equal( newExtra, - fieldStr.split(/\n/).filter(x => !x.startsWith('originalDate')).join("\n") + fieldStr.split(/\n/).filter(x => !x.startsWith('Original Date')).join("\n") + "\nThis is a note.\nOriginal Date: 1887\nFoo: Bar" ); }); @@ -386,6 +386,21 @@ describe("Zotero.Utilities.Internal", function () { }); }); + describe("#camelToTitleCase()", function () { + it("should convert 'fooBar' to 'Foo Bar'", function () { + assert.equal(Zotero.Utilities.Internal.camelToTitleCase('fooBar'), 'Foo Bar'); + }); + + it("should keep all-caps strings intact", function () { + assert.equal(Zotero.Utilities.Internal.camelToTitleCase('DOI'), 'DOI'); + }); + + it("should convert 'fooBAR' to 'Foo BAR'", function () { + assert.equal(Zotero.Utilities.Internal.camelToTitleCase('fooBAR'), 'Foo BAR'); + }); + + }); + describe("#getNextName()", function () { it("should get the next available numbered name", function () { var existing = ['Name', 'Name 1', 'Name 3'];