Revised logic of trashed collections in item cache (#4358)

- revert change from 2401a34031
that only loads un-trashed collections in _loadCollections.
If an item only belongs to deleted collections, item._loaded.collections = true
from _loadCollections will never run, so an exception
will be thrown in item.toJSON() when syncing happens.
Instead, to address the problem of item.getCollections()
having stale data #4307, add 'includeTrashed' parameter to
item.getCollections() based on which item._collections
will be filtered. Fixes: #4346
- revert earlier, no more necessary, changes from a532cfb475
to not alter item._collections cache when collections are being trashed or restored.
Collection is removed from item._collections only when it is permanently
erased.
- removed unnecessary test checking for consistent item._collections
value before and after reload, since item._collections is no longer
modified
- fix encountered bug where a trashed child collection is not
unloaded if a parent collection is erased without being trashed first.
- tweaked Zotero.Search sql construction to count items
that only belong to trashed collections into 'unfiled'. Fixes: #4347

---------

Co-authored-by: Dan Stillman

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

@@ -347,21 +347,6 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 		else {
 			let sql = "DELETE FROM deletedCollections WHERE collectionID=?";
 
-			// Subcollection is restored from trash - add it back into the object cache
-			if (this.parentKey) {
-				let parent = Zotero.Collections.getIDFromLibraryAndKey(this.libraryID, this.parentKey);
-				Zotero.DB.addCurrentCallback("commit", function () {
-					this.ObjectsClass.registerChildCollection(parent, this.id);
-				}.bind(this));
-			}
-
-			// Add restored collection back into item's _collections cache
-			this.getChildItems(false, true).forEach((item) => {
-				let collectionCached = item._collections.includes(this.id);
-				if (!collectionCached) {
-					item._collections.push(this.id);
-				}
-			});
 			yield Zotero.DB.queryAsync(sql, collectionID);
 		}
 		
@@ -616,7 +601,6 @@ Zotero.Collection.prototype.trash = Zotero.Promise.coroutine(function* (env) {
 	var libraryHasTrash = Zotero.Libraries.hasTrash(this.libraryID);
 	
 	var del = [];
-	var itemsToUpdate = [];
 	for(var i=0, len=descendents.length; i<len; i++) {
 		// Descendent collections
 		if (descendents[i].type == 'collection') {
@@ -641,10 +625,6 @@ Zotero.Collection.prototype.trash = Zotero.Promise.coroutine(function* (env) {
 				del.push(descendents[i].id);
 			}
 			
-			// If item isn't being removed or is just moving to the trash, mark for update
-			if (!env.options.deleteItems || libraryHasTrash) {
-				itemsToUpdate.push(descendents[i].id);
-			}
 		}
 	}
 	if (del.length) {
@@ -678,7 +658,6 @@ Zotero.Collection.prototype.trash = Zotero.Promise.coroutine(function* (env) {
 	if (env.isNew) {
 		return;
 	}
-	env.deletedObjectIDs = collections;
 	
 	// Reload collection data to show/restore deleted collections from trash
 	for (let collectionID of collections) {
@@ -686,14 +665,6 @@ Zotero.Collection.prototype.trash = Zotero.Promise.coroutine(function* (env) {
 		yield collection.loadDataType('primaryData', true);
 		yield collection.loadDataType('childCollections', true);
 	}
-	// Update collection cache for descendant items
-	if (itemsToUpdate.length) {
-		let deletedCollections = new Set(env.deletedObjectIDs);
-		itemsToUpdate.forEach((itemID) => {
-			let item = Zotero.Items.get(itemID);
-			item._collections = item._collections.filter(c => !deletedCollections.has(c));
-		});
-	}
 });
 
 /**
@@ -707,10 +678,13 @@ Zotero.Collection.prototype._eraseData = Zotero.Promise.coroutine(function* (env
 	}
 	
 	var collections = [this.id];
-	var descendents = this.getDescendents(false, null, true)
+	var descendents = this.getDescendents(false, null, true);
+	collections = descendents
 		.filter(d => d.type == 'collection')
-		.map(c => c.id);
-	collections = collections.concat(descendents);
+		.map(c => c.id).concat(collections);
+
+	// Make sure all descendant collections will be unloaded in this._finalizeErase()
+	env.deletedObjectIDs = collections;
 
 	yield Zotero.Utilities.Internal.forEachChunkAsync(
 		collections,
@@ -741,6 +715,14 @@ Zotero.Collection.prototype._eraseData = Zotero.Promise.coroutine(function* (env
 			this.ObjectsClass.unregisterChildCollection(parentCollectionID, this.id);
 		}.bind(this));
 	}
+	
+	// Remove erased collection from collection cache of descendant items
+	var itemsToUpdate = descendents.filter(d => d.type == 'item').map(c => c.id);
+	let deletedCollections = new Set(collections);
+	itemsToUpdate.forEach((itemID) => {
+		let item = Zotero.Items.get(itemID);
+		item._collections = item._collections.filter(c => !deletedCollections.has(c));
+	});
 });
 
 Zotero.Collection.prototype._finalizeErase = Zotero.Promise.coroutine(function* (env) {

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

@@ -4261,11 +4261,21 @@ Zotero.Item.prototype.removeAllTags = function() {
 /**
  * Gets the collections the item is in
  *
+ * @param {Boolean} includeTrashed Include trashed collections
  * @return {Array<Integer>}  An array of collectionIDs for all collections the item belongs to
  */
-Zotero.Item.prototype.getCollections = function () {
+Zotero.Item.prototype.getCollections = function (includeTrashed) {
 	this._requireData('collections');
-	return this._collections.concat();
+	if (includeTrashed) {
+		return this._collections.concat();
+	}
+	return this._collections.filter((id) => {
+		var col = Zotero.Collections.get(id);
+		if (!col) {
+			throw new Error("Collection " + id + " not found for item " + this.libraryKey);
+		}
+		return !col.deleted;
+	});
 };
 
 

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

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

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

@@ -1145,7 +1145,8 @@ Zotero.Search.prototype._buildQuery = Zotero.Promise.coroutine(function* () {
 	
 	if (unfiled) {
 		sql += " AND (itemID NOT IN ("
-			+ "SELECT itemID FROM collectionItems "
+			// Exclude items that belong to non-trashed collections
+			+ "SELECT itemID FROM collectionItems WHERE collectionID NOT IN (SELECT collectionID FROM deletedCollections) "
 			// Exclude children
 			+ "UNION SELECT itemID FROM itemAttachments WHERE parentItemID IS NOT NULL "
 			+ "UNION SELECT itemID FROM itemNotes WHERE parentItemID IS NOT NULL "

test/tests/collectionTest.js

@@ -102,8 +102,10 @@ describe("Zotero.Collection", function() {
 			collection1.deleted = true;
 			await collection1.saveTx();
 
-			// Deleted collection is gone from item's cache
+			// Trashed collection does not count as one of item's containers
 			assert.notInclude(item1.getCollections(), collection1.id);
+			// But it should still return it if includeTrashed=true is passed
+			assert.include(item1.getCollections(true), collection1.id);
 
 			// Restore deleted collection
 			collection1.deleted = false;
@@ -114,40 +116,25 @@ describe("Zotero.Collection", function() {
 			// Collection is restored from trash
 			assert.notInclude(deleted, collection1.id);
 
-			// Collection is back in item's cache
+			// Item belongs to the restored collection
 			assert.include(item1.getCollections(), collection1.id);
 		});
 
 		it("should permanently delete collections from trash", async function () {
 			var collection1 = await createDataObject('collection');
 			var collection2 = await createDataObject('collection', { parentID: collection1.id });
-			var collection3 = await createDataObject('collection', { parentID: collection2.id });
+			var collection3 = await createDataObject('collection', { parentID: collection2.id, deleted: true });
+			var item = await createDataObject('item', { collections: [collection1.id, collection2.id, collection3.id] });
 			
 			await collection1.eraseTx();
 			
 			assert.equal(await Zotero.Collections.getAsync(collection1.id), false);
 			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);
+			// Erased collections are fully removed as item's containers
+			assert.equal(item.getCollections().length, 0);
+			assert.equal(item.getCollections(true).length, 0);
 		});
 	})
 	

test/tests/itemTest.js

@@ -696,6 +696,46 @@ describe("Zotero.Item", function () {
 	})
 	
 	
+	describe("#getCollections()", function () {
+		it("shouldn't include collections in the trash", async function () {
+			var collection1 = await createDataObject('collection');
+			var collection2 = await createDataObject('collection');
+			var item = await createDataObject('item', { collections: [collection1.id, collection2.id] });
+			
+			assert.sameMembers(item.getCollections(), [collection1.id, collection2.id]);
+			
+			collection1.deleted = true;
+			await collection1.saveTx();
+			
+			assert.sameMembers(item.getCollections(), [collection2.id]);
+			
+			// Simulate a restart
+			await Zotero.Items.get(item.id).reload(null, true);
+			
+			// Make sure the deleted collection is not back in item's cache
+			assert.sameMembers(item.getCollections(), [collection2.id]);
+		});
+		
+		it("should include collections in the trash if includeTrashed=true", async function () {
+			var collection1 = await createDataObject('collection');
+			var collection2 = await createDataObject('collection');
+			var item = await createDataObject('item', { collections: [collection1.id, collection2.id] });
+			
+			assert.sameMembers(item.getCollections(true), [collection1.id, collection2.id]);
+			
+			collection1.deleted = true;
+			await collection1.saveTx();
+			
+			assert.sameMembers(item.getCollections(true), [collection1.id, collection2.id]);
+			
+			// Simulate a restart
+			await Zotero.Items.get(item.id).reload(null, true);
+			
+			assert.sameMembers(item.getCollections(true), [collection1.id, collection2.id]);
+		});
+	});
+	
+	
 	describe("#setCollections()", function () {
 		it("should add a collection with an all-numeric key", async function () {
 			var col = new Zotero.Collection();

test/tests/searchTest.js

@@ -582,6 +582,25 @@ describe("Zotero.Search", function() {
 					assert.include(matches, item1.id);
 					assert.notInclude(matches, item2.id);
 				});
+				it("should include items belonging only to trashed collections", async function () {
+					var collection = await createDataObject('collection');
+					var item = await createDataObject('item', { collections: [collection.id] });
+					
+					var s = new Zotero.Search;
+					s.libraryID = Zotero.Libraries.userLibraryID;
+					s.addCondition('unfiled', 'true');
+
+					// item belonging to a non-trashed collection is not unfiled
+					var matches = await s.search();
+					assert.notInclude(matches, item.id);
+
+					collection.deleted = true;
+					await collection.saveTx();
+
+					// item that only belongs to a trashed collection is unfiled
+					matches = await s.search();
+					assert.include(matches, item.id);
+				});
 			});
 			
 			describe("Quick search", function () {