diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index 04a070a60..b01096683 100644 --- 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} - 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 index 9d4d4605e..7e220e225 100644 --- 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 index efdfda2d6..b81666c5c 100644 --- 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 index 43f277abd..846ad92e4 100644 --- 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 index 37ee877f0..f5c724049 100644 --- 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());