new annotation search conditions

- added annotationType, annotationColor, and annotationAuthor
search conditions
- updated logic in search.js to only include annotation
on negative operators. Otherwise, 'annotationTitle' 'isNot' 'X'
would always match every attachment and top-level item
in the library

Fixes: zotero#5837

chrome/content/zotero/elements/zoteroSearch.js

@@ -374,7 +374,7 @@
 			return false;
 		}
 
-		onConditionSelected(conditionName, reload) {
+		async onConditionSelected(conditionName, reload) {
 			var conditionsMenu = this.querySelector('#conditionsmenu');
 			var operatorsList = this.querySelector('#operatorsmenu');
 			
@@ -490,6 +490,44 @@
 					this.createValueMenu(rows);
 					break;
 				}
+				case 'annotationColor':
+				{
+					let rows = [
+						{ name: Zotero.getString('general.yellow'), value: '#ffd400' },
+						{ name: Zotero.getString('general.red'), value: '#ff6666' },
+						{ name: Zotero.getString('general.green'), value: '#5fb236' },
+						{ name: Zotero.getString('general.blue'), value: '#2ea8e5' },
+						{ name: Zotero.getString('general.purple'), value: '#a28ae5' },
+						{ name: Zotero.getString('general.magenta'), value: '#e56eee' },
+						{ name: Zotero.getString('general.orange'), value: '#f19837' },
+						{ name: Zotero.getString('general.gray'), value: '#aaaaaa' },
+					];
+					this.createValueMenu(rows);
+					break;
+				}
+				case 'annotationType':
+				{
+					let rows = [
+						{ name: 'Highlight', value: Zotero.Annotations.ANNOTATION_TYPE_HIGHLIGHT },
+						{ name: 'Underline', value: Zotero.Annotations.ANNOTATION_TYPE_UNDERLINE },
+						{ name: 'Text', value: Zotero.Annotations.ANNOTATION_TYPE_TEXT },
+						{ name: 'Note', value: Zotero.Annotations.ANNOTATION_TYPE_NOTE },
+						{ name: 'Ink', value: Zotero.Annotations.ANNOTATION_TYPE_INK },
+						{ name: 'Image', value: Zotero.Annotations.ANNOTATION_TYPE_IMAGE },
+					];
+					this.createValueMenu(rows);
+					break;
+				}
+				case 'annotationAuthor':
+				{
+					let libraryID = this.parent.search.libraryID;
+					let authors = await Zotero.Annotations.getAllAuthors(libraryID);
+					let collation = Zotero.getLocaleCollation();
+					let rows = authors.map(a => ({ name: a.name, value: a.userID }));
+					rows.sort((a, b) => collation.compareString(1, a.name, b.name));
+					this.createValueMenu(rows);
+					break;
+				}
 				default:
 				{
 					if (operatorsList.value == 'isInTheLast') {
@@ -522,7 +560,10 @@
 			// Drop-down menu
 			if (this.selectedCondition == 'collection'
 					|| this.selectedCondition == 'itemType'
-					|| this.selectedCondition == 'fileTypeID') {
+					|| this.selectedCondition == 'fileTypeID'
+					|| this.selectedCondition == 'annotationColor'
+					|| this.selectedCondition == 'annotationType'
+					|| this.selectedCondition == 'annotationAuthor') {
 				this.querySelector('#valuefield').hidden = true;
 				this.querySelector('#valuemenu').hidden = false;
 				this.querySelector('#value-date-age').hidden = true;
@@ -731,6 +772,7 @@
 		onLibraryChange() {
 			switch (this.selectedCondition) {
 				case 'collection':
+				case 'annotationAuthor':
 					this.onConditionSelected(this.selectedCondition, true);
 					break;
 			}

chrome/content/zotero/xpcom/annotations.js

@@ -334,6 +334,22 @@ Zotero.Annotations = new function () {
 	 * @param {Zotero.Item[]} items
 	 * @returns {Promise<void>}
 	 */
+	/**
+	 * Return all annotation authors in a library
+	 *
+	 * @param {Integer} libraryID
+	 * @return {Object[]} - Array of { userID, name }
+	 */
+	this.getAllAuthors = async function (libraryID) {
+		let sql = `SELECT DISTINCT u.userID, u.name FROM users u
+			JOIN groupItems gi ON (gi.createdByUserID = u.userID)
+			JOIN itemAnnotations ia ON (ia.itemID = gi.itemID)
+			JOIN items i ON (i.itemID = ia.itemID)
+			WHERE i.libraryID = ?`;
+		let rows = await Zotero.DB.queryAsync(sql, libraryID);
+		return rows.map(row => ({ userID: row.userID, name: row.name }));
+	};
+
 	this.splitAnnotations = async function (items) {
 		if (!Array.isArray(items)) {
 			items = [items];

chrome/content/zotero/xpcom/data/search.js

@@ -1207,7 +1207,8 @@ Zotero.Search.prototype._buildQuery = async function () {
 				//
 				if (condition.table) {
 					let negationOperators = ['isNot', 'doesNotContain'];
-					let isNegationOperator = negationOperators.includes(condition.operator);
+					let isNegationOperator = negationOperators.includes(condition.operator);			
+					let isAnnotationCondition = condition.name.includes('annotation');
 					
 					condSelectSQL += 'itemID '
 					if (isNegationOperator) {
@@ -1215,23 +1216,23 @@ Zotero.Search.prototype._buildQuery = async function () {
 					}
 					condSelectSQL += 'IN (';
 					selectOpenParens = 1;
-					
-					// TEMP: Don't match annotations for negation operators, since it would result in
-					// all parent attachments being returned
+
+					// For negative operators, add all items of the "wrong" type to the
+					// NOT IN set so they get excluded from results:
+					// - Annotation conditions: exclude non-annotations (so only
+					//   annotations can match)
+					// - Non-annotation conditions: exclude annotations (so annotations
+					//   don't trivially match all negative queries)
 					if (isNegationOperator) {
-						condSelectSQL += "SELECT itemID FROM items WHERE itemTypeID="
+						condSelectSQL += "SELECT itemID FROM items WHERE itemTypeID"
+							+ (isAnnotationCondition ? "!=" : "=")
 							+ Zotero.ItemTypes.getID('annotation') + " UNION ";
 					}
 					
 					switch (condition.name) {
-						case 'tag':
-							condSQL += "SELECT itemID FROM itemTags "
-								+ "LEFT JOIN itemAnnotations IAnT USING (itemID) WHERE (";
-							break;
-						
-						case 'annotationText':
-						case 'annotationComment':
-							condSQL += `SELECT itemID FROM ${condition.table} WHERE (`
+						case 'annotationAuthor':
+							condSQL += "SELECT itemID FROM itemAnnotations "
+								+ "LEFT JOIN groupItems USING (itemID) WHERE (";
 							break;
 							
 						default:

chrome/content/zotero/xpcom/data/searchConditions.js

@@ -599,6 +599,39 @@ Zotero.SearchConditions = new function () {
 				special: false,
 			},
 			
+			{
+				name: 'annotationColor',
+				operators: {
+					is: true,
+					isNot: true
+				},
+				table: 'itemAnnotations',
+				field: 'color',
+				special: false,
+			},
+
+			{
+				name: 'annotationType',
+				operators: {
+					is: true,
+					isNot: true
+				},
+				table: 'itemAnnotations',
+				field: 'type',
+				special: false,
+			},
+
+			{
+				name: 'annotationAuthor',
+				operators: {
+					is: true,
+					isNot: true,
+				},
+				table: 'groupItems',
+				field: 'createdByUserID',
+				special: false,
+			},
+			
 			{
 				name: 'fulltextWord',
 				operators: {

chrome/locale/en-US/zotero/zotero.ftl

@@ -830,6 +830,9 @@ search-conditions-lastRead = Attachment Last Read
 search-conditions-annotationText = Annotation Text
 search-conditions-annotationComment = Annotation Comment
 search-conditions-anyField = Any Field
+search-conditions-annotationColor = Annotation Color
+search-conditions-annotationAuthor = Annotation Author
+search-conditions-annotationType = Annotation Type
 
 find-pdf-files-added = { $count ->
     [one] { $count } file added

test/content/support.js

@@ -1186,7 +1186,7 @@ async function createAnnotation(type, parentItem, options = {}) {
 	else {
 		annotation.annotationComment = Zotero.Utilities.randomString();
 	}
-	annotation.annotationColor = '#ffd400';
+	annotation.annotationColor = options.annotationColor || '#ffd400';
 	var page = Zotero.Utilities.rand(1, 100);
 	annotation.annotationPageLabel = `${page}`;
 	page = page.toString().padStart(5, '0');

test/tests/advancedSearchTest.js

@@ -225,6 +225,81 @@ describe("Advanced Search", function () {
 			});
 		});
 		
+		describe("Annotation Author", function () {
+			it("should show annotation authors for a group library", async function () {
+				var group = await getGroup();
+				var groupLibraryID = group.libraryID;
+
+				await Zotero.Users.setName(11111, 'Alice');
+				await Zotero.Users.setName(22222, 'Bob');
+
+				var groupAttachment = await importFileAttachment('test.pdf', {
+					libraryID: groupLibraryID,
+					contentType: 'application/pdf',
+				});
+
+				var annotation1 = await createAnnotation('highlight', groupAttachment);
+				annotation1.createdByUserID = 11111;
+				annotation1.lastModifiedByUserID = 11111;
+				await annotation1.saveTx({ skipEditCheck: true });
+
+				var annotation2 = await createAnnotation('highlight', groupAttachment);
+				annotation2.createdByUserID = 22222;
+				annotation2.lastModifiedByUserID = 22222;
+				await annotation2.saveTx({ skipEditCheck: true });
+
+				// Switch to group library
+				var libraryMenu = searchWin.document.getElementById('libraryMenu');
+				for (let i = 0; i < libraryMenu.itemCount; i++) {
+					let menuitem = libraryMenu.getItemAtIndex(i);
+					if (menuitem.value == groupLibraryID) {
+						menuitem.click();
+						break;
+					}
+				}
+
+				var s = new Zotero.Search();
+				s.libraryID = groupLibraryID;
+				s.addCondition('title', 'is', '');
+				searchBox.search = s;
+
+				var searchCondition = conditions.firstChild;
+				var valueMenu = searchCondition.querySelector('#valuemenu');
+
+				// Select 'Annotation Author' condition from the "More" submenu
+				var moreMenu = searchCondition.querySelector('#more-conditions-menu menupopup');
+				for (let menuitem of moreMenu.children) {
+					if (menuitem.value == 'annotationAuthor') {
+						menuitem.click();
+						break;
+					}
+				}
+
+				// Wait for async dropdown population
+				await Zotero.Promise.delay(9000);
+
+				assert.isFalse(valueMenu.hidden);
+				assert.equal(valueMenu.itemCount, 2);
+
+				var values = [];
+				var labels = [];
+				for (let i = 0; i < valueMenu.itemCount; i++) {
+					let menuitem = valueMenu.getItemAtIndex(i);
+					values.push(menuitem.getAttribute('value'));
+					labels.push(menuitem.getAttribute('label'));
+				}
+
+				assert.include(labels, 'Alice');
+				assert.include(labels, 'Bob');
+				assert.include(values, '11111');
+				assert.include(values, '22222');
+
+				await annotation1.eraseTx();
+				await annotation2.eraseTx();
+				await groupAttachment.eraseTx();
+			});
+		});
+
 		describe("Saved Search", function () {
 			it("shouldn't appear", async function () {
 				var searchCondition = conditions.firstChild;

test/tests/searchTest.js

@@ -404,8 +404,128 @@ describe("Zotero.Search", function () {
 					var matches = await s.search();
 					assert.sameMembers(matches, [annotation.id]);
 				});
+
+				it("should not return parent item on 'doesNotContain' condition", async function () {
+					var item = await createDataObject('item');
+					var attachment = await importPDFAttachment(item);
+					var annotation = await createAnnotation('highlight', attachment, { annotationComment: "some comment" });
+
+					var s = new Zotero.Search();
+					s.libraryID = userLibraryID;
+					s.addCondition('annotationText', 'doesNotContain', 'nonexistent');
+					var matches = await s.search();
+					assert.include(matches, annotation.id);
+					assert.notInclude(matches, item.id);
+					assert.notInclude(matches, attachment.id);
+				});
 			});
-			
+
+			describe("title", function () {
+				it("should not return annotations for a 'doesNotContain' condition", async function () {
+					var item = await createDataObject('item', { title: 'Test Title' });
+					var attachment = await importPDFAttachment(item);
+					var annotation = await createAnnotation('highlight', attachment);
+
+					var s = new Zotero.Search();
+					s.libraryID = userLibraryID;
+					s.addCondition('title', 'doesNotContain', 'nonexistent');
+					var matches = await s.search();
+					assert.include(matches, item.id);
+					assert.notInclude(matches, annotation.id);
+				});
+			});
+
+			describe("annotationColor", function () {
+				it("should return annotation matching color", async function () {
+					var attachment = await importPDFAttachment();
+					var annotationOne = await createAnnotation('highlight', attachment, { annotationColor: '#ffd400' });
+					var annotationTwo = await createAnnotation('highlight', attachment, { annotationColor: '#ff6666' });
+
+					var s = new Zotero.Search();
+					s.libraryID = userLibraryID;
+					s.addCondition('annotationColor', 'is', '#ffd400');
+					var matches = await s.search();
+					assert.include(matches, annotationOne.id);
+					assert.notInclude(matches, annotationTwo.id);
+				});
+			});
+
+			describe("annotationType", function () {
+				it("should return annotation matching type", async function () {
+					var attachment = await importPDFAttachment();
+					var annotationOne = await createAnnotation('highlight', attachment);
+					var annotationTwo = await createAnnotation('underline', attachment);
+					
+					var s = new Zotero.Search();
+					s.libraryID = userLibraryID;
+					s.addCondition('annotationType', 'is', Zotero.Annotations.ANNOTATION_TYPE_HIGHLIGHT);
+					var matches = await s.search();
+					assert.include(matches, annotationOne.id);
+					assert.notInclude(matches, annotationTwo.id);
+				});
+			});
+
+			describe("annotationAuthor", function () {
+				it("should return annotation by matching author and not by another", async function () {
+					var group = await getGroup();
+
+					await Zotero.Users.setName(12345, 'User One');
+					await Zotero.Users.setName(67890, 'User Two');
+
+					var groupAttachment = await importFileAttachment('test.pdf', {
+						libraryID: group.libraryID,
+						contentType: 'application/pdf',
+					});
+
+					var annotation1 = await createAnnotation('highlight', groupAttachment);
+					annotation1.createdByUserID = 12345;
+					annotation1.lastModifiedByUserID = 12345;
+					await annotation1.saveTx({ skipEditCheck: true });
+
+					var annotation2 = await createAnnotation('highlight', groupAttachment);
+					annotation2.createdByUserID = 67890;
+					annotation2.lastModifiedByUserID = 67890;
+					await annotation2.saveTx({ skipEditCheck: true });
+
+					var s = new Zotero.Search();
+					s.libraryID = group.libraryID;
+					s.addCondition('annotationAuthor', 'is', 12345);
+					var matches = await s.search();
+					assert.include(matches, annotation1.id);
+					assert.notInclude(matches, annotation2.id);
+				});
+
+				it("should not return annotation by matching author with 'isNot'", async function () {
+					var group = await getGroup();
+
+					await Zotero.Users.setName(12345, 'User One');
+					await Zotero.Users.setName(67890, 'User Two');
+
+					var groupAttachment = await importFileAttachment('test.pdf', {
+						libraryID: group.libraryID,
+						contentType: 'application/pdf',
+					});
+
+					var annotation1 = await createAnnotation('highlight', groupAttachment);
+					annotation1.createdByUserID = 12345;
+					annotation1.lastModifiedByUserID = 12345;
+					await annotation1.saveTx({ skipEditCheck: true });
+
+					var annotation2 = await createAnnotation('highlight', groupAttachment);
+					annotation2.createdByUserID = 67890;
+					annotation2.lastModifiedByUserID = 67890;
+					await annotation2.saveTx({ skipEditCheck: true });
+
+					var s = new Zotero.Search();
+					s.libraryID = group.libraryID;
+					s.addCondition('annotationAuthor', 'isNot', 12345);
+					var matches = await s.search();
+					assert.notInclude(matches, annotation1.id);
+					assert.include(matches, annotation2.id);
+					assert.notInclude(matches, groupAttachment.id);
+				});
+			});
+
 			describe("fulltextWord", function () {
 				it("should return matches with full-text conditions", async function () {
 					let s = new Zotero.Search();