From 1710eb1c4b93ec4fc56f72f0880b95a5f9335d5c Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 18 Oct 2019 03:20:18 -0400 Subject: [PATCH] Don't store unknown/invalid fields in Extra in non-strict mode And fix a couple things for if we turn it back on This code came along with the type/field handling overhaul, but I think it was originally intended for handling unknown fields during sync before we decided on strict mode, so it wasn't finished and causes various problems [1]. It could still be useful for preserving fields from translators before they're available on items, but the better fix there is just to add the missing fields, so I'm not sure if we'll end up needing it. [1] https://groups.google.com/d/msg/zotero-dev/a1IPUJ2m_3s/hfmdK2P3BwAJ --- chrome/content/zotero/xpcom/data/item.js | 20 ++++++++++++------- .../zotero/xpcom/utilities_internal.js | 2 +- test/tests/itemTest.js | 4 ++-- test/tests/utilities_internalTest.js | 17 ++++++++++++++++ 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 7dbe7fceff..e152c8906f 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -4206,11 +4206,11 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { var isValidForType = {}; var setFields = new Set(); - var { fields: extraFields, 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 @@ -4266,6 +4266,7 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { break; case 'creators': + //this.setCreators(json.creators.concat(extraCreators), options); this.setCreators(json.creators, options); break; @@ -4316,11 +4317,12 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { throw e; } // Otherwise store in Extra - if (typeof val == 'string') { + // TEMP: Disabled for now, along with tests in itemTest.js + /*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; } @@ -4342,9 +4344,12 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { throw e; } // Otherwise store in Extra - Zotero.warn(`Storing invalid field '${origField}' for type ${type} in Extra for ` + // 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 ` + `item ${this.libraryKey}`); - extraFields.set(field, val); + extraFields.set(field, val);*/ continue; } this.setField(field, json[origField]); @@ -4352,7 +4357,8 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) { } } - this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields)); + //this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields)); + this.setField('extra', json.extra !== undefined ? json.extra : ''); 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 309f677c45..8ffb12fe44 100644 --- a/chrome/content/zotero/xpcom/utilities_internal.js +++ b/chrome/content/zotero/xpcom/utilities_internal.js @@ -993,7 +993,7 @@ Zotero.Utilities.Internal = { var skipKeys = new Set(); var lines = extra.split(/\n/g); for (let line of lines) { - let parts = line.match(/^([a-z -_]+):(.+)/i); + let parts = line.match(/^([a-z][a-z -_]+):(.+)/i); // Old citeproc.js cheater syntax; if (!parts) { parts = line.match(/^{:([a-z -_]+):(.+)}/i); diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index d5947b53ea..3df03e0430 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1799,7 +1799,7 @@ describe("Zotero.Item", function () { assert.equal(item.getField('extra'), `doi: ${doi2}`); });*/ - it("should store unknown field in Extra in non-strict mode", function () { + it.skip("should store unknown field in Extra in non-strict mode", function () { var json = { itemType: "journalArticle", title: "Test", @@ -1811,7 +1811,7 @@ describe("Zotero.Item", function () { assert.equal(item.getField('extra'), 'foo: Bar'); }); - it("should replace unknown field in Extra in non-strict mode", function () { + it.skip("should replace unknown field in Extra in non-strict mode", function () { var json = { itemType: "journalArticle", title: "Test", diff --git a/test/tests/utilities_internalTest.js b/test/tests/utilities_internalTest.js index 212db95930..7a770bfe46 100644 --- a/test/tests/utilities_internalTest.js +++ b/test/tests/utilities_internalTest.js @@ -190,6 +190,12 @@ describe("Zotero.Utilities.Internal", function () { assert.equal(extra, "Publisher Place: " + place2); }); + it("shouldn't extract a field from a line that begins with a whitespace", function () { + var str = '\n number-of-pages: 11'; + var { fields, extra } = Zotero.Utilities.Internal.extractExtraFields(str); + assert.equal(fields.size, 0); + }); + it("shouldn't extract a field that already exists on the item", function () { var item = createUnsavedDataObject('item', { itemType: 'book' }); item.setField('numPages', 10); @@ -198,6 +204,17 @@ describe("Zotero.Utilities.Internal", function () { assert.equal(fields.size, 0); }); + it("should extract an author and add it to existing creators", function () { + var item = createUnsavedDataObject('item', { itemType: 'book' }); + item.setCreator(0, { creatorType: 'author', name: 'Foo' }); + var str = 'author: Bar'; + var { fields, creators, extra } = Zotero.Utilities.Internal.extractExtraFields(str, item); + assert.equal(fields.size, 0); + assert.lengthOf(creators, 1); + assert.equal(creators[0].creatorType, 'author'); + assert.equal(creators[0].name, 'Bar'); + }); + it("should extract a CSL name", function () { var str = 'container-author: First || Last'; var { creators, extra } = Zotero.Utilities.Internal.extractExtraFields(str);