Save correct data to cache when merging non-conflicting sync changes

When non-conflicting changes were automatically merged, the local object
would be correctly marked as unsynced, but the merged object rather than
the remote object would be saved to the sync cache. When the object was
then uploaded, it matched the cache version exactly, so an empty patch
object (other than an unchanged dateModified, which is always included)
would be uploaded and the local change wouldn't make it to the server.
The empty patch would result in an 'unchanged' response, which would
cause the empty patch object to be saved to the sync cache (which is a
bug that I'll fix separately). If the local object was modified again,
the patch would include all fields (since the cache object was empty)
and the local change would be uploaded, but there could also be
unnecessary conflicts due to it looking like all local fields had been
modified.

This patch causes the remote object to be saved to the sync cache
instead, so the local change looks like a local edit and is correctly
uploaded.

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

@@ -944,6 +944,13 @@ Zotero.Sync.Data.Local = {
 								Zotero.debug(`Applying remote changes to ${objectType} `
 									+ obj.libraryKey);
 								Zotero.debug(result.changes);
+								// If there were local changes as well, keep object as unsynced and
+								// save the remote version to the sync cache rather than the merged
+								// version
+								if (result.localChanged) {
+									saveOptions.saveAsUnsynced = true;
+									saveOptions.cacheObject = jsonObject.data;
+								}
 								Zotero.DataObjectUtilities.applyChanges(
 									jsonDataLocal, result.changes
 								);
@@ -955,10 +962,6 @@ 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

test/tests/syncLocalTest.js

@@ -526,35 +526,54 @@ describe("Zotero.Sync.Data.Local", function() {
 			}
 		})
 		
-		it("should keep local item changes while applying non-conflicting remote changes", function* () {
+		it("should keep local item changes while applying non-conflicting remote changes", async function () {
 			var libraryID = Zotero.Libraries.userLibraryID;
 			
-			var type = 'item';
-			let obj = yield createDataObject(type, { version: 5 });
-			let data = obj.toJSON();
-			yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [data]);
+			let item = await createDataObject('item', { version: 5 });
+			let data = item.toJSON();
+			await Zotero.Sync.Data.Local.saveCacheObjects('item', libraryID, [data]);
 			
 			// Change local title
-			yield modifyDataObject(obj)
-			var changedTitle = obj.getField('title');
+			await modifyDataObject(item)
+			item.setTags([
+				{ tag: 'A' }
+			]);
+			await item.saveTx();
+			var changedTitle = item.getField('title');
 			
 			// Create remote version without title but with changed place
-			data.key = obj.key;
+			data.key = item.key;
 			data.version = 10;
+			data.tags = [
+				{
+					tag: 'B'
+				}
+			]
 			var changedPlace = data.place = 'New York';
 			let json = {
-				key: obj.key,
+				key: item.key,
 				version: 10,
-				data: data
+				data
 			};
-			yield Zotero.Sync.Data.Local.processObjectsFromJSON(
-				type, libraryID, [json], { stopOnError: true }
+			var results = await Zotero.Sync.Data.Local.processObjectsFromJSON(
+				'item', libraryID, [json], { stopOnError: true }
 			);
-			assert.equal(obj.version, 10);
-			assert.equal(obj.getField('title'), changedTitle);
-			assert.equal(obj.getField('place'), changedPlace);
+			assert.isTrue(results[0].processed);
+			assert.isUndefined(results[0].changes);
+			assert.isUndefined(results[0].conflicts);
+			assert.equal(item.version, 10);
+			assert.equal(item.getField('title'), changedTitle);
+			assert.equal(item.getField('place'), changedPlace);
+			assert.sameDeepMembers(item.getTags(), [{ tag: 'A' }, { tag: 'B' }]);
 			// Item should be marked as unsynced so the local changes are uploaded
-			assert.isFalse(obj.synced);
+			assert.isFalse(item.synced);
+			// Sync cache should match remote
+			var cacheJSON = await Zotero.Sync.Data.Local.getCacheObject(
+				'item', libraryID, data.key, data.version
+			);
+			assert.notProperty(cacheJSON.data, 'title');
+			assert.equal(cacheJSON.data.place, data.place);
+			assert.sameDeepMembers(cacheJSON.data.tags, data.tags);
 		});
 		
 		it("should keep local item changes while ignoring matching remote changes", async function () {