Sync logic improvements

- Cancel sync when cancelling conflict resolution window
- Don't try to upload unsynced objects if present in sync queue

chrome/content/zotero/xpcom/sync/syncEngine.js

@@ -71,6 +71,12 @@ Zotero.Sync.Data.Engine = function (options) {
 	});
 };
 
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CONTINUE = 1;
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD = 2;
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_NO_CHANGES_TO_UPLOAD = 3;
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_RESTART = 4;
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CANCEL = 5;
+
 Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_SUCCESS = 1;
 Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_NOTHING_TO_UPLOAD = 2;
 Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_LIBRARY_CONFLICT = 3;
@@ -113,7 +119,8 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* ()
 	sync:
 	while (true) {
 		let uploadResult = yield this._startUpload();
-		Zotero.debug("UPLOAD RESULT WITH " + uploadResult);
+		let downloadResult;
+		Zotero.debug("Upload result is " + uploadResult, 4);
 		
 		switch (uploadResult) {
 		// If upload succeeded, we're done
@@ -132,16 +139,21 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* ()
 			yield this._fullSync();
 			break;
 		
-		// If conflict, start at beginning with downloads
 		case this.UPLOAD_RESULT_NOTHING_TO_UPLOAD:
-			let localChanges = yield this._startDownload();
+			downloadResult = yield this._startDownload();
-			if (!localChanges) {
+			Zotero.debug("Download result is " + downloadResult, 4);
+			if (downloadResult == this.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD) {
+				break;
+			}
+			break sync;
+		
+		// If conflict, start at beginning with downloads
+		case this.UPLOAD_RESULT_LIBRARY_CONFLICT:
+			downloadResult = yield this._startDownload();
+			Zotero.debug("Download result is " + downloadResult, 4);
+			if (downloadResult == this.DOWNLOAD_RESULT_CANCEL) {
 				break sync;
 			}
-			break;
-			
-		case this.UPLOAD_RESULT_LIBRARY_CONFLICT:
-			yield this._startDownload();
 			
 			if (!gen) {
 				var gen = Zotero.Utilities.Internal.delayGenerator(
@@ -183,7 +195,7 @@ Zotero.Sync.Data.Engine.prototype.stop = function () {
 /**
  * Download updated objects from API and save to local cache
  *
- * @return {Boolean} True if an upload is needed, false otherwise
+ * @return {Promise<Integer>} - A download result code (this.DOWNLOAD_RESULT_*)
  */
 Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(function* () {
 	var localChanges = false;
@@ -222,9 +234,12 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 						top: true
 					}
 				);
-				if (result == -1) {
+				if (result == this.DOWNLOAD_RESULT_RESTART) {
 					continue loop;
 				}
+				else if (result == this.DOWNLOAD_RESULT_CANCEL) {
+					return this.DOWNLOAD_RESULT_CANCEL;
+				}
 			}
 			
 			let result = yield this._downloadUpdatedObjects(
@@ -233,15 +248,18 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 				lastLibraryVersion,
 				gen
 			);
-			if (result == -1) {
+			if (result == this.DOWNLOAD_RESULT_RESTART) {
 				continue loop;
 			}
+			else if (result == this.DOWNLOAD_RESULT_CANCEL) {
+				return this.DOWNLOAD_RESULT_CANCEL;
+			}
 		}
 		
 		//
 		// Get deleted objects
 		//
-		results = yield this.apiClient.getDeleted(
+		let results = yield this.apiClient.getDeleted(
 			this.library.libraryType,
 			this.libraryTypeID,
 			libraryVersion
@@ -327,7 +345,10 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 						return 0;
 					});
 					var mergeData = Zotero.Sync.Data.Local.showConflictResolutionWindow(conflicts);
-					if (mergeData) {
+					if (!mergeData) {
+						Zotero.debug("Cancelling sync");
+						return this.DOWNLOAD_RESULT_CANCEL;
+					}
 					let concurrentObjects = 50;
 					yield Zotero.Utilities.Internal.forEachChunkAsync(
 						mergeData,
@@ -352,7 +373,6 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 						}.bind(this)
 					);
 				}
-				}
 				
 				if (toDelete.length) {
 					yield Zotero.DB.executeTransaction(function* () {
@@ -377,7 +397,9 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 		yield Zotero.Libraries.setVersion(this.libraryID, lastLibraryVersion);
 	}
 	
-	return localChanges;
+	return localChanges
+		? this.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD
+		: this.DOWNLOAD_RESULT_NO_CHANGES_TO_UPLOAD;
 });
 
 
@@ -420,8 +442,9 @@ Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(f
 
 
 /**
- * @return {Boolean|Integer} - True if objects downloaded, false if none, or -1 to restart sync
+ * Get versions of objects updated remotely since the last sync time and kick off object downloading
- *     if library version has changed
+ *
+ * @return {Promise<Integer>} - A download result code (this.DOWNLOAD_RESULT_*)
  */
 Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.coroutine(function* (objectType, libraryVersion, lastLibraryVersion, delayGenerator, options = {}) {
 	var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType);
@@ -456,7 +479,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
 		if (!keepGoing) {
 			throw new Error("Could not update " + this.library.name + " -- library in use");
 		}
-		return -1;
+		return this.DOWNLOAD_RESULT_RESTART;
 	}
 	
 	
@@ -511,14 +534,19 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
 	
 	if (!keys.length) {
 		Zotero.debug(`No ${objectTypePlural} to download`);
-		return false;
+		return this.DOWNLOAD_RESULT_CONTINUE;
 	}
 	
-	yield this._downloadObjects(objectType, keys);
+	return this._downloadObjects(objectType, keys);
-	return true;
 });
 
 
+/**
+ * Download data for specified objects from the API and run processing on them, and show the conflict
+ * resolution window if necessary
+ *
+ * @return {Promise<Integer>} - A download result code (this.DOWNLOAD_RESULT_*)
+ */
 Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) {
 	var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType);
 	
@@ -653,8 +681,13 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 		);
 		// Keys can be unprocessed if conflict resolution is cancelled
 		let keys = results.filter(x => x.processed).map(x => x.key);
+		if (!keys.length) {
+			return this.DOWNLOAD_RESULT_CANCEL;
+		}
 		yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, keys);
 	}
+	
+	return this.DOWNLOAD_RESULT_CONTINUE;
 });
 
 
@@ -693,9 +726,18 @@ Zotero.Sync.Data.Engine.prototype._startUpload = Zotero.Promise.coroutine(functi
 	// Get unsynced local objects for each object type
 	for (let objectType of Zotero.DataObjectUtilities.getTypesForLibrary(this.libraryID)) {
 		let objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType);
+		let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
 		
 		// New/modified objects
 		let ids = yield Zotero.Sync.Data.Local.getUnsynced(objectType, this.libraryID);
+		
+		// Skip objects in sync queue, because they might have unresolved conflicts.
+		// The queue only has keys, so we have to convert to keys and back.
+		let unsyncedKeys = ids.map(id => objectsClass.getLibraryAndKeyFromID(id).key);
+		let queueKeys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, this.libraryID);
+		unsyncedKeys = Zotero.Utilities.arrayDiff(unsyncedKeys, queueKeys);
+		ids = unsyncedKeys.map(key => objectsClass.getIDFromLibraryAndKey(this.libraryID, key));
+		
 		if (ids.length) {
 			Zotero.debug(ids.length + " "
 				+ (ids.length == 1 ? objectType : objectTypePlural)

test/tests/syncEngineTest.js

@@ -1543,8 +1543,9 @@ describe("Zotero.Sync.Data.Engine", function () {
 		})
 		
 		it("should handle cancellation of conflict resolution window", function* () {
-			var userLibraryID = Zotero.Libraries.userLibraryID;
+			var library = Zotero.Libraries.userLibrary;
-			yield Zotero.Libraries.setVersion(userLibraryID, 5);
+			library.libraryVersion = 5;
+			yield library.saveTx();
 			({ engine, client, caller } = yield setup());
 			
 			var item = yield createDataObject('item');
@@ -1630,17 +1631,124 @@ describe("Zotero.Sync.Data.Engine", function () {
 				var wizard = doc.documentElement;
 				wizard.getButton('cancel').click();
 			})
-			yield engine._startDownload();
+			var downloadResult = yield engine._startDownload();
+			assert.equal(downloadResult, engine.DOWNLOAD_RESULT_CANCEL);
 			
 			// Non-conflicted item should be saved
-			assert.ok(Zotero.Items.getIDFromLibraryAndKey(userLibraryID, "AAAAAAAA"));
+			assert.ok(Zotero.Items.getIDFromLibraryAndKey(library.id, "AAAAAAAA"));
 			
 			// Conflicted item should be skipped and in queue
 			assert.isFalse(Zotero.Items.exists(itemID));
-			var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', userLibraryID);
+			var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', library.id);
 			assert.sameMembers(keys, [itemKey]);
+			
+			// Library version should not have advanced
+			assert.equal(library.libraryVersion, 5);
 		});
+		
+		
+		/**
+		 * The CR window for remote deletions is triggered separately, so test separately
+		 */
+		it("should handle cancellation of remote deletion conflict resolution window", function* () {
+			var library = Zotero.Libraries.userLibrary;
+			library.libraryVersion = 5;
+			yield library.saveTx();
+			({ engine, client, caller } = yield setup());
+			
+			// Create local unsynced items
+			var item = createUnsavedDataObject('item');
+			item.setField('title', 'A');
+			item.synced = false;
+			var itemID1 = yield item.saveTx();
+			var itemKey1 = item.key;
+			
+			item = createUnsavedDataObject('item');
+			item.setField('title', 'B');
+			item.synced = false;
+			var itemID2 = yield item.saveTx();
+			var itemKey2 = item.key;
+			
+			var headers = {
+				"Last-Modified-Version": 6
+			};
+			setResponse({
+				method: "GET",
+				url: "users/1/settings?since=5",
+				status: 200,
+				headers,
+				json: {}
+			});
+			setResponse({
+				method: "GET",
+				url: "users/1/collections?format=versions&since=5",
+				status: 200,
+				headers,
+				json: {}
+			});
+			setResponse({
+				method: "GET",
+				url: "users/1/searches?format=versions&since=5",
+				status: 200,
+				headers,
+				json: {}
+			});
+			setResponse({
+				method: "GET",
+				url: "users/1/items/top?format=versions&since=5&includeTrashed=1",
+				status: 200,
+				headers,
+				json: {}
+			});
+			setResponse({
+				method: "GET",
+				url: "users/1/items?format=versions&since=5&includeTrashed=1",
+				status: 200,
+				headers,
+				json: {}
+			});
+			setResponse({
+				method: "GET",
+				url: "users/1/deleted?since=5",
+				status: 200,
+				headers,
+				json: {
+					settings: [],
+					collections: [],
+					searches: [],
+					items: [itemKey1, itemKey2]
+				}
+			});
+			
+			waitForWindow('chrome://zotero/content/merge.xul', function (dialog) {
+				var doc = dialog.document;
+				var wizard = doc.documentElement;
+				wizard.getButton('cancel').click();
 			})
+			var downloadResult = yield engine._startDownload();
+			assert.equal(downloadResult, engine.DOWNLOAD_RESULT_CANCEL);
+			
+			// Conflicted items should still exists
+			assert.isTrue(Zotero.Items.exists(itemID1));
+			assert.isTrue(Zotero.Items.exists(itemID2));
+			
+			// Library version should not have advanced
+			assert.equal(library.libraryVersion, 5);
+		});
+	});
+	
+	
+	describe("#_startUpload()", function () {
+		it("shouldn't upload unsynced objects if present in sync queue", function* () {
+			({ engine, client, caller } = yield setup());
+			var libraryID = Zotero.Libraries.userLibraryID;
+			var objectType = 'item';
+			var obj = yield createDataObject(objectType);
+			yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(objectType, libraryID, [obj.key]);
+			var result = yield engine._startUpload();
+			assert.equal(result, engine.UPLOAD_RESULT_NOTHING_TO_UPLOAD);
+		});
+	});
 	
 	
 	describe("Conflict Resolution", function () {
@@ -1852,6 +1960,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 			assert.lengthOf(keys, 0);
 		})
 		
+		// Note: Conflicts with remote deletions are handled in _startDownload()
 		it("should handle local item deletion, keeping deletion", function* () {
 			var libraryID = Zotero.Libraries.userLibraryID;
 			({ engine, client, caller } = yield setup());