From 34d66381d1200b0ffaadeeb6e402f34902f8360d Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 11 Jul 2022 00:42:06 -0400 Subject: [PATCH] Revert "Validate fields in ItemTree::getSortFields()" This reverts commit a8ed30ce803a9c5b3b5c04d50f6bd6b94faeee86 and related commits. We'll address breakage from invalid sort fields another way, without inconveniencing plugin authors. https://groups.google.com/g/zotero-dev/c/kc0-C6-SA74/m/bhHniGceAQAJ --- chrome/content/zotero/itemTree.jsx | 54 +++++------------------------- test/tests/itemTreeTest.js | 48 +------------------------- 2 files changed, 9 insertions(+), 93 deletions(-) diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx index fb7f52c385..a2756c3583 100644 --- a/chrome/content/zotero/itemTree.jsx +++ b/chrome/content/zotero/itemTree.jsx @@ -1820,41 +1820,24 @@ var ItemTree = class ItemTree extends LibraryTree { getSortFields() { var fields = [this.getSortField()]; - if (!this._isValidSortField(fields[0])) { - Zotero.logError(`'${fields[0]}' is not a valid sort field -- skipping`); - fields.shift(); - } var secondaryField = this._getSecondarySortField(); if (secondaryField) { fields.push(secondaryField); } - var fallbackFields; try { - fallbackFields = Zotero.Prefs.get('fallbackSort') + var fallbackFields = Zotero.Prefs.get('fallbackSort') .split(',') - .map(x => x.trim()) - .filter(x => x !== ''); - for (let field of fallbackFields) { - if (!this._isValidSortField(field)) { - // eslint-disable-next-line no-throw-literal - throw `'${field}' is not a valid field in fallbackSort -- resetting`; - } - } + .map((x) => x.trim()) + .filter((x) => x !== ''); } catch (e) { + Zotero.debug(e, 1); Zotero.logError(e); - Zotero.Prefs.clear('fallbackSort'); - fallbackFields = Zotero.Prefs.get('fallbackSort').split(','); + // This should match the default value for the fallbackSort pref + var fallbackFields = ['firstCreator', 'date', 'title', 'dateAdded']; } fields = Zotero.Utilities.arrayUnique(fields.concat(fallbackFields)); - // If no valid fields, use default fallback - if (!fields.length) { - Zotero.logError(`No valid fields in getSortFields() (${fields.join(',')}) ` - + '-- resetting'); - Zotero.Prefs.clear('fallbackSort'); - fields = Zotero.Prefs.get('fallbackSort').split(','); - } - + // If date appears after year, remove it, unless it's the explicit secondary sort var yearPos = fields.indexOf('year'); if (yearPos != -1) { @@ -1866,21 +1849,7 @@ var ItemTree = class ItemTree extends LibraryTree { return fields; } - - _isValidSortField(field) { - switch (field) { - // Non-standard columns - case 'itemType': - case 'year': - case 'hasAttachment': - case 'numNotes': - // Feed items - case 'id': - return true; - } - return !!Zotero.ItemFields.getID(field) || Zotero.Items.primaryFields.includes(field); - } - + /** * @param index {Integer} * @param selectAll {Boolean} Whether the selection is part of a select-all event @@ -3753,13 +3722,6 @@ var ItemTree = class ItemTree extends LibraryTree { if (!secondaryField || secondaryField == primaryField) { return false; } - // Make sure specified field is valid - if (!this._isValidSortField(secondaryField)) { - Zotero.logError(`'${secondaryField}' is not a valid field in ` - + `secondarySort.${primaryField} -- resetting`); - Zotero.Prefs.clear('secondarySort.' + primaryField); - return false; - } return secondaryField; } diff --git a/test/tests/itemTreeTest.js b/test/tests/itemTreeTest.js index 2215b0b358..9e14aae882 100644 --- a/test/tests/itemTreeTest.js +++ b/test/tests/itemTreeTest.js @@ -151,49 +151,6 @@ describe("Zotero.ItemTree", function() { }) }) - describe("#getSortFields()", function () { - beforeEach(async function () { - itemsView = zp.itemsView; - - // Sort by title - const colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'title'); - await itemsView.tree._columns.toggleSort(colIndex); - }); - - after(function () { - Zotero.Prefs.clear('fallbackSort'); - }); - - it("should ignore invalid secondary-sort field", function () { - Zotero.Prefs.set('secondarySort.title', 'invalidField'); - var fields = itemsView.getSortFields(); - assert.isUndefined(Zotero.Prefs.get('secondarySort.title')); - // fallbackSort pref with title moved to beginning - assert.sameMembers(['title', 'firstCreator', 'date', 'dateAdded'], fields); - - Zotero.Prefs.clear('secondarySort.title'); - }); - - it("should ignore invalid fallback-sort field", function () { - Zotero.Prefs.clear('fallbackSort'); - var originalFallback = Zotero.Prefs.get('fallbackSort'); - Zotero.Prefs.set('fallbackSort', 'invalidField,' + originalFallback); - var fields = itemsView.getSortFields(); - assert.equal(Zotero.Prefs.get('fallbackSort'), originalFallback); - // fallbackSort pref with title moved to beginning - assert.sameMembers(['title', 'firstCreator', 'date', 'dateAdded'], fields); - }); - - it("should sort by item type", async function () { - // Sort by item type - const colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'itemType'); - await itemsView.tree._columns.toggleSort(colIndex); - - var fields = itemsView.getSortFields(); - assert.sameMembers(['itemType', 'firstCreator', 'date', 'title', 'dateAdded'], fields); - }); - }); - describe("#notify()", function () { beforeEach(function () { sinon.spy(win.ZoteroPane, "itemSelected"); @@ -625,10 +582,7 @@ describe("Zotero.ItemTree", function() { var item3 = await createDataObject('item', { title: title + " 5" }); var item4 = await createDataObject('item', { title: title + " 7" }); - // Sort by creator and then title, to make sure we're sorting by title ascending - var colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'firstCreator'); - await itemsView.tree._columns.toggleSort(colIndex); - colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'title'); + const colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'title'); await itemsView.tree._columns.toggleSort(colIndex); // Check initial sort order