From b54b7edf9b21c4806b7003335dd0204c1ac49025 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Sat, 19 Feb 2022 13:41:59 -0500 Subject: [PATCH] Don't set author name of current user when copying annotation from group Fixes #2358 --- chrome/content/zotero/xpcom/data/items.js | 8 +-- test/tests/collectionTreeTest.js | 51 ++++-------------- test/tests/itemsTest.js | 65 +++++++++++++++++++++++ 3 files changed, 79 insertions(+), 45 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index 332d0e1c98..c87c05c8f8 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/chrome/content/zotero/xpcom/data/items.js @@ -902,9 +902,11 @@ Zotero.Items = function() { } let newAnnotation = annotation.clone(toItem.libraryID); newAnnotation.parentItemID = toItem.id; - // If there's no explicit author and we're copying from a group, set the author - // to the creating user - if (!annotation.annotationAuthorName && fromGroup) { + // If there's no explicit author and we're copying an annotation created by another + // user from a group, set the author to the creating user + if (fromGroup + && !annotation.annotationAuthorName + && annotation.createdByUserID != Zotero.Users.getCurrentUserID()) { newAnnotation.annotationAuthorName = Zotero.Users.getName(annotation.createdByUserID); } diff --git a/test/tests/collectionTreeTest.js b/test/tests/collectionTreeTest.js index 0148b3a00d..686b2d3e98 100644 --- a/test/tests/collectionTreeTest.js +++ b/test/tests/collectionTreeTest.js @@ -815,6 +815,9 @@ describe("Zotero.CollectionTree", function() { }); it("should copy an item with a PDF attachment containing annotations to a group", async function () { + await Zotero.Users.setCurrentUserID(1); + await Zotero.Users.setName(1, 'Name'); + var group = await createGroup(); var item = await createDataObject('item', false, { skipSelect: true }); @@ -860,6 +863,10 @@ describe("Zotero.CollectionTree", function() { }); it("should copy a group item with a PDF attachment containing annotations to the personal library", async function () { + await Zotero.Users.setCurrentUserID(1); + await Zotero.Users.setName(1, 'Name 1'); + await Zotero.Users.setName(12345, 'Name 2'); + var group = await createGroup(); await cv.selectLibrary(group.libraryID); @@ -871,55 +878,15 @@ describe("Zotero.CollectionTree", function() { parentItemID: groupItem.id }); var annotation = await createAnnotation('highlight', attachment); - await Zotero.Users.setName(12345, 'Name'); - annotation.createdByUserID = 12345; - await annotation.saveTx({ - skipEditCheck: true - }); + await annotation.saveTx(); var ids = (await onDrop('item', 'L1', [groupItem.id])).ids; var newItem = Zotero.Items.get(ids[0]); - var newAttachment = Zotero.Items.get(newItem.getAttachments())[0]; - // Check annotation + var newAttachment = Zotero.Items.get(newItem.getAttachments())[0]; var annotations = newAttachment.getAnnotations(); assert.lengthOf(annotations, 1); - // Name transferred by Zotero.Items.copyChildItems() - assert.equal(annotations[0].annotationAuthorName, 'Name'); - - return group.eraseTx(); - }); - - it("should preserve an explicit annotation author name when copied from a group to a personal library", async function () { - var group = await createGroup(); - await cv.selectLibrary(group.libraryID); - - var groupItem = await createDataObject('item', { libraryID: group.libraryID }); - var file = getTestDataDirectory(); - file.append('test.pdf'); - var attachment = await Zotero.Attachments.importFromFile({ - file, - parentItemID: groupItem.id - }); - var annotation = await createAnnotation('highlight', attachment); - await Zotero.Users.setName(1, 'Name'); - annotation.createdByUserID = 1; - annotation.annotationAuthorName = 'Another Name'; - await annotation.saveTx({ - skipEditCheck: true - }); - - var ids = (await onDrop('item', 'L1', [groupItem.id])).ids; - var newItem = Zotero.Items.get(ids[0]); - - var newAttachment = Zotero.Items.get(newItem.getAttachments())[0]; - - // Check annotation - var annotations = newAttachment.getAnnotations(); - assert.lengthOf(annotations, 1); - // Name transferred by Zotero.Items.copyChildItems() - assert.equal(annotations[0].annotationAuthorName, 'Another Name'); return group.eraseTx(); }); diff --git a/test/tests/itemsTest.js b/test/tests/itemsTest.js index 01da27ca09..ac1eb21882 100644 --- a/test/tests/itemsTest.js +++ b/test/tests/itemsTest.js @@ -153,6 +153,71 @@ describe("Zotero.Items", function () { }); + describe("#copyChildItems()", function () { + var group; + + before(async function () { + group = await createGroup(); + }); + + after(async function () { + await group.eraseTx(); + }); + + it("should copy annotations from a group library to a personal library", async function () { + await Zotero.Users.setCurrentUserID(1); + await Zotero.Users.setName(1, 'Name 1'); + await Zotero.Users.setName(12345, 'Name 2'); + + var userLibraryID = Zotero.Libraries.userLibraryID; + + var item = await createDataObject('item', { libraryID: group.libraryID }); + var file = getTestDataDirectory(); + file.append('test.pdf'); + var attachment = await Zotero.Attachments.importFromFile({ + file, + parentItemID: item.id + }); + // Annotation by this user + var annotation1 = await createAnnotation('highlight', attachment); + await annotation1.saveTx(); + // Annotation by this user with createdByUserID set + var annotation2 = await createAnnotation('highlight', attachment); + annotation2.createdByUserID = 1; + await annotation2.saveTx({ + skipEditCheck: true + }); + // Annotation by another user + var annotation3 = await createAnnotation('highlight', attachment); + annotation3.createdByUserID = 12345; + await annotation3.saveTx({ + skipEditCheck: true + }); + + var newItem = item.clone(userLibraryID); + await newItem.saveTx(); + var newAttachment = attachment.clone(userLibraryID); + await newAttachment.saveTx(); + + await Zotero.DB.executeTransaction(async function () { + await Zotero.Items.copyChildItems(attachment, newAttachment); + }); + + // Check annotations + var annotations = newAttachment.getAnnotations(); + assert.lengthOf(annotations, 3); + var newAnnotation1 = annotations.find(o => o.annotationText == annotation1.annotationText); + var newAnnotation2 = annotations.find(o => o.annotationText == annotation2.annotationText); + var newAnnotation3 = annotations.find(o => o.annotationText == annotation3.annotationText); + // Current user's name shouldn't have been transferred + assert.isNull(newAnnotation1.annotationAuthorName); + assert.isNull(newAnnotation2.annotationAuthorName); + // Other user's should've been transferred + assert.equal(newAnnotation3.annotationAuthorName, 'Name 2'); + }); + }); + + describe("#merge()", function () { it("should merge two items", function* () { var item1 = yield createDataObject('item');