commit f82fb89e1cf59cb4aae68ede61bff37f62f01327
parent 12e369b2b683b1d7885fd3f84f3c685fc579dc5b
Author: Dan Stillman <dstillman@zotero.org>
Date: Mon, 25 Apr 2016 20:16:31 -0400
Sort parent collections and items first in uploads
Closes #972
Diffstat:
5 files changed, 230 insertions(+), 8 deletions(-)
diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js
@@ -717,8 +717,8 @@ Zotero.Collection.prototype.toJSON = function (options = {}) {
* nodes instead of flat array
* @param {String} [type] 'item', 'collection', or NULL for both
* @param {Boolean} [includeDeletedItems=false] Include items in Trash
- * @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'
+ * @return {Object[]} - An array of objects with 'id', 'key', 'type' ('item' or 'collection'),
+ * 'parent', and, if collection, 'name' and the nesting 'level'
*/
Zotero.Collection.prototype.getDescendents = function (nested, type, includeDeletedItems, level) {
if (!this.id) {
diff --git a/chrome/content/zotero/xpcom/data/collections.js b/chrome/content/zotero/xpcom/data/collections.js
@@ -157,6 +157,69 @@ Zotero.Collections = function() {
}
+ /**
+ * Sort an array of collectionIDs from top-level to deepest
+ *
+ * Order within each level is undefined.
+ *
+ * This is used to sort higher-level collections first in upload JSON, since otherwise the API
+ * would reject lower-level collections for having missing parents.
+ */
+ this.sortByLevel = function (ids) {
+ let levels = {};
+
+ // Get objects from ids
+ let objs = {};
+ ids.forEach(id => objs[id] = Zotero.Collections.get(id));
+
+ // Get top-level collections
+ let top = ids.filter(id => !objs[id].parentID);
+ levels["0"] = top.slice();
+ ids = Zotero.Utilities.arrayDiff(ids, top);
+
+ // For each collection in list, walk up its parent tree. If a parent is present in the
+ // list of ids, add it to the appropriate level bucket and remove it.
+ while (ids.length) {
+ let tree = [ids[0]];
+ let keep = [ids[0]];
+ let id = ids.shift();
+ while (true) {
+ let c = Zotero.Collections.get(id);
+ let parentID = c.parentID;
+ if (!parentID) {
+ break;
+ }
+ tree.push(parentID);
+ // If parent is in list, remove it
+ let pos = ids.indexOf(parentID);
+ if (pos != -1) {
+ keep.push(parentID);
+ ids.splice(pos, 1);
+ }
+ id = parentID;
+ }
+ let level = tree.length - 1;
+ for (let i = 0; i < tree.length; i++) {
+ let currentLevel = level - i;
+ for (let j = 0; j < keep.length; j++) {
+ if (tree[i] != keep[j]) continue;
+
+ if (!levels[currentLevel]) {
+ levels[currentLevel] = [];
+ }
+ levels[currentLevel].push(keep[j]);
+ }
+ }
+ }
+
+ var orderedIDs = [];
+ for (let level in levels) {
+ orderedIDs = orderedIDs.concat(levels[level]);
+ }
+ return orderedIDs;
+ };
+
+
this._loadChildCollections = Zotero.Promise.coroutine(function* (libraryID, ids, idSQL) {
var sql = "SELECT C1.collectionID, C2.collectionID AS childCollectionID "
+ "FROM collections C1 LEFT JOIN collections C2 ON (C1.collectionID=C2.parentCollectionID) "
diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js
@@ -228,14 +228,25 @@ Zotero.Sync.Data.Local = {
*/
getUnsynced: Zotero.Promise.coroutine(function* (objectType, libraryID) {
var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
- var sql = "SELECT " + objectsClass.idColumn + " FROM " + objectsClass.table
- + " WHERE libraryID=? AND synced=0";
+ var sql = "SELECT O." + objectsClass.idColumn + " FROM " + objectsClass.table + " O";
+ if (objectType == 'item') {
+ sql += " LEFT JOIN itemAttachments IA USING (itemID) "
+ + "LEFT JOIN itemNotes INo ON (O.itemID=INo.itemID) ";
+ }
+ sql += " WHERE libraryID=? AND synced=0";
+ // Sort child items last
+ if (objectType == 'item') {
+ sql += " ORDER BY COALESCE(IA.parentItemID, INo.parentItemID)";
+ }
- // TODO: RETRIEVE PARENT DOWN? EVEN POSSIBLE?
- // items via parent
- // collections via getDescendents?
+ var ids = yield Zotero.DB.columnQueryAsync(sql, [libraryID]);
- return Zotero.DB.columnQueryAsync(sql, [libraryID]);
+ // Sort descendent collections last
+ if (objectType == 'collection') {
+ ids = Zotero.Collections.sortByLevel(ids);
+ }
+
+ return ids;
}),
diff --git a/test/tests/collectionsTest.js b/test/tests/collectionsTest.js
@@ -72,4 +72,48 @@ describe("Zotero.Collections", function () {
assert.notInstanceOf(collection, Zotero.Feed);
});
});
+
+
+ describe("#sortByLevel()", function () {
+ it("should return collections sorted from top-level to deepest", function* () {
+ // - A
+ // - B
+ // - C
+ // - D
+ // - E
+ // - F
+ // - G
+ // - H
+ // - I
+
+ // Leave out B and G
+ // Order should be {A, E}, {D, F}, {C, I}, {H} (internal order is undefined)
+
+ var check = function (arr) {
+ assert.sameMembers(arr.slice(0, 2), [c1.id, c5.id]);
+ assert.sameMembers(arr.slice(2, 4), [c4.id, c6.id]);
+ assert.sameMembers(arr.slice(4, 6), [c3.id, c9.id]);
+ assert.equal(arr[6], c8.id);
+ };
+
+ var c1 = yield createDataObject('collection', { "name": "A" });
+ var c2 = yield createDataObject('collection', { "name": "B", parentID: c1.id });
+ var c3 = yield createDataObject('collection', { "name": "C", parentID: c2.id });
+ var c4 = yield createDataObject('collection', { "name": "D", parentID: c1.id });
+ var c5 = yield createDataObject('collection', { "name": "E" });
+ var c6 = yield createDataObject('collection', { "name": "F", parentID: c5.id });
+ var c7 = yield createDataObject('collection', { "name": "G", parentID: c6.id });
+ var c8 = yield createDataObject('collection', { "name": "H", parentID: c7.id });
+ var c9 = yield createDataObject('collection', { "name": "I", parentID: c6.id });
+
+ var arr = Zotero.Collections.sortByLevel([c1, c3, c4, c5, c6, c8, c9].map(c => c.id));
+ //Zotero.debug(arr.map(id => Zotero.Collections.get(id).name));
+ check(arr);
+
+ // Check reverse order
+ arr = Zotero.Collections.sortByLevel([c1, c3, c4, c5, c6, c8, c9].reverse().map(c => c.id));
+ //Zotero.debug(arr.map(id => Zotero.Collections.get(id).name));
+ check(arr);
+ });
+ });
})
diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
@@ -501,6 +501,110 @@ describe("Zotero.Sync.Data.Engine", function () {
}
})
+
+ it("should upload child item after parent item", function* () {
+ ({ engine, client, caller } = yield setup());
+
+ var libraryID = Zotero.Libraries.userLibraryID;
+ var lastLibraryVersion = 5;
+ yield Zotero.Libraries.setVersion(libraryID, lastLibraryVersion);
+
+ // Create top-level note, book, and child note
+ var item1 = new Zotero.Item('note');
+ item1.setNote('A');
+ yield item1.saveTx();
+ var item2 = yield createDataObject('item');
+ var item3 = new Zotero.Item('note');
+ item3.parentItemID = item2.id;
+ item3.setNote('B');
+ yield item3.saveTx();
+ // Move note under parent
+ item1.parentItemID = item2.id;
+ yield item1.saveTx();
+ var handled = false;
+
+ server.respond(function (req) {
+ if (req.method == "POST" && req.url == baseURL + "users/1/items") {
+ let json = JSON.parse(req.requestBody);
+ assert.lengthOf(json, 3);
+ assert.equal(json[0].key, item2.key);
+ assert.equal(json[1].key, item1.key);
+ assert.equal(json[2].key, item3.key);
+ handled = true;
+ req.respond(
+ 200,
+ {
+ "Content-Type": "application/json",
+ "Last-Modified-Version": ++lastLibraryVersion
+ },
+ JSON.stringify({
+ successful: {
+ "0": item2.toResponseJSON(),
+ "1": item1.toResponseJSON(),
+ "2": item3.toResponseJSON()
+ },
+ unchanged: {},
+ failed: {}
+ })
+ );
+ return;
+ }
+ });
+
+ yield engine.start();
+ assert.isTrue(handled);
+ });
+
+
+ it("should upload child collection after parent collection", function* () {
+ ({ engine, client, caller } = yield setup());
+
+ var libraryID = Zotero.Libraries.userLibraryID;
+ var lastLibraryVersion = 5;
+ yield Zotero.Libraries.setVersion(libraryID, lastLibraryVersion);
+
+ var collection1 = yield createDataObject('collection');
+ var collection2 = yield createDataObject('collection');
+ var collection3 = yield createDataObject('collection', { parentID: collection2.id });
+ // Move collection under the other
+ collection1.parentID = collection2.id;
+ yield collection1.saveTx();
+
+ var handled = false;
+
+ server.respond(function (req) {
+ if (req.method == "POST" && req.url == baseURL + "users/1/collections") {
+ let json = JSON.parse(req.requestBody);
+ assert.lengthOf(json, 3);
+ assert.equal(json[0].key, collection2.key);
+ assert.equal(json[1].key, collection1.key);
+ assert.equal(json[2].key, collection3.key);
+ handled = true;
+ req.respond(
+ 200,
+ {
+ "Content-Type": "application/json",
+ "Last-Modified-Version": ++lastLibraryVersion
+ },
+ JSON.stringify({
+ successful: {
+ "0": collection2.toResponseJSON(),
+ "1": collection1.toResponseJSON(),
+ "2": collection3.toResponseJSON()
+ },
+ unchanged: {},
+ failed: {}
+ })
+ );
+ return;
+ }
+ });
+
+ yield engine.start();
+ assert.isTrue(handled);
+ });
+
+
it("should upload synced storage properties", function* () {
({ engine, client, caller } = yield setup());
