diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index 10dab3c779..d29dcbf4c2 100644 --- a/chrome/content/zotero/xpcom/data/collection.js +++ b/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 { - 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) { diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 6d5714638b..eaa5aa1cfa 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/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} 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; + }); }; diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index 1f32eafbde..17bea222c7 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/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; diff --git a/chrome/content/zotero/xpcom/data/search.js b/chrome/content/zotero/xpcom/data/search.js index 7991805cae..20a36b77f9 100644 --- a/chrome/content/zotero/xpcom/data/search.js +++ b/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 " diff --git a/test/tests/collectionTest.js b/test/tests/collectionTest.js index fc80254369..b4340bb434 100644 --- a/test/tests/collectionTest.js +++ b/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); }); }) diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index aa48c3a6cb..5029628f40 100644 --- a/test/tests/itemTest.js +++ b/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(); diff --git a/test/tests/searchTest.js b/test/tests/searchTest.js index 2f554015c6..c2c7a85096 100644 --- a/test/tests/searchTest.js +++ b/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 () {