From 3de54455f68bbcd376505de5a5d3503831e8b901 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 16 Jan 2020 16:56:25 -0500 Subject: [PATCH] Automatically save unknown/invalid fields to Extra in non-strict mode This is a prerequisite for starting to use new fields in translators, since otherwise switching from, say, storing originalDate in Extra to using an originalDate field would cause the value to be lost in clients without the newer schema. Closes #1504 --- chrome/content/zotero/xpcom/data/item.js | 49 ++++++++++++---- .../zotero/xpcom/utilities_internal.js | 4 +- test/tests/itemTest.js | 56 ++++++++++++++++--- test/tests/utilities_internalTest.js | 19 ++++++- 4 files changed, 104 insertions(+), 24 deletions(-) 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'];