From 4b040c78a7759f6615a08adc8c66e09bda64ffdf Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 17 Apr 2015 19:27:37 -0400 Subject: [PATCH] Fix various saved search bugs, and add tests Search condition ids are now indexed from 0, and always saved contiguously (no more 'fixGaps' option), since they're just in an array in the API. (They're still returned as an object from Zotero.Search.prototype.getConditions() because it's easier for the advanced search window to not have to deal with shifting ids between saves.) --- chrome/content/zotero/advancedSearch.js | 6 ++ .../content/zotero/bindings/zoterosearch.xml | 34 +++--- chrome/content/zotero/searchDialog.js | 4 +- chrome/content/zotero/xpcom/search.js | 102 +++++++++--------- chrome/content/zotero/zoteroPane.js | 38 ++++--- test/tests/searchTests.js | 54 +++++++++- 6 files changed, 149 insertions(+), 89 deletions(-) diff --git a/chrome/content/zotero/advancedSearch.js b/chrome/content/zotero/advancedSearch.js index 2f2e2c9173..c549812397 100644 --- a/chrome/content/zotero/advancedSearch.js +++ b/chrome/content/zotero/advancedSearch.js @@ -48,6 +48,12 @@ var ZoteroAdvancedSearch = new function() { io.dataIn.search.loadPrimaryData() .then(function () { + return Zotero.Groups.getAll(); + }) + .then(function (groups) { + // Since the search box can be used as a modal dialog, which can't use promises, + // it expects groups to be passed in. + _searchBox.groups = groups; _searchBox.search = io.dataIn.search; }); } diff --git a/chrome/content/zotero/bindings/zoterosearch.xml b/chrome/content/zotero/bindings/zoterosearch.xml index 137d7c6ec8..8203bbbe11 100644 --- a/chrome/content/zotero/bindings/zoterosearch.xml +++ b/chrome/content/zotero/bindings/zoterosearch.xml @@ -36,6 +36,8 @@ + + @@ -49,28 +51,25 @@ conditionsBox.removeChild(conditionsBox.firstChild); var conditions = this.search.getConditions(); - - for(var id in conditions) - { + for (let id in conditions) { + let condition = conditions[id]; // Checkboxes - switch (conditions[id]['condition']) { + switch (condition.condition) { case 'recursive': case 'noChildren': case 'includeParentsAndChildren': - var checkbox = conditions[id]['condition'] + 'Checkbox'; - this.id(checkbox).setAttribute('condition',id); - this.id(checkbox).checked = (conditions[id]['operator']=='true'); + let checkbox = condition.condition + 'Checkbox'; + this.id(checkbox).setAttribute('condition', id); + this.id(checkbox).checked = condition.operator == 'true'; continue; } - if(conditions[id]['condition'] == 'joinMode') - { - this.id('joinModeMenu').setAttribute('condition',id); - this.id('joinModeMenu').value = conditions[id]['operator']; + if(condition.condition == 'joinMode') { + this.id('joinModeMenu').setAttribute('condition', id); + this.id('joinModeMenu').value = condition.operator; } - else - { - this.addCondition(conditions[id]); + else { + this.addCondition(condition); } } ]]> @@ -96,9 +95,8 @@ menupopup.appendChild(menuitem); // Add groups - var groups = Zotero.Groups.getAll(); - for (let i=0; i diff --git a/chrome/content/zotero/searchDialog.js b/chrome/content/zotero/searchDialog.js index 8632c026dc..5b946a027e 100644 --- a/chrome/content/zotero/searchDialog.js +++ b/chrome/content/zotero/searchDialog.js @@ -35,7 +35,9 @@ function doLoad() io = window.arguments[0]; - document.getElementById('search-box').search = io.dataIn.search; + var searchBox = document.getElementById('search-box'); + searchBox.groups = io.dataIn.groups; + searchBox.search = io.dataIn.search; document.getElementById('search-name').value = io.dataIn.name; } diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js index b5ff008c9d..9fd531fe79 100644 --- a/chrome/content/zotero/xpcom/search.js +++ b/chrome/content/zotero/xpcom/search.js @@ -32,8 +32,8 @@ Zotero.Search = function() { this._scopeIncludeChildren = null; this._sql = null; this._sqlParams = false; - this._maxSearchConditionID = 0; - this._conditions = []; + this._maxSearchConditionID = -1; + this._conditions = {}; this._hasPrimaryConditions = false; } @@ -179,7 +179,6 @@ Zotero.Search.prototype._initSave = Zotero.Promise.coroutine(function* (env) { }); Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) { - var fixGaps = env.options.fixGaps; var isNew = env.isNew; var searchID = env.id = this._id = this.id ? this.id : yield Zotero.ID.get('savedSearches'); @@ -217,39 +216,28 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) { yield Zotero.DB.queryAsync(sql, this.id); } - // Close gaps in savedSearchIDs - var saveConditions = {}; - var i = 1; - for (var id in this._conditions) { - if (!fixGaps && id != i) { - Zotero.DB.rollbackTransaction(); - throw ('searchConditionIDs not contiguous and |fixGaps| not set in save() of saved search ' + this._id); - } - saveConditions[i] = this._conditions[id]; - i++; - } - - this._conditions = saveConditions; - - for (var i in this._conditions){ - var sql = "INSERT INTO savedSearchConditions (savedSearchID, " - + "searchConditionID, condition, operator, value, required) " - + "VALUES (?,?,?,?,?,?)"; + var i = 0; + var sql = "INSERT INTO savedSearchConditions " + + "(savedSearchID, searchConditionID, condition, operator, value, required) " + + "VALUES (?,?,?,?,?,?)"; + for (let id in this._conditions) { + let condition = this._conditions[id]; // Convert condition and mode to "condition[/mode]" - var condition = this._conditions[i].mode ? - this._conditions[i].condition + '/' + this._conditions[i].mode : - this._conditions[i].condition + let conditionString = condition.mode ? + condition.condition + '/' + condition.mode : + condition.condition var sqlParams = [ searchID, i, - condition, - this._conditions[i].operator ? this._conditions[i].operator : null, - this._conditions[i].value ? this._conditions[i].value : null, - this._conditions[i].required ? 1 : null + conditionString, + condition.operator ? condition.operator : null, + condition.value ? condition.value : null, + condition.required ? 1 : null ]; yield Zotero.DB.queryAsync(sql, sqlParams); + i++; } }); @@ -274,6 +262,7 @@ Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) return id; } + yield this.loadPrimaryData(true); yield this.reload(); this._clearChanged(); @@ -400,11 +389,13 @@ Zotero.Search.prototype.addCondition = function (condition, operator, value, req mode: mode, operator: operator, value: value, - required: required + required: !!required }; this._sql = null; this._sqlParams = false; + this._markFieldChange('conditions', this._conditions); + this._changed.conditions = true; return searchConditionID; } @@ -426,8 +417,8 @@ Zotero.Search.prototype.setScope = function (searchObj, includeChildren) { * @param {Boolean} [required] * @return {Promise} */ -Zotero.Search.prototype.updateCondition = Zotero.Promise.coroutine(function* (searchConditionID, condition, operator, value, required){ - yield this.loadPrimaryData(); +Zotero.Search.prototype.updateCondition = function (searchConditionID, condition, operator, value, required) { + this._requireData('conditions'); if (typeof this._conditions[searchConditionID] == 'undefined'){ throw new Error('Invalid searchConditionID ' + searchConditionID); @@ -447,23 +438,27 @@ Zotero.Search.prototype.updateCondition = Zotero.Promise.coroutine(function* (se mode: mode, operator: operator, value: value, - required: required + required: !!required }; this._sql = null; this._sqlParams = false; -}); + this._markFieldChange('conditions', this._conditions); + this._changed.conditions = true; +} -Zotero.Search.prototype.removeCondition = Zotero.Promise.coroutine(function* (searchConditionID){ - yield this.loadPrimaryData(); +Zotero.Search.prototype.removeCondition = function (searchConditionID) { + this._requireData('conditions'); if (typeof this._conditions[searchConditionID] == 'undefined'){ throw ('Invalid searchConditionID ' + searchConditionID + ' in removeCondition()'); } delete this._conditions[searchConditionID]; -}); + this._markFieldChange('conditions', this._conditions); + this._changed.conditions = true; +} /* @@ -896,38 +891,37 @@ Zotero.Search.prototype.loadConditions = Zotero.Promise.coroutine(function* (rel this._maxSearchConditionID = conditions[conditions.length - 1].searchConditionID; } - // Reindex conditions, in case they're not contiguous in the DB - var conditionID = 1; + this._conditions = {}; + // Reindex conditions, in case they're not contiguous in the DB for (let i=0; i