Fix updating of deleted collections in Libraries & Collections (#4324)

- listen to 'collection' notifier events and re-render
the section if a relevant collection is moved to trash or
modified. That way, a deleted collection will be removed,
a restored collection will be added back, and renaming a
collection will update the name.
- fixed a bug where a restored collection would not
be added into item._collections cache. Fixed false-positive
test for it.
- do not add deleted collections into items. _collections cache in
Zotero.Items._loadCollections. Otherwise, deleted collections will
appear in librariesCollectionsBox after the app is restarted.

chrome/content/zotero/elements/librariesCollectionsBox.js

@@ -65,7 +65,7 @@ import { getCSSIcon } from 'components/icons';
 		}
 
 		init() {
-			this._notifierID = Zotero.Notifier.registerObserver(this, ['item'], 'librariesCollectionsBox');
+			this._notifierID = Zotero.Notifier.registerObserver(this, ['item', 'collection'], 'librariesCollectionsBox');
 			this._body = this.querySelector('.body');
 			this.initCollapsibleSection();
 			this._section.addEventListener('add', this._handleAdd);
@@ -77,11 +77,19 @@ import { getCSSIcon } from 'components/icons';
 		}
 
 		notify(action, type, ids) {
+			if (!this._item) return;
 			if (action == 'modify'
-					&& this._item
+					&& type == "item"
 					&& (ids.includes(this._item.id) || this._linkedItems.some(item => ids.includes(item.id)))) {
 				this._forceRenderAll();
 			}
+			
+			if (["modify", "trash"].includes(action) && type == "collection") {
+				let isRelevantCollection = ids.some(id => Zotero.Collections.get(id).hasItem(this._item));
+				if (isRelevantCollection) {
+					this._forceRenderAll();
+				}
+			}
 		}
 		
 		_buildRow(obj, level, contextItem) {

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

@@ -357,8 +357,8 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 
 			// Add restored collection back into item's _collections cache
 			this.getChildItems(false, true).forEach((item) => {
-				const collectionNotCached = item._collections.filter(c => c != this.id).length == 0;
-				if (collectionNotCached) {
+				let collectionCached = item._collections.includes(this.id);
+				if (!collectionCached) {
 					item._collections.push(this.id);
 				}
 			});

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

@@ -797,7 +797,8 @@ Zotero.Items = function() {
 	this._loadCollections = Zotero.Promise.coroutine(function* (libraryID, ids, idSQL) {
 		var sql = "SELECT itemID, collectionID FROM items "
 			+ "LEFT JOIN collectionItems USING (itemID) "
-			+ "WHERE libraryID=?" + idSQL;
+			+ "LEFT JOIN deletedCollections USING (collectionID) "
+			+ "WHERE deletedCollections.collectionID IS NULL AND libraryID=?" + idSQL;
 		var params = [libraryID];
 		
 		var lastItemID;

test/tests/collectionTest.js

@@ -94,7 +94,8 @@ describe("Zotero.Collection", function() {
 
 		it("should restore deleted collection", async function () {
 			var collection1 = await createDataObject('collection');
-			var item1 = await createDataObject('item', { collections: [collection1.id] });
+			var collection2 = await createDataObject('collection');
+			var item1 = await createDataObject('item', { collections: [collection1.id, collection2.id] });
 
 			assert.include(item1.getCollections(), collection1.id);
 
@@ -128,6 +129,26 @@ describe("Zotero.Collection", function() {
 			assert.equal(await Zotero.Collections.getAsync(collection2.id), false);
 			assert.equal(await Zotero.Collections.getAsync(collection3.id), false);
 		});
+
+		it("should not load deleted collections into item's cache on startup", async function () {
+			var collection1 = await createDataObject('collection');
+			var collection2 = await createDataObject('collection');
+			var item = await createDataObject('item', { collections: [collection1.id, collection2.id] });
+
+			collection1.deleted = true;
+			await collection1.saveTx();
+
+			// Make sure the deleted collection is gone from item's cache
+			assert.notInclude(item.getCollections(), collection1.id);
+			assert.include(item.getCollections(), collection2.id);
+
+			// Reload the item's data from DB
+			await Zotero.Items.get(item.id).reload(null, true);
+
+			// Make sure the deleted collection is not back in item's cache
+			assert.notInclude(item.getCollections(), collection1.id);
+			assert.include(item.getCollections(), collection2.id);
+		});
 	})
 	
 	describe("#version", function () {

test/tests/itemPaneTest.js

@@ -394,6 +394,60 @@ describe("Item pane", function () {
 		});
 	});
 
+	describe("Libraries and collections pane", function () {
+		var item, collectionParent, collectionChild, section;
+
+		// Fresh setup of an item belonging to 2 collections - parent and child - for each test
+		beforeEach(async function () {
+			collectionParent = await createDataObject('collection');
+			collectionChild = await createDataObject('collection', { parentID: collectionParent.id });
+			item = await createDataObject('item', { collections: [collectionParent.id, collectionChild.id] });
+			await ZoteroPane.selectItem(item.id);
+			section = ZoteroPane.itemPane._itemDetails.getPane("libraries-collections");
+		});
+		
+		it("should update collection's name after rename", async function () {
+			collectionChild.name = "Updated collection name";
+			collectionChild.saveTx();
+
+			await waitForNotifierEvent('modify', 'collection');
+
+			let collectionRow = section.querySelector(`.row[data-id="C${collectionChild.id}"]`);
+			assert.equal(collectionRow.innerText, collectionChild.name);
+		});
+
+		it("should remove collection that has been trashed", async function () {
+			collectionChild.deleted = true;
+			collectionChild.saveTx();
+
+			await waitForNotifierEvent('trash', 'collection');
+
+			let rowIDs = [...section.querySelectorAll(".row")].map(node => node.dataset.id);
+			assert.deepEqual(rowIDs, [`L${item.libraryID}`, `C${collectionParent.id}`]);
+		});
+
+		it("should bring back collection restored from trash", async function () {
+			collectionChild.deleted = true;
+			collectionChild.saveTx();
+
+			await waitForNotifierEvent('trash', 'collection');
+
+			// Make sure the collection is actually gone
+			let rowIDs = [...section.querySelectorAll(".row")].map(node => node.dataset.id);
+			assert.deepEqual(rowIDs, [`L${item.libraryID}`, `C${collectionParent.id}`]);
+
+			// Restore the collection from trash
+			collectionChild.deleted = false;
+			collectionChild.saveTx();
+
+			await waitForNotifierEvent('modify', 'collection');
+
+			// The collection row should appear again
+			rowIDs = [...section.querySelectorAll(".row")].map(node => node.dataset.id);
+			assert.deepEqual(rowIDs, [`L${item.libraryID}`, `C${collectionParent.id}`, `C${collectionChild.id}`]);
+		});
+	});
+
 	describe("Attachments pane", function () {
 		let paneID = "attachments";