From c7ece79f79ef88e4c29434fedcfdcdbbeb9faa8e Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Wed, 9 Mar 2022 02:37:15 -0500
Subject: [PATCH] Fix missing creators not being removed in item.fromJSON()

Apparently there's been a bug for years where removing a creator
remotely hasn't caused it to be removed locally via sync...

https://forums.zotero.org/discussion/94910/desktop-app-not-correctly-syncing
---
 chrome/content/zotero/xpcom/data/item.js | 10 ++---
 test/content/support.js                  |  3 ++
 test/tests/itemTest.js                   | 32 +++++++++++++++
 test/tests/syncLocalTest.js              | 52 ++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 5 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 7e8d734907..9d8c8b0c63 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -1112,12 +1112,12 @@ Zotero.Item.prototype.setCreator = function (orderIndex, data, options = {}) {
  * @param {Object[]} data - An array of creator data in internal or API JSON format
  */
 Zotero.Item.prototype.setCreators = function (data, options = {}) {
-	// If empty array, clear all existing creators
-	if (!data.length) {
-		while (this.hasCreatorAt(0)) {
-			this.removeCreator(0);
+	// Clear existing creators beyond the number of provided ones
+	var numCreators = this.numCreators();
+	if (data.length < numCreators) {
+		while (this.hasCreatorAt(data.length)) {
+			this.removeCreator(data.length);
 		}
-		return;
 	}
 	
 	for (let i = 0; i < data.length; i++) {
diff --git a/test/content/support.js b/test/content/support.js
index edfdbfa789..af297eec10 100644
--- a/test/content/support.js
+++ b/test/content/support.js
@@ -453,6 +453,9 @@ function createUnsavedDataObject(objectType, params = {}) {
 		if (params.title !== undefined || params.setTitle) {
 			obj.setField('title', params.title !== undefined ? params.title : Zotero.Utilities.randomString());
 		}
+		if (params.creators !== undefined) {
+			obj.setCreators(params.creators);
+		}
 		if (params.collections !== undefined) {
 			obj.setCollections(params.collections);
 		}
diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js
index d0817b193e..0787383565 100644
--- a/test/tests/itemTest.js
+++ b/test/tests/itemTest.js
@@ -2267,6 +2267,38 @@ describe("Zotero.Item", function () {
 			assert.strictEqual(item.getField('accessDate'), '');
 		});
 		
+		it("should remove missing creators and change existing", function () {
+			var item = new Zotero.Item('book');
+			item.setCreators(
+				[
+					{
+						name: "A",
+						creatorType: "author"
+					},
+					{
+						name: "B",
+						creatorType: "author"
+					},
+					{
+						name: "C",
+						creatorType: "author"
+					}
+				]
+			);
+			var json = item.toJSON();
+			// Remove creators, which should cause them to be cleared in fromJSON()
+			var newCreators = [
+				{
+					name: "D",
+					creatorType: "author"
+				}
+			];
+			json.creators = newCreators;
+			
+			item.fromJSON(json);
+			assert.sameDeepMembers(item.getCreatorsJSON(), newCreators);
+		});
+		
 		it("should remove item from collection if 'collections' property not provided", function* () {
 			var collection = yield createDataObject('collection');
 			// Create standalone attachment in collection
diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js
index 137a29ef84..19b25da955 100644
--- a/test/tests/syncLocalTest.js
+++ b/test/tests/syncLocalTest.js
@@ -747,6 +747,58 @@ describe("Zotero.Sync.Data.Local", function() {
 			assert.propertyVal(cacheJSON.data, "name", changedName);
 		});
 		
+		it("should remove creators that were removed remotely and change existing", async function () {
+			var libraryID = Zotero.Libraries.userLibraryID;
+			
+			let item = await createDataObject(
+				'item',
+				{
+					version: 5,
+					creators: [
+						{
+							name: "A",
+							creatorType: "author"
+						},
+						{
+							name: "B",
+							creatorType: "author"
+						},
+						{
+							name: "C",
+							creatorType: "author"
+						}
+					]
+				}
+			);
+			let data = item.toJSON();
+			await Zotero.Sync.Data.Local.saveCacheObjects('item', libraryID, [data]);
+			
+			var newCreators = [
+				{
+					name: "D",
+					creatorType: "author"
+				}
+			];
+			
+			// Create remote version with removed creators
+			data.version = 10;
+			data.creators = newCreators;
+			let json = {
+				key: item.key,
+				version: 10,
+				data
+			};
+			await Zotero.Sync.Data.Local.processObjectsFromJSON(
+				'item', libraryID, [json], { stopOnError: true }
+			);
+			assert.equal(item.version, 10);
+			var creatorJSON = item.getCreatorsJSON();
+			assert.sameDeepMembers(creatorJSON, newCreators);
+			// Sync cache should match remote
+			var cacheJSON = await Zotero.Sync.Data.Local.getCacheObject('item', libraryID, data.key, data.version);
+			assert.sameDeepMembers(cacheJSON.data.creators, newCreators);
+		});
+		
 		it("should delete older versions in sync cache after processing", function* () {
 			var libraryID = Zotero.Libraries.userLibraryID;