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,7 +1028,6 @@ 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;
 	
@@ -1036,12 +1035,11 @@ Zotero.CollectionTreeView.prototype._expandRow = Zotero.Promise.coroutine(functi
 		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)
+	/**
+	 * Get collections that are subcollection of a given collection
 	 *
-	 * Returns an array of all collections are children of a collection
-	 * as Zotero.Collection instances
-	 *
-	 * 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/data_access.js

@@ -1,89 +0,0 @@
-/*
-    ***** BEGIN LICENSE BLOCK *****
-    
-    Copyright © 2009 Center for History and New Media
-                     George Mason University, Fairfax, Virginia, USA
-                     http://zotero.org
-    
-    This file is part of Zotero.
-    
-    Zotero is free software: you can redistribute it and/or modify
-    it under the terms of the GNU Affero General Public License as published by
-    the Free Software Foundation, either version 3 of the License, or
-    (at your option) any later version.
-    
-    Zotero is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU Affero General Public License for more details.
-    
-    You should have received a copy of the GNU Affero General Public License
-    along with Zotero.  If not, see <http://www.gnu.org/licenses/>.
-    
-    ***** END LICENSE BLOCK *****
-*/
-
-
-/*
- * Zotero.getCollections(parent)
- *
- * Returns an array of all collections are children of a collection
- * as Zotero.Collection instances
- *
- * Takes parent collectionID as optional parameter;
- * by default, returns root collections
- */
-Zotero.getCollections = function(parent, recursive, libraryID) {
-	var toReturn = new Array();
-	
-	if (!parent) {
-		parent = null;
-	}
-	
-	var sql = "SELECT collectionID AS id, collectionName AS name FROM collections C "
-		+ "WHERE libraryID=? AND parentCollectionID " + (parent ? '=' + parent : 'IS NULL');
-	var children = Zotero.DB.query(sql, [libraryID ? libraryID : Zotero.Libraries.userLibraryID]);
-	
-	if (!children) {
-		Zotero.debug('No child collections of collection ' + parent, 5);
-		return toReturn;
-	}
-	
-	// Do proper collation sort
-	var collation = Zotero.getLocaleCollation();
-	children.sort(function (a, b) {
-		return collation.compareString(1, a.name, b.name);
-	});
-	
-	for (var i=0, len=children.length; i<len; i++) {
-		var obj = Zotero.Collections.get(children[i].id);
-		if (!obj) {
-			throw ('Collection ' + children[i].id + ' not found');
-		}
-		
-		toReturn.push(obj);
-		
-		// If recursive, get descendents
-		if (recursive) {
-			var desc = obj.getDescendents(false, 'collection');
-			for (var j in desc) {
-				var obj2 = Zotero.Collections.get(desc[j]['id']);
-				if (!obj2) {
-					throw ('Collection ' + desc[j] + ' not found');
-				}
-				
-				// TODO: This is a quick hack so that we can indent subcollections
-				// in the search dialog -- ideally collections would have a
-				// getLevel() method, but there's no particularly quick way
-				// of calculating that without either storing it in the DB or
-				// changing the schema to Modified Preorder Tree Traversal,
-				// and I don't know if we'll actually need it anywhere else.
-				obj2.level = desc[j].level;
-				
-				toReturn.push(obj2);
-			}
-		}
-	}
-	
-	return toReturn;
-}

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;
 }
 

test/tests/collectionsTest.js

@@ -0,0 +1,48 @@
+describe("Zotero.Collections", function () {
+	describe("#getByLibrary()", function () {
+		it("should get all root collections in a library", function* () {
+			var col1 = yield createDataObject('collection');
+			var col2 = yield createDataObject('collection');
+			var col3 = yield createDataObject('collection', { parentID: col2.id });
+			var cols = yield Zotero.Collections.getByLibrary(Zotero.Libraries.userLibraryID);
+			assert.isAbove(cols.length, 1);
+			assert.includeMembers(cols.map(col => col.id), [col1.id, col2.id]);
+			assert.ok(cols.every(col =>
+				col.libraryID == Zotero.Libraries.userLibraryID && !col.parentID
+			));
+		})
+		
+		it("should get all collections in a library in recursive mode", function* () {
+			var col1 = yield createDataObject('collection');
+			var col2 = yield createDataObject('collection');
+			var col3 = yield createDataObject('collection', { parentID: col2.id });
+			var cols = yield Zotero.Collections.getByLibrary(Zotero.Libraries.userLibraryID, true);
+			assert.isAbove(cols.length, 2);
+			assert.includeMembers(cols.map(col => col.id), [col1.id, col2.id, col3.id]);
+			assert.ok(cols.every(col => col.libraryID == Zotero.Libraries.userLibraryID));
+		})
+	})
+	
+	describe("#getByParent()", function () {
+		it("should get all direct subcollections of a library", function* () {
+			var col1 = yield createDataObject('collection');
+			var col2 = yield createDataObject('collection');
+			var col3 = yield createDataObject('collection', { parentID: col2.id });
+			assert.lengthOf((yield Zotero.Collections.getByParent(col1.id)), 0);
+			var cols = yield Zotero.Collections.getByParent(col2.id);
+			assert.lengthOf(cols, 1);
+			assert.sameMembers(cols.map(col => col.id), [col3.id]);
+		})
+		
+		it("should get all collections underneath a collection in recursive mode", function* () {
+			var col1 = yield createDataObject('collection');
+			var col2 = yield createDataObject('collection');
+			var col3 = yield createDataObject('collection', { parentID: col2.id });
+			var col4 = yield createDataObject('collection', { parentID: col3.id });
+			assert.lengthOf((yield Zotero.Collections.getByParent(col1.id)), 0);
+			var cols = yield Zotero.Collections.getByParent(col2.id, true);
+			assert.lengthOf(cols, 2);
+			assert.includeMembers(cols.map(col => col.id), [col3.id, col4.id]);
+		})
+	})
+})