From 5fc524bcb2a3e48b2786642040758969e77c30de Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 1 Jun 2015 19:52:17 -0400 Subject: [PATCH] Generalize Zotero.CachedTypes.add(), and tweak item charset handling .attachmentCharset on an item now requires a string, not a charsetID. (It accepted a charset before but didn't quite work right.) --- chrome/content/zotero/xpcom/attachments.js | 5 +- .../content/zotero/xpcom/data/cachedTypes.js | 130 ++++++++++-------- chrome/content/zotero/xpcom/data/item.js | 10 +- test/tests/itemTest.js | 14 ++ 4 files changed, 98 insertions(+), 61 deletions(-) diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js index f02d78f740..4f7cb61bac 100644 --- a/chrome/content/zotero/xpcom/attachments.js +++ b/chrome/content/zotero/xpcom/attachments.js @@ -1342,8 +1342,9 @@ Zotero.Attachments = new function(){ var disabled = Zotero.Notifier.disable(); var item = yield Zotero.Items.getAsync(itemID); - charset = yield Zotero.CharacterSets.add(charset); - item.attachmentCharset = charset; + if (yield Zotero.CharacterSets.add(charset)) { + item.attachmentCharset = charset; + } yield item.saveTx(); if (disabled) { diff --git a/chrome/content/zotero/xpcom/data/cachedTypes.js b/chrome/content/zotero/xpcom/data/cachedTypes.js index be3e043819..47568048de 100644 --- a/chrome/content/zotero/xpcom/data/cachedTypes.js +++ b/chrome/content/zotero/xpcom/data/cachedTypes.js @@ -35,12 +35,18 @@ * * And the following properties: * - * this._typeDesc = 'c'; + * this._typeDesc = ''; + * this._typeDescPlural = ''; * this._idCol = ''; * this._nameCol = ''; * this._table = ''; - * this._ignoreCase = false; - * + * + * Optional properties: + * + * this._allowAdd: Allow new types to be added via .add(name) + * this._ignoreCase: Ignore case when looking for types, and add new types as lowercase + * + * And add .init() to zotero.js */ Zotero.CachedTypes = function() { this._types = null; @@ -52,12 +58,10 @@ Zotero.CachedTypes = function() { this._idCol = ''; this._nameCol = ''; this._table = ''; + this._allowAdd = false; this._ignoreCase = false; this._hasCustom = false; - this.getName = getName; - this.getID = getID; - this.init = Zotero.Promise.coroutine(function* () { this._types = {}; @@ -70,10 +74,10 @@ Zotero.CachedTypes = function() { }); - function getName(idOrName) { + this.getName = function (idOrName) { if (!this._types) { throw new Zotero.Exception.UnloadedDataException( - this._typeDesc[0].toUpperCase() + this._typeDesc.substr(1) + " data not yet loaded" + Zotero.Utilities.capitalize(this._typeDesc) + " data not yet loaded" ); } @@ -83,8 +87,7 @@ Zotero.CachedTypes = function() { } if (!this._types['_' + idOrName]) { - Zotero.debug('Invalid ' + this._typeDesc + ' ' + idOrName, 1); - Zotero.debug((new Error()).stack, 1); + Zotero.debug('Unknown ' + this._typeDesc + ' ' + idOrName, 1); return ''; } @@ -92,10 +95,10 @@ Zotero.CachedTypes = function() { } - function getID(idOrName) { + this.getID = function (idOrName) { if (!this._types) { throw new Zotero.Exception.UnloadedDataException( - this._typeDesc[0].toUpperCase() + this._typeDesc.substr(1) + " data not yet loaded" + Zotero.Utilities.capitalize(this._typeDesc) + " data not yet loaded" ); } @@ -105,7 +108,7 @@ Zotero.CachedTypes = function() { } if (!this._types['_' + idOrName]) { - Zotero.debug('Invalid ' + this._typeDesc + ' ' + idOrName, 1); + Zotero.debug('Unknown ' + this._typeDesc + ' ' + idOrName, 1); return false; } @@ -113,10 +116,10 @@ Zotero.CachedTypes = function() { } - this.getTypes = function () { + this.getAll = this.getTypes = function () { if (!this._typesArray) { throw new Zotero.Exception.UnloadedDataException( - this._typeDesc[0].toUpperCase() + this._typeDesc.substr(1) + " data not yet loaded" + Zotero.Utilities.capitalize(this._typeDesc) + " data not yet loaded" ); } return this._typesArray; @@ -129,6 +132,55 @@ Zotero.CachedTypes = function() { } + /** + * Add a new type to the data and return its id. If the type already exists, return its id. + * + * @param {String} name - Type name to add + * @return {Integer|False} - The type id (new or existing), or false if invalid type name + */ + this.add = Zotero.Promise.coroutine(function* (name) { + if (!this._allowAdd) { + throw new Error("New " + this._typeDescPlural + " cannot be added"); + } + + if (typeof name != 'string' || name === "") { + throw new Error("'name' must be a string"); + } + + var id = this.getID(name); + if (id) { + return id; + } + + if (this._ignoreCase) { + name = name.toLowerCase(); + } + + var allow = this._valueCheck(name); + if (!allow) { + return false; + } + + var sql = "INSERT INTO " + this._table + " (" + this._nameCol + ") VALUES (?)"; + yield Zotero.DB.queryAsync(sql, name); + + sql = "SELECT " + this._idCol + " FROM " + this._table + " WHERE " + this._nameCol + "=?"; + var id = yield Zotero.DB.valueQueryAsync(sql, name); + + this._cacheTypeData({ + id: id, + name: name + }); + + return id; + }); + + + this._valueCheck = function (name) { + return true; + } + + /** * @return {Promise} */ @@ -171,6 +223,7 @@ Zotero.CreatorTypes = new function() { this.isValidForItemType = isValidForItemType; this._typeDesc = 'creator type'; + this._typeDescPlural = 'creator types'; this._idCol = 'creatorTypeID'; this._nameCol = 'creatorType'; this._table = 'creatorTypes'; @@ -279,11 +332,10 @@ Zotero.ItemTypes = new function() { Zotero.CachedTypes.apply(this, arguments); this.constructor.prototype = new Zotero.CachedTypes(); - this.getImageSrc = getImageSrc; - this.customIDOffset = 10000; this._typeDesc = 'item type'; + this._typeDescPlural = 'item types'; this._idCol = 'itemTypeID'; this._nameCol = 'typeName'; this._table = 'itemTypesCombined'; @@ -388,7 +440,7 @@ Zotero.ItemTypes = new function() { return Zotero.getString("itemTypes." + typeName); } - function getImageSrc(itemType) { + this.getImageSrc = function (itemType) { var suffix = Zotero.hiDPI ? "@2x" : ""; if (this.isCustom(itemType)) { @@ -460,6 +512,7 @@ Zotero.FileTypes = new function() { this.constructor.prototype = new Zotero.CachedTypes(); this._typeDesc = 'file type'; + this._typeDescPlural = 'file types'; this._idCol = 'fileTypeID'; this._nameCol = 'fileType'; this._table = 'fileTypes'; @@ -480,52 +533,21 @@ Zotero.CharacterSets = new function() { this.constructor.prototype = new Zotero.CachedTypes(); this._typeDesc = 'character set'; + this._typeDescPlural = 'character sets'; this._idCol = 'charsetID'; this._nameCol = 'charset'; this._table = 'charsets'; this._ignoreCase = true; - - this.getAll = getAll; - - function getAll() { - return this.getTypes(); - } + this._allowAdd = true; - /** - * @param {String} name - Type name to add - * @return {Integer|False} - The type id (new or existing), or false if invalid type name - */ - this.add = Zotero.Promise.coroutine(function* (name) { - if (typeof name != 'string' || name === "") { - return false; - } - - var id = this.getID(name); - if (id) { - return id; - } - - name = name.toLowerCase(); - + this._valueCheck = function (name) { // Don't allow too-long or non-ASCII names if (name.length > 50 || !name.match(/^[a-z0-9\-_]+$/)) { return false; } - - var sql = "INSERT INTO " + this._table + " (" + this._nameCol + ") VALUES (?)"; - yield Zotero.DB.queryAsync(sql, name); - - sql = "SELECT " + this._idCol + " FROM " + this._table + " WHERE " + this._nameCol + "=?"; - var id = yield Zotero.DB.valueQueryAsync(sql, name); - - this._cacheTypeData({ - id: id, - name: name - }); - - return id; - }); + return true; + } /** diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 02a60a6925..d18c841fbb 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -1463,7 +1463,9 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) { + "contentType, charsetID, path, syncState) VALUES (?,?,?,?,?,?,?)"; let linkMode = this.attachmentLinkMode; let contentType = this.attachmentContentType; - let charsetID = yield Zotero.CharacterSets.add(this.attachmentCharset); + let charsetID = this.attachmentCharset + ? (yield Zotero.CharacterSets.add(this.attachmentCharset)) + : null; let path = this.attachmentPath; let syncState = this.attachmentSyncState; @@ -2732,11 +2734,9 @@ Zotero.defineProperty(Zotero.Item.prototype, 'attachmentCharset', { } if (typeof val == 'number') { - oldVal = Zotero.CharacterSets.getID(this.attachmentCharset); - } - else { - oldVal = this.attachmentCharset; + throw new Error("Character set must be a string"); } + oldVal = this.attachmentCharset; if (!val) { val = ""; diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index bee454cc9d..a2af312742 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -423,10 +423,24 @@ describe("Zotero.Item", function () { item.attachmentLinkMode = Zotero.Attachments.LINK_MODE_IMPORTED_FILE; item.attachmentCharset = charset; var itemID = yield item.saveTx(); + assert.equal(item.attachmentCharset, charset); item = yield Zotero.Items.getAsync(itemID); assert.equal(item.attachmentCharset, charset); }) + it("should not allow a numerical value", function* () { + var charset = 1; + var item = new Zotero.Item("attachment"); + try { + item.attachmentCharset = charset; + } + catch (e) { + assert.equal(e.message, "Character set must be a string") + return; + } + assert.fail("Numerical charset was allowed"); + }) + it("should not be marked as changed if not changed", function* () { var charset = 'utf-8'; var item = new Zotero.Item("attachment");