mirrors/zotero
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Automatically save unknown/invalid fields to Extra in non-strict mode

Browse source 
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
This commit is contained in:
Dan Stillman 2020-01-16 16:56:25 -05:00
parent 55c88dc91c
commit 3de54455f6
4 changed files with 104 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

49
chrome/content/zotero/xpcom/data/item.js
View file

@ -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);

4
chrome/content/zotero/xpcom/utilities_internal.js
View file

@ -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);
},

56
test/tests/itemTest.js
View file

@ -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 () {

19
test/tests/utilities_internalTest.js
View file

@ -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'];