From 8992efc869e14014420ffaaf9c642fdcc47390bf 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 54ec57a440..1832e6583a 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 e0806a5e28..f1244f13de 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