From 3fbb17a2e6ca25927c324555beefe8718a492c12 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Wed, 8 May 2019 04:30:54 -0400
Subject: [PATCH] Properly upload local changes after automatic conflict
 resolution

If an object changed on both sides and the changes were either
non-conflicting or identical but there were other local changes, the
local object was incorrectly being marked as synced, causing it not to
be uploaded until it was next modified locally.
---
 chrome/content/zotero/xpcom/sync/syncLocal.js | 21 ++++++++--
 test/tests/syncLocalTest.js                   | 38 ++++++++++++++++++-
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js
index 947b5d6de0..31142cebb6 100644
--- a/chrome/content/zotero/xpcom/sync/syncLocal.js
+++ b/chrome/content/zotero/xpcom/sync/syncLocal.js
@@ -886,9 +886,11 @@ Zotero.Sync.Data.Local = {
 									Zotero.debug("No remote changes to apply to local "
 										+ objectType + " " + obj.libraryKey);
 									saveOptions.skipData = true;
-									// If local object is different but we ignored the changes
-									// (e.g., ISBN hyphenation), save as unsynced. Since we're skipping
-									// data, the local version won't be overwritten.
+									// If either there were additional local changes after cancelling
+									// out equivalent changes on both sides or the local object was
+									// different but we ignored the changes (e.g., ISBN hyphenation),
+									// keep as unsynced. In the latter case, since we're skipping
+									// data, the local fields won't be overwritten.
 									if (result.localChanged) {
 										saveOptions.saveAsUnsynced = true;
 									}
@@ -940,6 +942,10 @@ Zotero.Sync.Data.Local = {
 									jsonDataLocal[x] = jsonData[x];
 								})
 								jsonObject.data = jsonDataLocal;
+								// If there were additional local changes, keep as unsynced
+								if (result.localChanged) {
+									saveOptions.saveAsUnsynced = true;
+								}
 							}
 						}
 						// Object doesn't exist locally
@@ -1535,6 +1541,7 @@ Zotero.Sync.Data.Local = {
 					let creators2 = c2.value;
 					if (creators1.length == creators2.length
 							&& creators1.every((c, index) => Zotero.Creators.equals(c, creators2[index]))) {
+						changeset1.splice(i--, 1);
 						changeset2.splice(j--, 1);
 						continue;
 					}
@@ -1551,6 +1558,7 @@ Zotero.Sync.Data.Local = {
 						|| (c1.op == 'member-remove' && c2.op == 'member-remove')
 						|| (c1.op == 'property-member-add' && c2.op == 'property-member-add')
 						|| (c1.op == 'property-member-remove' && c2.op == 'property-member-remove')) {
+					changeset1.splice(i--, 1);
 					changeset2.splice(j--, 1);
 					continue;
 				}
@@ -1558,6 +1566,7 @@ Zotero.Sync.Data.Local = {
 				// If both sides have values, see if they're the same, and if so remove the
 				// second one
 				if (c1.op != 'delete' && c2.op != 'delete' && c1.value === c2.value) {
+					changeset1.splice(i--, 1);
 					changeset2.splice(j--, 1);
 					continue;
 				}
@@ -1570,6 +1579,7 @@ Zotero.Sync.Data.Local = {
 				}
 				
 				// Conflict
+				changeset1.splice(i--, 1);
 				changeset2.splice(j--, 1);
 				conflicts.push([c1, c2]);
 			}
@@ -1577,7 +1587,10 @@ Zotero.Sync.Data.Local = {
 		
 		return {
 			changes: changeset2,
-			conflicts
+			conflicts,
+			// If there were unique local changes, we need to know that so the item can be kept as
+			// unsynced
+			localChanged: !!changeset1.length
 		};
 	},
 	
diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js
index f9ef47ab02..783a1ab03a 100644
--- a/test/tests/syncLocalTest.js
+++ b/test/tests/syncLocalTest.js
@@ -526,7 +526,43 @@ describe("Zotero.Sync.Data.Local", function() {
 			assert.equal(obj.version, 10);
 			assert.equal(obj.getField('title'), changedTitle);
 			assert.equal(obj.getField('place'), changedPlace);
-		})
+			// Item should be marked as unsynced so the local changes are uploaded
+			assert.isFalse(obj.synced);
+		});
+		
+		it("should keep local item changes while ignoring matching remote changes", async function () {
+			var libraryID = Zotero.Libraries.userLibraryID;
+			
+			var type = 'item';
+			let obj = await createDataObject(type, { version: 5 });
+			let data = obj.toJSON();
+			await Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [data]);
+			
+			// Change local title and place
+			await modifyDataObject(obj)
+			var changedTitle = obj.getField('title');
+			var changedPlace = 'New York';
+			obj.setField('place', changedPlace);
+			await obj.saveTx();
+			
+			// Create remote version without title but with changed place
+			data.key = obj.key;
+			data.version = 10;
+			data.place = changedPlace;
+			let json = {
+				key: obj.key,
+				version: 10,
+				data: data
+			};
+			await Zotero.Sync.Data.Local.processObjectsFromJSON(
+				type, libraryID, [json], { stopOnError: true }
+			);
+			assert.equal(obj.version, 10);
+			assert.equal(obj.getField('title'), changedTitle);
+			assert.equal(obj.getField('place'), changedPlace);
+			// Item should be marked as unsynced so the local changes are uploaded
+			assert.isFalse(obj.synced);
+		});
 		
 		it("should save item with overriding local conflict as unsynced", async function () {
 			var libraryID = Zotero.Libraries.userLibraryID;