From 4a30dd2e4f30a9930833434c50bdabd3dbf07428 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 21 Apr 2023 06:29:39 -0400
Subject: [PATCH] Fix potential sync error after Replace Online Library in
 group

https://forums.zotero.org/discussion/104431/syncing-problem

Replace Online Library can upload annotations created by others in a
group library, so if the upload resulted in a local write, "Cannot edit
item in library" was thrown, since annotations by others aren't
writable. This should've only been a problem if the uploaded data was
actually modified by the server, but we were also checking whether
objects were editable before checking if they had actually changed, so
it would happen for any upload of another person's annotation.

This fixes the order of checks when saving objects and makes an
edit-check exception for saving uploaded data for group annotations.
---
 .../content/zotero/xpcom/data/dataObject.js   | 10 ++--
 .../content/zotero/xpcom/sync/syncEngine.js   | 10 +++-
 test/tests/syncEngineTest.js                  | 49 +++++++++++++++++++
 3 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js
index ae94a397c7..45bf86e7e1 100644
--- a/chrome/content/zotero/xpcom/data/dataObject.js
+++ b/chrome/content/zotero/xpcom/data/dataObject.js
@@ -987,6 +987,11 @@ Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env)
 	
 	env.isNew = !this.id;
 	
+	if (!this.hasChanged()) {
+		Zotero.debug(this._ObjectType + ' ' + this.id + ' has not changed', 4);
+		return false;
+	}
+	
 	if (!env.options.skipEditCheck) {
 		if (!this.isEditable()) {
 			throw new Error("Cannot edit " + this._objectType + " in library "
@@ -999,11 +1004,6 @@ Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env)
 		throw new Error("Cannot add " + this._objectType + " to a " + targetLib.libraryType + " library");
 	}
 	
-	if (!this.hasChanged()) {
-		Zotero.debug(this._ObjectType + ' ' + this.id + ' has not changed', 4);
-		return false;
-	}
-	
 	// Undo registerObject() on failure
 	if (env.isNew) {
 		var func = function () {
diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js
index 8762e2bade..07e6c9a108 100644
--- a/chrome/content/zotero/xpcom/sync/syncEngine.js
+++ b/chrome/content/zotero/xpcom/sync/syncEngine.js
@@ -1331,7 +1331,15 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func
 						skipSyncedUpdate: true,
 						// We want to minimize the times when server writes actually result in local
 						// updates, but when they do, don't update the user-visible timestamp
-						skipDateModifiedUpdate: true
+						skipDateModifiedUpdate: true,
+						// Replace Online Library can reupload annotations created by others in a
+						// group library, and the server could theoretically override a value, so
+						// allow updating local annotations even if they were created by someone
+						// else. There might be other cases where this is necessary, but let's start
+						// with this.
+						skipEditCheck: objectType == 'item'
+							&& this.library.isGroup
+							&& toSave[i].isAnnotation()
 					});
 				}
 				if (this.library.libraryVersion == this.library.storageVersion) {
diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
index d867b37075..2565873338 100644
--- a/test/tests/syncEngineTest.js
+++ b/test/tests/syncEngineTest.js
@@ -3049,6 +3049,55 @@ describe("Zotero.Sync.Data.Engine", function () {
 		});
 		
 		
+		it("should allow applying remotely saved version to local annotation in group library", async function () {
+			var group = await createGroup({
+				libraryVersion: 5
+			});
+			var libraryID = group.libraryID;
+			({ engine, client, caller } = await setup({ libraryID }));
+			
+			var createdByUserID = 2352512;
+			await Zotero.Users.setName(createdByUserID, 'user');
+			
+			var attachment = await importFileAttachment('test.pdf', { libraryID });
+			attachment.synced = true;
+			await attachment.saveTx();
+			var annotation = await createAnnotation('highlight', attachment);
+			annotation.createdByUserID = createdByUserID;
+			await annotation.saveTx({
+				skipEditCheck: true
+			});
+			var responseJSON = annotation.toResponseJSON();
+			responseJSON.version = 10;
+			responseJSON.data.version = 10;
+			var newComment = 'new comment';
+			responseJSON.data.annotationComment = newComment;
+			
+			let response = {
+				successful: {
+					"0": responseJSON
+				},
+				unchanged: {},
+				failed: {}
+			};
+			setResponse({
+				method: "POST",
+				url: `groups/${group.id}/items`,
+				status: 200,
+				headers: {
+					"Last-Modified-Version": 10
+				},
+				json: response
+			})
+			
+			var result = await engine._startUpload();
+			
+			assert.equal(result, engine.UPLOAD_RESULT_SUCCESS);
+			assert.equal(annotation.version, 10);
+			assert.equal(annotation.annotationComment, newComment);
+		});
+		
+		
 		it("should prompt to reset library on 403 write response and reset on accept", function* () {
 			var group = yield createGroup({
 				libraryVersion: 5