From 424cae173be31c442d1e0e4ecc2fdfc4fb223634 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 2 Jun 2015 19:08:52 -0400 Subject: [PATCH] Fix getField/setField behavior/tests with regard to falsy values In particular, 0 is kept as a value, and passing undefined to setField now throws an error. I'm not sure if we actually want to return an empty string in all cases for missing/invalid fields, but that's what we do currently. --- chrome/content/zotero/xpcom/data/item.js | 6 +- test/tests/itemTest.js | 88 +++++++++++++++++++----- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 9fc4eddbdd..5e528777ec 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -269,7 +269,7 @@ Zotero.Item.prototype.getField = function(field, unformatted, includeBaseMapped) ); } - value = value ? value : ''; + value = (value !== null && value !== false) ? value : ''; if (!unformatted) { // Multipart date fields @@ -619,6 +619,10 @@ Zotero.Item.prototype.getFieldsNotInType = function (itemTypeID, allowBaseConver Zotero.Item.prototype.setField = function(field, value, loadIn) { this._disabledCheck(); + if (value === undefined) { + throw new Error("Value cannot be undefined"); + } + if (typeof value == 'string') { value = value.trim().normalize(); } diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index 5364fb0916..0595c72052 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -1,64 +1,114 @@ describe("Zotero.Item", function () { describe("#getField()", function () { - it("should return false for valid unset fields on unsaved items", function () { + it("should return an empty string for valid unset fields on unsaved items", function () { var item = new Zotero.Item('book'); - assert.equal(item.getField('rights'), false); + assert.strictEqual(item.getField('rights'), ""); }); - it("should return false for valid unset fields on unsaved items after setting on another field", function () { + it("should return an empty string for valid unset fields on unsaved items after setting on another field", function () { var item = new Zotero.Item('book'); item.setField('title', 'foo'); - assert.equal(item.getField('rights'), false); + assert.strictEqual(item.getField('rights'), ""); }); - it("should return false for invalid unset fields on unsaved items after setting on another field", function () { + it("should return an empty string for invalid unset fields on unsaved items after setting on another field", function () { var item = new Zotero.Item('book'); item.setField('title', 'foo'); - assert.equal(item.getField('invalid'), false); + assert.strictEqual(item.getField('invalid'), ""); }); }); describe("#setField", function () { it("should throw an error if item type isn't set", function () { var item = new Zotero.Item; - assert.throws(item.setField.bind(item, 'title', 'test'), "Item type must be set before setting field data"); + assert.throws(() => item.setField('title', 'test'), "Item type must be set before setting field data"); }) it("should mark a field as changed", function () { var item = new Zotero.Item('book'); item.setField('title', 'Foo'); - assert.ok(item._changed.itemData[Zotero.ItemFields.getID('title')]); - assert.ok(item.hasChanged()); + assert.isTrue(item._changed.itemData[Zotero.ItemFields.getID('title')]); + assert.isTrue(item.hasChanged()); }) - it("should clear an existing field set to a falsy value", function* () { + it('should clear an existing field when ""/null/false is passed', function* () { var field = 'title'; + var val = 'foo'; var fieldID = Zotero.ItemFields.getID(field); var item = new Zotero.Item('book'); - item.setField(field, 'Foo'); - id = yield item.saveTx(); - item = yield Zotero.Items.getAsync(id); + item.setField(field, val); + yield item.saveTx(); item.setField(field, ""); assert.ok(item._changed.itemData[fieldID]); - assert.ok(item.hasChanged()); + assert.isTrue(item.hasChanged()); + + // Reset to original value yield item.reload(); - assert.isFalse(item.hasChanged()); + assert.equal(item.getField(field), val); + // false item.setField(field, false); assert.ok(item._changed.itemData[fieldID]); - assert.ok(item.hasChanged()); + assert.isTrue(item.hasChanged()); + + // Reset to original value yield item.reload(); - assert.isFalse(item.hasChanged()); + assert.equal(item.getField(field), val); + // null item.setField(field, null); assert.ok(item._changed.itemData[fieldID]); - assert.ok(item.hasChanged()); + assert.isTrue(item.hasChanged()); yield item.saveTx(); - assert.isFalse(item.getField(fieldID)); + assert.equal(item.getField(field), ""); + }) + + it('should clear a field set to 0 when a ""/null/false is passed', function* () { + var field = 'title'; + var val = 0; + var fieldID = Zotero.ItemFields.getID(field); + var item = new Zotero.Item('book'); + item.setField(field, val); + yield item.saveTx(); + + assert.strictEqual(item.getField(field), val); + + // "" + item.setField(field, ""); + assert.ok(item._changed.itemData[fieldID]); + assert.isTrue(item.hasChanged()); + + // Reset to original value + yield item.reload(); + assert.isFalse(item.hasChanged()); + assert.strictEqual(item.getField(field), val); + + // False + item.setField(field, false); + assert.ok(item._changed.itemData[fieldID]); + assert.isTrue(item.hasChanged()); + + // Reset to original value + yield item.reload(); + assert.isFalse(item.hasChanged()); + assert.strictEqual(item.getField(field), val); + + // null + item.setField(field, null); + assert.ok(item._changed.itemData[fieldID]); + assert.isTrue(item.hasChanged()); + + yield item.saveTx(); + assert.strictEqual(item.getField(field), ""); + }) + + it("should throw if value is undefined", function () { + var item = new Zotero.Item('book'); + assert.throws(() => item.setField('title'), "Value cannot be undefined"); }) it("should not mark an empty field set to an empty string as changed", function () {