commit 4b040c78a7759f6615a08adc8c66e09bda64ffdf
parent d9c32a8e90a1ae616f9034badd4efee3767460ed
Author: Dan Stillman <dstillman@zotero.org>
Date: Fri, 17 Apr 2015 19:27:37 -0400
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.)
Diffstat:
6 files changed, 148 insertions(+), 88 deletions(-)
diff --git 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
@@ -36,6 +36,8 @@
</resources>
<implementation>
+ <property name="groups"/>
+
<field name="searchRef"/>
<property name="search" onget="return this.searchRef;">
<setter>
@@ -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<groups.length; i++) {
- let group = groups[i];
+ for (let i = 0; i < this.groups.length; i++) {
+ let group = this.groups[i];
let menuitem = document.createElement('menuitem');
menuitem.setAttribute('label', group.name);
menuitem.setAttribute('libraryID', group.libraryID);
@@ -232,7 +230,7 @@
<body>
<![CDATA[
this.updateSearch();
- return this.search.save({fixGaps: true});
+ return this.search.save();
]]>
</body>
</method>
diff --git 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
@@ -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<conditions.length; i++) {
+ let condition = conditions[i];
+
// Parse "condition[/mode]"
- var [condition, mode] =
- Zotero.SearchConditions.parseCondition(conditions[i]['condition']);
+ let [conditionName, mode] = Zotero.SearchConditions.parseCondition(condition.condition);
- var cond = Zotero.SearchConditions.get(condition);
+ let cond = Zotero.SearchConditions.get(conditionName);
if (!cond || cond.noLoad) {
- Zotero.debug("Invalid saved search condition '" + condition + "' -- skipping", 2);
+ Zotero.debug("Invalid saved search condition '" + conditionName + "' -- skipping", 2);
continue;
}
// Convert itemTypeID to itemType
//
// TEMP: This can be removed at some point
- if (condition == 'itemTypeID') {
- condition = 'itemType';
- conditions[i].value = Zotero.ItemTypes.getName(conditions[i].value);
+ if (conditionName == 'itemTypeID') {
+ conditionName = 'itemType';
+ condition.value = Zotero.ItemTypes.getName(condition.value);
}
- this._conditions[conditionID] = {
- id: conditionID,
- condition: condition,
+ this._conditions[i] = {
+ id: i,
+ condition: conditionName,
mode: mode,
- operator: conditions[i].operator,
- value: conditions[i].value,
- required: conditions[i].required
+ operator: condition.operator,
+ value: condition.value,
+ required: !!condition.required
};
-
- conditionID++;
}
this._loaded.conditions = true;
diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js
@@ -1809,7 +1809,7 @@ var ZoteroPane = new function()
});
- this.editSelectedCollection = function () {
+ this.editSelectedCollection = Zotero.Promise.coroutine(function* () {
if (!this.canEdit()) {
this.displayCannotEditLibraryMessage();
return;
@@ -1832,22 +1832,32 @@ var ZoteroPane = new function()
}
}
else {
- var s = new Zotero.Search();
+ let s = new Zotero.Search();
s.id = row.ref.id;
- s.loadPrimaryData()
- .then(function () {
- return s.loadConditions();
- })
- .then(function () {
- var io = {dataIn: {search: s, name: row.getName()}, dataOut: null};
- window.openDialog('chrome://zotero/content/searchDialog.xul','','chrome,modal',io);
- if (io.dataOut) {
- this.onCollectionSelected(); //reload itemsView
- }
- }.bind(this));
+ yield s.loadPrimaryData();
+ yield s.loadConditions();
+ let groups = [];
+ // Promises don't work in the modal dialog, so get the group name here, if
+ // applicable, and pass it in. We only need the group that this search belongs
+ // to, if any, since the library drop-down is disabled for saved searches.
+ if (Zotero.Libraries.getType(s.libraryID) == 'group') {
+ groups.push(yield Zotero.Groups.getByLibraryID(s.libraryID));
+ }
+ var io = {
+ dataIn: {
+ search: s,
+ name: row.getName(),
+ groups: groups
+ },
+ dataOut: null
+ };
+ window.openDialog('chrome://zotero/content/searchDialog.xul','','chrome,modal',io);
+ if (io.dataOut) {
+ this.onCollectionSelected(); //reload itemsView
+ }
}
}
- }
+ });
this.copySelectedItemsToClipboard = function (asCitations) {
diff --git a/test/tests/searchTests.js b/test/tests/searchTests.js
@@ -37,11 +37,61 @@ describe("Zotero.Search", function() {
yield s.loadConditions();
var conditions = s.getConditions();
assert.lengthOf(Object.keys(conditions), 1);
- assert.property(conditions, "1"); // searchConditionIDs start at 1
- var condition = conditions[1];
+ assert.property(conditions, "0");
+ var condition = conditions[0];
assert.propertyVal(condition, 'condition', 'title')
assert.propertyVal(condition, 'operator', 'is')
assert.propertyVal(condition, 'value', 'test')
+ assert.propertyVal(condition, 'required', false)
+ });
+
+ it("should add a condition to an existing search", function* () {
+ // Save search
+ var s = new Zotero.Search;
+ s.libraryID = Zotero.Libraries.userLibraryID;
+ s.name = "Test";
+ s.addCondition('title', 'is', 'test');
+ var id = yield s.save();
+ assert.typeOf(id, 'number');
+
+ // Add condition
+ s = yield Zotero.Searches.getAsync(id);
+ yield s.loadConditions();
+ s.addCondition('title', 'contains', 'foo');
+ var saved = yield s.save();
+ assert.isTrue(saved);
+
+ // Check saved search
+ s = yield Zotero.Searches.getAsync(id);
+ yield s.loadConditions();
+ var conditions = s.getConditions();
+ assert.lengthOf(Object.keys(conditions), 2);
+ });
+
+ it("should remove a condition from an existing search", function* () {
+ // Save search
+ var s = new Zotero.Search;
+ s.libraryID = Zotero.Libraries.userLibraryID;
+ s.name = "Test";
+ s.addCondition('title', 'is', 'test');
+ s.addCondition('title', 'contains', 'foo');
+ var id = yield s.save();
+ assert.typeOf(id, 'number');
+
+ // Remove condition
+ s = yield Zotero.Searches.getAsync(id);
+ yield s.loadConditions();
+ s.removeCondition(0);
+ var saved = yield s.save();
+ assert.isTrue(saved);
+
+ // Check saved search
+ s = yield Zotero.Searches.getAsync(id);
+ yield s.loadConditions();
+ var conditions = s.getConditions();
+ assert.lengthOf(Object.keys(conditions), 1);
+ assert.property(conditions, "0");
+ assert.propertyVal(conditions[0], 'value', 'foo')
});
});
});
