Fix quick search item tree issues when moving attachments into/out of parents

Closes #3561

chrome/content/zotero/itemTree.jsx

@@ -246,6 +246,11 @@ var ItemTree = class ItemTree extends LibraryTree {
 				let row = this._rows[i];
 				// Top-level items
 				if (row.level == 0) {
+					// A top-level attachment moved into a parent. Don't copy, it will be added
+					// via this loop for the parent item.
+					if (row.ref.parentID) {
+						continue;
+					}
 					let isSearchParent = newSearchParentIDs.has(row.ref.id);
 					// If not showing children or no children match the search, close
 					if (this.regularOnly || !isSearchParent) {
@@ -260,6 +265,12 @@ var ItemTree = class ItemTree extends LibraryTree {
 						continue;
 					}
 				}
+				else if (row.level == 1 && !row.ref.parentID) {
+					// A child attachment moved into top-level. It needs to be added anew in a different
+					// location.
+					itemsToAdd.push(row.ref);
+					continue;
+				}
 				// Child items
 				else if (skipChildren) {
 					continue;
@@ -2391,21 +2402,6 @@ var ItemTree = class ItemTree extends LibraryTree {
 					});
 				}
 			}
-
-			// If moving, remove items from source collection
-			if (dropEffect == 'move' && toMove.length) {
-				if (!sameLibrary) {
-					throw new Error("Cannot move items between libraries");
-				}
-				if (!sourceCollectionTreeRow || !sourceCollectionTreeRow.isCollection()) {
-					throw new Error("Drag source must be a collection");
-				}
-				if (collectionTreeRow.id != sourceCollectionTreeRow.id) {
-					await Zotero.DB.executeTransaction(async function () {
-						await collectionTreeRow.ref.removeItems(toMove);
-					}.bind(this));
-				}
-			}
 		}
 		else if (dataType == 'text/x-moz-url' || dataType == 'application/x-moz-file') {
 			// Disallow drop into read-only libraries

test/tests/itemTreeTest.js

@@ -34,42 +34,116 @@ describe("Zotero.ItemTree", function() {
 	});
 	
 	describe("when performing a quick search", function () {
-		let parentItem, match, nonMatch;
+		let quicksearch;
-		let selectAllEvent = {key: 'a'};
+		
-		before(async function () {
+		before(() => {
-			parentItem = await createDataObject('item');
+			quicksearch = win.document.getElementById('zotero-tb-search');
-			match = await importFileAttachment('test.png', { title: 'find-me', parentItemID: parentItem.id });
-			nonMatch = await importFileAttachment('test.png', { title: 'not-a-result', parentItemID: parentItem.id });
-			if (Zotero.isMac) {
-				selectAllEvent.metaKey = true;
-			} else {
-				selectAllEvent.ctrlKey = true;
-			}
 		});
-
+		after(async () => {
-		it("should not select non-matching children when issuing a Select All command", async function () {
+			quicksearch.value = "";
-			var quicksearch = win.document.getElementById('zotero-tb-search');
-			quicksearch.value = match.getField('title');
 			quicksearch.doCommand();
 			await itemsView._refreshPromise;
-			itemsView.tree._onKeyDown(selectAllEvent);
-
-			var selected = itemsView.getSelectedItems(true);
-			assert.lengthOf(selected, 1);
-			assert.equal(selected[0], match.id);
 		});
-
+		
-		it("should expand collapsed parents with matching children when issuing a Select All command", async function () {
+		describe("when issuing a Select All command", function () {
-			itemsView.collapseAllRows();
+			let parentItem, match;
-			var selected = itemsView.getSelectedItems(true);
+			let selectAllEvent = { key: 'a' };
-			// After collapse the parent item is selected
-			assert.lengthOf(selected, 1);
-			assert.equal(selected[0], parentItem.id);
 			
-			itemsView.tree._onKeyDown(selectAllEvent);
+			before(async function () {
-			selected = itemsView.getSelectedItems(true);
+				parentItem = await createDataObject('item');
-			assert.lengthOf(selected, 1);
+				match = await importFileAttachment('test.png', { title: 'find-me', parentItemID: parentItem.id });
-			assert.equal(selected[0], match.id);
+				await importFileAttachment('test.png', { title: 'not-a-result', parentItemID: parentItem.id });
+				if (Zotero.isMac) {
+					selectAllEvent.metaKey = true;
+				}
+				else {
+					selectAllEvent.ctrlKey = true;
+				}
+			});
+			
+			after(async function() {
+				await parentItem.erase();
+			});
+			
+			it("should not select non-matching children", async function () {
+				quicksearch.value = match.getField('title');
+				quicksearch.doCommand();
+				await itemsView._refreshPromise;
+				itemsView.tree._onKeyDown(selectAllEvent);
+
+				var selected = itemsView.getSelectedItems(true);
+				assert.lengthOf(selected, 1);
+				assert.equal(selected[0], match.id);
+			});
+
+			it("should expand collapsed parents with matching children", async function () {
+				itemsView.collapseAllRows();
+				var selected = itemsView.getSelectedItems(true);
+				// After collapse the parent item is selected
+				assert.lengthOf(selected, 1);
+				assert.equal(selected[0], parentItem.id);
+				
+				itemsView.tree._onKeyDown(selectAllEvent);
+				selected = itemsView.getSelectedItems(true);
+				assert.lengthOf(selected, 1);
+				assert.equal(selected[0], match.id);
+			});
+		});
+		
+		describe("when dragging attachments", function () {
+			let parentItem, childItem;
+			before(async () => {
+				parentItem = await createDataObject('item', { title: "match-parent" });
+				childItem = await importFileAttachment('test.png', { title: 'match-child', parentItemID: parentItem.id });
+			});
+			
+			it("should display a child attachment when it is dragged into top level if it matches the search", async function () {
+				childItem.parentID = parentItem.id;
+				await childItem.save();
+				
+				quicksearch.value = "match";
+				quicksearch.doCommand();
+				
+				await itemsView._refreshPromise;
+				assert.lengthOf(itemsView._rows, 2);
+				assert.equal(itemsView.getRow(0).id, parentItem.id);
+				assert.equal(itemsView.getRow(1).id, childItem.id);
+				assert.equal(itemsView.getRow(1).level, 1);
+				
+				// The drop effectively does this
+				childItem.parentID = false;
+				await childItem.save();
+				await itemsView._refreshPromise;
+				
+				assert.lengthOf(itemsView._rows, 2);
+				assert.equal(itemsView.getRow(0).id, childItem.id);
+				assert.equal(itemsView.getRow(0).level, 0);
+				assert.equal(itemsView.getRow(1).id, parentItem.id);
+			});
+			
+			it("should display a child attachment when it is dragged onto a parent item if it matches the search", async function () {
+				childItem.parentID = false;
+				await childItem.save();
+				
+				quicksearch.value = "match";
+				quicksearch.doCommand();
+				
+				await itemsView._refreshPromise;
+				assert.lengthOf(itemsView._rows, 2);
+				assert.equal(itemsView.getRow(0).id, childItem.id);
+				assert.equal(itemsView.getRow(0).level, 0);
+				assert.equal(itemsView.getRow(1).id, parentItem.id);
+				
+				// The drop effectively does this
+				childItem.parentID = parentItem.id;
+				await childItem.save();
+				await itemsView._refreshPromise;
+				
+				assert.lengthOf(itemsView._rows, 2);
+				assert.equal(itemsView.getRow(0).id, parentItem.id);
+				assert.equal(itemsView.getRow(1).id, childItem.id);
+				assert.equal(itemsView.getRow(1).level, 1);
+			});
 		});
 	});