Fix trashing of descendant items when deleting a collection

Also allows 'collections' property to be passed to
createDataObject()/createUnsavedDataObject() in tests.

chrome/content/zotero/itemPane.js

@@ -179,7 +179,7 @@ var ZoteroItemPane = new function() {
 		var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
 								.getService(Components.interfaces.nsIPromptService);
 		if (ps.confirm(null, '', Zotero.getString('pane.item.notes.delete.confirm'))) {
-			Zotero.Items.trash(id);
+			Zotero.Items.trashTx(id);
 		}
 	}
 	

chrome/content/zotero/xpcom/collectionTreeView.js

@@ -1037,7 +1037,9 @@ Zotero.CollectionTreeView.prototype.deleteSelection = Zotero.Promise.coroutine(f
 		//erase collection from DB:
 		var treeRow = this.getRow(rows[i]-i);
 		if (treeRow.isCollection()) {
-			yield treeRow.ref.erase(deleteItems);
+			yield treeRow.ref.eraseTx({
+				deleteItems: true
+			});
 		}
 		else if (treeRow.isSearch()) {
 			yield Zotero.Searches.erase(treeRow.ref.id);

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

@@ -1136,13 +1136,15 @@ Zotero.DataObject.prototype.updateSynced = Zotero.Promise.coroutine(function* (s
  * Delete object from database
  *
  * @param {Object} [options]
+ * @param {Boolean} [options.deleteItems] - Move descendant items to trash (Collection only)
  * @param {Boolean} [options.skipDeleteLog] - Don't add to sync delete log
  */
-Zotero.DataObject.prototype.erase = Zotero.Promise.coroutine(function* (options) {
-	options = options || {};
-	var env = {
-		options: options
-	};
+Zotero.DataObject.prototype.erase = Zotero.Promise.coroutine(function* (options = {}) {
+	if (!options || typeof options != 'object') {
+		throw new Error("'options' must be an object");
+	}
+	
+	var env = { options };
 	
 	if (!env.options.tx && !Zotero.DB.inTransaction()) {
 		Zotero.logError("erase() called on Zotero." + this._ObjectType + " without a wrapping "

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

@@ -148,6 +148,7 @@ Zotero.defineProperty(Zotero.Item.prototype, 'parentItemKey', {
 	set: function(val) this.parentKey = val
 });
 
+
 Zotero.defineProperty(Zotero.Item.prototype, 'firstCreator', {
 	get: function() this._firstCreator
 });

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

@@ -492,32 +492,39 @@ Zotero.Items = function() {
 	};
 	
 	
-	this.trash = function (ids) {
+	this.trash = Zotero.Promise.coroutine(function* (ids) {
+		Zotero.DB.requireTransaction();
+		
 		ids = Zotero.flattenArguments(ids);
 		
-		return Zotero.DB.executeTransaction(function* () {
-			for (let i=0; i<ids.length; i++) {
-				let id = ids[i];
-				let item = yield this.getAsync(id);
-				if (!item) {
-					Zotero.debug('Item ' + id + ' does not exist in Items.trash()!', 1);
-					Zotero.Notifier.queue('delete', 'item', id);
-					continue;
-				}
-				
-				if (!item.isEditable()) {
-					throw new Error(item._ObjectType + " " + item.libraryKey + " is not editable");
-				}
-				
-				if (!Zotero.Libraries.hasTrash(item.libraryID)) {
-					throw new Error(Zotero.Libraries.getName(item.libraryID) + " does not have Trash");
-				}
-				
-				item.deleted = true;
-				yield item.save({
-					skipDateModifiedUpdate: true
-				});
+		for (let i=0; i<ids.length; i++) {
+			let id = ids[i];
+			let item = yield this.getAsync(id);
+			if (!item) {
+				Zotero.debug('Item ' + id + ' does not exist in Items.trash()!', 1);
+				Zotero.Notifier.queue('delete', 'item', id);
+				continue;
 			}
+			
+			if (!item.isEditable()) {
+				throw new Error(item._ObjectType + " " + item.libraryKey + " is not editable");
+			}
+			
+			if (!Zotero.Libraries.hasTrash(item.libraryID)) {
+				throw new Error(Zotero.Libraries.getName(item.libraryID) + " does not have Trash");
+			}
+			
+			item.deleted = true;
+			yield item.save({
+				skipDateModifiedUpdate: true
+			});
+		}
+	});
+	
+	
+	this.trashTx = function (ids) {
+		return Zotero.DB.executeTransaction(function* () {
+			return this.trash(ids);
 		}.bind(this));
 	}
 	

chrome/content/zotero/xpcom/itemTreeView.js

@@ -1797,7 +1797,7 @@ Zotero.ItemTreeView.prototype.deleteSelection = Zotero.Promise.coroutine(functio
 		Zotero.Items.erase(ids);
 	}
 	else if (collectionTreeRow.isLibrary(true) || force) {
-		Zotero.Items.trash(ids);
+		Zotero.Items.trashTx(ids);
 	}
 	else if (collectionTreeRow.isCollection()) {
 		collectionTreeRow.ref.removeItems(ids);

test/content/support.js

@@ -319,6 +319,9 @@ function createUnsavedDataObject(objectType, params = {}) {
 		if (params.title !== undefined || params.setTitle) {
 			obj.setField('title', params.title !== undefined ? params.title : Zotero.Utilities.randomString());
 		}
+		if (params.collections !== undefined) {
+			obj.setCollections(params.collections);
+		}
 		break;
 	
 	case 'collection':

test/tests/collectionTest.js

@@ -13,6 +13,31 @@ describe("Zotero.Collection", function() {
 		});
 	})
 	
+	describe("#erase()", function () {
+		it("should delete a collection but not its descendant item by default", function* () {
+			var collection = yield createDataObject('collection');
+			var item = yield createDataObject('item', { collections: [collection.id] });
+			assert.isTrue(collection.hasItem(item.id));
+			
+			yield collection.eraseTx();
+			
+			assert.isFalse((yield Zotero.Items.getAsync(item.id)).deleted);
+		})
+		
+		it("should delete a collection and trash its descendant items with deleteItems: true", function* () {
+			var collection = yield createDataObject('collection');
+			var item1 = yield createDataObject('item', { collections: [collection.id] });
+			var item2 = yield createDataObject('item', { collections: [collection.id] });
+			assert.isTrue(collection.hasItem(item1.id));
+			assert.isTrue(collection.hasItem(item2.id));
+			
+			yield collection.eraseTx({ deleteItems: true });
+			
+			assert.isTrue((yield Zotero.Items.getAsync(item1.id)).deleted);
+			assert.isTrue((yield Zotero.Items.getAsync(item2.id)).deleted);
+		})
+	})
+	
 	describe("#version", function () {
 		it("should set object version", function* () {
 			var version = 100;

test/tests/dataObjectUtilitiesTest.js

@@ -35,7 +35,7 @@ describe("Zotero.DataObjectUtilities", function() {
 					var changes = Zotero.DataObjectUtilities.diff(json1, json2);
 					assert.lengthOf(changes, 0);
 					
-					yield Zotero.Items.erase(id1, id2);
+					yield Zotero.Items.erase([id1, id2]);
 				})
 				
 				it("should not show empty strings as different", function () {