Collections data layer cleanup

Get rid of data_access.js, at long last. Existing calls to
Zotero.getCollections() will need to be replaced with
Zotero.Collections.getByLibrary() or .getByParent().

Also removes Zotero.Collection::getCollections(), which is redundant
with Zotero.Collections.getByLibrary(), and Zotero.Collections.add().
The latter didn't didn't include a libraryID anyway, so code might as
well just use 'new Zotero.Collection' instead.

chrome/content/zotero/bindings/zoterosearch.xml

@@ -440,7 +440,7 @@
 							var rows = [];
 							
 							var libraryID = this.parent.search.libraryID;
-							var cols = Zotero.getCollections(false, true, libraryID);
+							var cols = Zotero.getCollections(false, true, libraryID); // TODO: Replace with Zotero.Collections.getByLibrary()
 							for (var i in cols) {
 								// Indent subcollections
 								var indent = '';

chrome/content/zotero/fileInterface.js

@@ -285,7 +285,7 @@ var Zotero_File_Interface = new function() {
 				var leafName = translation.location.leafName;
 				var collectionName = (translation.location.isDirectory() || leafName.indexOf(".") === -1 ? leafName
 					: leafName.substr(0, leafName.lastIndexOf(".")));
-				var allCollections = Zotero.getCollections();
+				var allCollections = Zotero.getCollections(); // TODO: Replace with Zotero.Collections.getBy*
 				for(var i=0; i<allCollections.length; i++) {
 					if(allCollections[i].name == collectionName) {
 						collectionName += " "+(new Date()).toLocaleString();
@@ -298,7 +298,7 @@ var Zotero_File_Interface = new function() {
 
 			if(createNewCollection) {
 				// Create a new collection to take imported items
-				importCollection = Zotero.Collections.add(collectionName);
+				importCollection = Zotero.Collections.add(collectionName); // TODO: Fix
 			} else {
 				// Import into currently selected collection
 				try {

chrome/content/zotero/xpcom/collectionTreeView.js

@@ -1028,20 +1028,18 @@ Zotero.CollectionTreeView.prototype._expandRow = Zotero.Promise.coroutine(functi
 	var treeRow = rows[row];
 	var level = rows[row].level;
 	var isLibrary = treeRow.isLibrary(true);
-	var isGroup = treeRow.isGroup();
 	var isCollection = treeRow.isCollection();
 	var libraryID = treeRow.ref.libraryID;
 
 	if (treeRow.isPublications()) {
 		return false;
 	}
 
-	if (isGroup) {
-		var group = yield Zotero.Groups.getByLibraryID(libraryID);
-		var collections = yield group.getCollections();
+	if (isLibrary) {
+		var collections = yield Zotero.Collections.getByLibrary(libraryID, treeRow.ref.id);
 	}
-	else {
-		var collections = yield Zotero.Collections.getByParent(libraryID, treeRow.ref.id);
+	else if (isCollection) {
+		var collections = yield Zotero.Collections.getByParent(treeRow.ref.id);
 	}
 
 	if (isLibrary) {

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

@@ -146,17 +146,12 @@ Zotero.Collection.prototype.hasChildItems = function() {
 /**
  * Returns subcollections of this collection
  *
- * @param	bool		asIDs		Return as collectionIDs
- * @return	array				Array of Zotero.Collection instances
- *									or collectionIDs, or FALSE if none
+ * @param {Boolean} [asIDs=false] Return as collectionIDs
+ * @return {Zotero.Collection[]|Integer[]}
  */
 Zotero.Collection.prototype.getChildCollections = function (asIDs) {
 	this._requireData('childCollections');
 
-	if (this._childCollections.length == 0) {
-		return false;
-	}
-
 	// Return collectionIDs
 	if (asIDs) {
 		var ids = [];
@@ -681,13 +676,12 @@ Zotero.Collection.prototype.toJSON = function (options, patch) {
  *											nodes instead of flat array
  * @param	{String}	[type]				'item', 'collection', or NULL for both
  * @param	{Boolean}	[includeDeletedItems=false]		Include items in Trash
- * @return	{Object[]}			Array of objects with 'id', 'key',
- *								'type' ('item' or 'collection'), 'parent',
- *								and, if collection, 'name' and the nesting 'level'
+ * @return	{Promise<Object[]>} - A promise for an array of objects with 'id', 'key',
+ *   'type' ('item' or 'collection'), 'parent', and, if collection, 'name' and the nesting 'level'
  */
 Zotero.Collection.prototype.getChildren = Zotero.Promise.coroutine(function* (recursive, nested, type, includeDeletedItems, level) {
 	if (!this.id) {
-		throw ('Zotero.Collection.getChildren() cannot be called on an unsaved item');
+		throw new Error('Cannot be called on an unsaved item');
 	}
 
 	var toReturn = [];

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

@@ -53,43 +53,47 @@ Zotero.Collections = function() {
 	this._primaryDataSQLFrom = "FROM collections O "
 			+ "LEFT JOIN collections CP ON (O.parentCollectionID=CP.collectionID)";
 
+
 	/**
-	* Add new collection to DB and return Collection object
-	*
-	* _name_ is non-empty string
-	* _parent_ is optional collectionID -- creates root collection by default
-	*
-	* Returns true on success; false on error
-	**/
-	this.add = function (name, parent) {
-		var col = new Zotero.Collection;
-		col.name = name;
-		col.parent = parent;
-		var id = col.save();
-		return this.getAsync(id);
+	 * Get collections within a library
+	 *
+	 * Either libraryID or parentID must be provided
+	 *
+	 * @param {Integer} libraryID
+	 * @param {Boolean} [recursive=false]
+	 * @return {Promise<Zotero.Collection[]>}
+	 */
+	this.getByLibrary = function (libraryID, recursive) {
+		return _getByContainer(libraryID, null, recursive);
 	}
 
 
-	/*
-	 * Zotero.getCollections(parent)
-	 *
-	 * Returns an array of all collections are children of a collection
-	 * as Zotero.Collection instances
+	/**
+	 * Get collections that are subcollection of a given collection
 	 *
-	 * Takes parent collectionID as optional parameter;
-	 * by default, returns root collections
+	 * @param {Integer} parentCollectionID
+	 * @param {Boolean} [recursive=false]
+	 * @return {Promise<Zotero.Collection[]>}
 	 */
-	this.getByParent = Zotero.Promise.coroutine(function* (libraryID, parentID, recursive) {
+	this.getByParent = function (parentCollectionID, recursive) {
+		return _getByContainer(null, parentCollectionID, recursive);
+	}
+
+
+	var _getByContainer = Zotero.Promise.coroutine(function* (libraryID, parentID, recursive) {
 		let children;
 
 		if (parentID) {
-			let parent = yield this.getAsync(parentID);
+			let parent = yield Zotero.Collections.getAsync(parentID);
 			yield parent.loadChildCollections();
 			children = parent.getChildCollections();
 		} else if (libraryID) {
-			children = yield this.getCollectionsInLibrary(libraryID);
+			let sql = "SELECT collectionID AS id FROM collections "
+				+ "WHERE libraryID=? AND parentCollectionID IS NULL";
+			let ids = yield Zotero.DB.columnQueryAsync(sql, [libraryID]);
+			children = yield this.getAsync(ids);
 		} else {
-			throw new Error("Either library ID or parent collection ID must be provided to getNumCollectionsByParent");
+			throw new Error("Either library ID or parent collection ID must be provided");
 		}
 
 		if (!children.length) {
@@ -106,7 +110,7 @@ Zotero.Collections = function() {
 			var obj = children[i];
 			toReturn.push(obj);
 
-			var desc = obj.getDescendents(false, 'collection');
+			var desc = yield obj.getDescendents(false, 'collection');
 			for (var j in desc) {
 				var obj2 = yield this.getAsync(desc[j]['id']);
 				if (!obj2) {
@@ -126,18 +130,7 @@ Zotero.Collections = function() {
 		}
 
 		return toReturn;
-	});
-
-
-	this.getCollectionsInLibrary = Zotero.Promise.coroutine(function* (libraryID) {
-		let sql = "SELECT collectionID AS id FROM collections C "
-			+ "WHERE libraryID=? AND parentCollectionId IS NULL";
-		let ids = yield Zotero.DB.queryAsync(sql, [libraryID]);
-		let collections = yield this.getAsync(ids.map(function(row) row.id));
-		if (!collections.length) return collections;
-
-		return collections.sort(function (a, b) Zotero.localeCompare(a.name, b.name));
-	});
+	}.bind(this));
 
 
 	this.getCollectionsContainingItems = function (itemIDs, asIDs) {

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

@@ -182,34 +182,6 @@ Zotero.Group.prototype.clearSearchCache = function () {
 	this._hasSearches = null;
 }
 
-/**
- * Returns collections of this group
- *
- * @param	{Boolean}		asIDs				Return as collectionIDs
- * @return	{Zotero.Collection[]}				Array of Zotero.Collection instances
- */
-Zotero.Group.prototype.getCollections = Zotero.Promise.coroutine(function* (parent) {
-	var sql = "SELECT collectionID FROM collections WHERE libraryID=? AND "
-				+ "parentCollectionID " + (parent ? '=' + parent : 'IS NULL');
-	var ids = yield Zotero.DB.columnQueryAsync(sql, this.libraryID);
-
-	// Return Zotero.Collection objects
-	var objs = [];
-	for each(var id in ids) {
-		var col = yield Zotero.Collections.getAsync(id);
-		objs.push(col);
-	}
-
-	// Do proper collation sort
-	var collation = Zotero.getLocaleCollation();
-	objs.sort(function (a, b) {
-		return collation.compareString(1, a.name, b.name);
-	});
-
-	return objs;
-});
-
-
 Zotero.Group.prototype.hasItem = function (item) {
 	if (!(item instanceof Zotero.Item)) {
 		throw new Error("item must be a Zotero.Item");

chrome/content/zotero/xpcom/translation/translate_item.js

@@ -159,7 +159,7 @@ Zotero.Translate.ItemSaver.prototype = {
 		}.bind(this));
 	},
 
-	"saveCollection":function(collection) {
+	"saveCollection": Zotero.Promise.coroutine(function* (collection) {
 		var collectionsToProcess = [collection];
 		var parentIDs = [null];
 		var topLevelCollection;
@@ -168,7 +168,14 @@ Zotero.Translate.ItemSaver.prototype = {
 			var collection = collectionsToProcess.shift();
 			var parentID = parentIDs.shift();
 
-			var newCollection = Zotero.Collections.add(collection.name, parentID);
+			var newCollection = new Zotero.Collection;
+			newCollection.libraryID = this._libraryID;
+			newCollection.name = collection.name;
+			if (parentID) {
+				newCollection.parentID = parentID;
+			}
+			yield newCollection.save();
+
 			if(parentID === null) topLevelCollection = newCollection;
 
 			this.newCollections.push(newCollection.id);
@@ -193,12 +200,12 @@ Zotero.Translate.ItemSaver.prototype = {
 
 			if(toAdd.length) {
 				Zotero.debug("Translate: Adding " + toAdd, 5);
-				newCollection.addItems(toAdd);
+				yield newCollection.addItems(toAdd);
 			}
 		}
 
 		return topLevelCollection;
-	},
+	}),
 
 	/**
 	 * Saves a translator attachment to the database
@@ -717,7 +724,7 @@ Zotero.Translate.ItemGetter.prototype = {
 
 		if(getChildCollections) {
 			// get child collections
-			this._collectionsLeft = Zotero.getCollections(collection.id, true);
+			this._collectionsLeft = Zotero.getCollections(collection.id, true); // TODO: Replace with Zotero.Collections.getByParent()
 
 			// get items in child collections
 			for each(var collection in this._collectionsLeft) {
@@ -740,7 +747,7 @@ Zotero.Translate.ItemGetter.prototype = {
 		this._itemsLeft = Zotero.Items.getAll(libraryID, true);
 
 		if(getChildCollections) {
-			this._collectionsLeft = Zotero.getCollections(null, true, libraryID);
+			this._collectionsLeft = Zotero.getCollections(null, true, libraryID); // TODO: Replace with Zotero.Collections.getByLibrary()
 		}
 
 		this.numItems = this._itemsLeft.length;

components/zotero-service.js

@@ -61,7 +61,6 @@ const xpcomFilesLocal = [
 	'attachments',
 	'cite',
 	'cookieSandbox',
-	'data_access',
 	'data/dataObject',
 	'data/dataObjects',
 	'data/dataObjectUtilities',

test/content/support.js

@@ -160,12 +160,12 @@ function createUnsavedDataObject(objectType, params) {
 		obj.name = params.name !== undefined ? params.name : "Test";
 		break;
 	}
-	if (params.version !== undefined) {
-		obj.version = params.version
-	}
-	if (params.synced !== undefined) {
-		obj.synced = params.synced
-	}
+	var allowedParams = ['parentID', 'parentKey', 'synced', 'version'];
+	allowedParams.forEach(function (param) {
+		if (params[param] !== undefined) {
+			obj[param] = params[param];
+		}
+	})
 	return obj;
 }