Better handling of remotely changed items in locally missing collections

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

@@ -794,6 +794,7 @@ Zotero.Sync.Data.Local = {
 				
 				// Skip objects with unmet dependencies
 				if (objectType == 'item' || objectType == 'collection') {
+					// Missing parent collection or item
 					let parentProp = 'parent' + objectType[0].toUpperCase() + objectType.substr(1);
 					let parentKey = jsonData[parentProp];
 					if (parentKey) {
@@ -815,17 +816,37 @@ Zotero.Sync.Data.Local = {
 						}
 					}
 					
-					/*if (objectType == 'item') {
+					// Missing collection -- this could happen if the collection was deleted
-						for (let j = 0; j < jsonData.collections.length; i++) {
+					// locally and an item in it was modified remotely
-							let parentKey = jsonData.collections[j];
+					if (objectType == 'item' && jsonData.collections) {
-							let parentCollection = Zotero.Collections.getByLibraryAndKey(
+						let error;
-								libraryID, parentKey, { noCache: true }
+						for (let key of jsonData.collections) {
-							);
+							let collection = Zotero.Collections.getByLibraryAndKey(libraryID, key);
-							if (!parentCollection) {
+							if (!collection) {
-								// ???
+								error = new Error(`Collection ${libraryID}/${key} not found `
+									+ `-- skipping item`);
+								error.name = "ZoteroMissingObjectError";
+								Zotero.debug(error.message);
+								results.push({
+									key: objectKey,
+									processed: false,
+									error,
+									retry: false
+								});
+								
+								// If the collection is in the delete log, the deletion will upload
+								// after downloads are done. Otherwise, we somehow missed
+								// downloading it and should add it to the queue to try again.
+								if (!(yield this.getDateDeleted('collection', libraryID, key))) {
+									yield this.addObjectsToSyncQueue('collection', libraryID, [key]);
+								}
+								break;
 							}
 						}
-					}*/
+						if (error) {
+							continue;
+						}
+					}
 				}
 				
 				// Errors have to be thrown in order to roll back the transaction, so catch those here

test/tests/syncEngineTest.js

@@ -2112,23 +2112,13 @@ describe("Zotero.Sync.Data.Engine", function () {
 			var library = Zotero.Libraries.userLibrary;
 			library.libraryVersion = lastLibraryVersion;
 			await library.saveTx();
-			({ engine, client, caller } = await setup({
+			({ engine, client, caller } = await setup());
-				stopOnError: false
-			}));
 			
 			// Create local deleted collection
 			var collection = await createDataObject('collection');
 			var collectionKey = collection.key;
 			await collection.eraseTx();
-			// Create item JSON
+			var itemKey = "AAAAAAAA";
-			var item = await createDataObject('item');
-			var itemResponseJSON = item.toResponseJSON();
-			// Add collection to remote item
-			itemResponseJSON.data.collections = [collectionKey];
-			var itemKey = item.key;
-			await item.eraseTx({
-				skipDeleteLog: true
-			});
 			
 			var headers = {
 				"Last-Modified-Version": newLibraryVersion
@@ -2151,7 +2141,14 @@ describe("Zotero.Sync.Data.Engine", function () {
 				url: `users/1/items?format=json&itemKey=${itemKey}&includeTrashed=1`,
 				status: 200,
 				headers,
-				json: [itemResponseJSON]
+				json: [
+					makeItemJSON({
+						key: itemKey,
+						version: newLibraryVersion,
+						itemType: "book",
+						collections: [collectionKey]
+					})
+				]
 			});
 			
 			await engine._startDownload();
@@ -2159,6 +2156,67 @@ describe("Zotero.Sync.Data.Engine", function () {
 			// Item should be skipped and added to queue, which will allow collection deletion to upload
 			var keys = await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', library.id);
 			assert.sameMembers(keys, [itemKey]);
+			
+			// Collection should not be in sync queue
+			assert.lengthOf(
+				await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('collection', library.id), 0
+			);
+		});
+		
+		
+		it("should handle new remote item referencing locally missing collection", async function () {
+			var lastLibraryVersion = 5;
+			var newLibraryVersion = 6;
+			var library = Zotero.Libraries.userLibrary;
+			library.libraryVersion = lastLibraryVersion;
+			await library.saveTx();
+			({ engine, client, caller } = await setup());
+			
+			var collectionKey = 'AAAAAAAA';
+			var itemKey = 'BBBBBBBB'
+			
+			var headers = {
+				"Last-Modified-Version": newLibraryVersion
+			};
+			setDefaultResponses({
+				lastLibraryVersion,
+				libraryVersion: newLibraryVersion
+			});
+			setResponse({
+				method: "GET",
+				url: `users/1/items?format=versions&since=${lastLibraryVersion}&includeTrashed=1`,
+				status: 200,
+				headers,
+				json: {
+					[itemKey]: newLibraryVersion
+				}
+			});
+			setResponse({
+				method: "GET",
+				url: `users/1/items?format=json&itemKey=${itemKey}&includeTrashed=1`,
+				status: 200,
+				headers,
+				json: [
+					makeItemJSON({
+						key: itemKey,
+						version: newLibraryVersion,
+						itemType: "book",
+						collections: [collectionKey]
+					})
+				]
+			});
+			
+			await engine._startDownload();
+			
+			// Item should be skipped and added to queue
+			var keys = await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', library.id);
+			assert.sameMembers(keys, [itemKey]);
+			
+			// Collection should be in queue
+			assert.sameMembers(
+				await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('collection', library.id),
+				[collectionKey]
+			);
 		});
 		
 		
@@ -2168,9 +2226,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 			var library = Zotero.Libraries.userLibrary;
 			library.libraryVersion = lastLibraryVersion;
 			await library.saveTx();
-			({ engine, client, caller } = await setup({
+			({ engine, client, caller } = await setup());
-				stopOnError: false
-			}));
 			
 			// Create local deleted collection and item
 			var collection = await createDataObject('collection');
@@ -2207,20 +2263,6 @@ describe("Zotero.Sync.Data.Engine", function () {
 				json: [itemResponseJSON]
 			});
 			
-			// This will trigger a conflict for a remote item that references a collection that doesn't
-			// exist. The collection should be removed for display in the CR window, and then the save
-			// should fail and the item should be added to the queue so that the collection deletion
-			// can upload.
-			waitForWindow('chrome://zotero/content/merge.xul', function (dialog) {
-				var doc = dialog.document;
-				var wizard = doc.documentElement;
-				var mergeGroup = wizard.getElementsByTagName('zoteromergegroup')[0];
-				
-				// 1 (accept remote)
-				//assert.equal(mergeGroup.leftpane.getAttribute('selected'), 'true');
-				mergeGroup.rightpane.click();
-				wizard.getButton('finish').click();
-			});
 			await engine._startDownload();
 			
 			// Item should be skipped and added to queue, which will allow collection deletion to upload