From 7533775a357f1c90e7d3b9a73188c0254e6ce273 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Mon, 20 Nov 2023 15:25:46 +0000
Subject: [PATCH 01/25] merge simple attributes

---
 app/models/person.rb             |  2 ++
 lib/seek/merging/person_merge.rb | 35 ++++++++++++++++++++++++++++++++
 test/unit/person_test.rb         | 15 ++++++++++++++
 3 files changed, 52 insertions(+)
 create mode 100644 lib/seek/merging/person_merge.rb

diff --git a/app/models/person.rb b/app/models/person.rb
index dd1778a7a6..70648300b8 100644
--- a/app/models/person.rb
+++ b/app/models/person.rb
@@ -124,6 +124,8 @@ class Person < ApplicationRecord
   after_destroy :updated_contributed_items_contributor_after_destroy
   after_destroy :update_publication_authors_after_destroy
 
+  include Seek::Merging::PersonMerge
+
   # to make it look like a User
   def person
     self
diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
new file mode 100644
index 0000000000..f01dfd2730
--- /dev/null
+++ b/lib/seek/merging/person_merge.rb
@@ -0,0 +1,35 @@
+module Seek
+  module Merging
+    module PersonMerge
+      def merge(other_person)
+        merge_simple_attributes(other_person)
+      end
+
+      private
+
+      # This attributes are directly copied from other_person if they are empty in the person to which its merging.
+      # first_letter is also updated
+      def simple_attributes
+        %i[
+          first_name
+          last_name
+          email
+          phone
+          skype_name
+          web_page
+          description
+          avatar_id
+          orcid
+        ]
+      end
+
+      def merge_simple_attributes(other_person)
+        simple_attributes.each do |attribute|
+          send("#{attribute}=", other_person.send(attribute)) if send(attribute).nil?
+        end
+        update_first_letter
+      end
+
+    end
+  end
+end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index f456e4441f..4a35e99f2c 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -2,6 +2,7 @@
 
 class PersonTest < ActiveSupport::TestCase
   fixtures :users, :people, :roles
+  include Seek::Merging::PersonMerge
 
   def test_work_groups
     p = FactoryBot.create(:person_in_multiple_projects)
@@ -1407,4 +1408,18 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert_equal [project, project2, project3], person.projects.sort_by(&:id)
     assert_equal [project,project2], person.administered_projects.sort_by(&:id)
   end
+
+  test 'merge simple attributes' do
+    person_to_keep = FactoryBot.create(:min_person)
+    other_person = FactoryBot.create(:max_person)
+    person_to_keep.merge(other_person)
+    assert_equal 'Minimal', person_to_keep.last_name
+    assert_equal 'minimal_person@email.com', person_to_keep.email
+    attributes = simple_attributes - %i[last_name email]
+    attributes.each do |attribute|
+      assert_equal other_person.send(attribute), person_to_keep.send(attribute),
+                   "Should copy #{attribute} if empty in person to keep"
+    end
+  end
+
 end

From 92fc04abcd77427244bbaae34b3ce8a8fc9b6a71 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 21 Nov 2023 09:24:25 +0000
Subject: [PATCH 02/25] destroy other person after merge

---
 lib/seek/merging/person_merge.rb | 2 ++
 test/unit/person_test.rb         | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index f01dfd2730..38448bc8ac 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -3,6 +3,8 @@ module Merging
     module PersonMerge
       def merge(other_person)
         merge_simple_attributes(other_person)
+
+        other_person.destroy
       end
 
       private
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 4a35e99f2c..150e07df25 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1422,4 +1422,13 @@ def test_updated_not_changed_when_adding_notifiee_info
     end
   end
 
+  test 'other person is destroyed after merge' do
+    person_to_keep = FactoryBot.create(:min_person)
+    other_person = FactoryBot.create(:max_person)
+    assert_difference('Person.count', -1) do
+      disable_authorization_checks { person_to_keep.merge(other_person) }
+    end
+    assert_nil Person.find_by_id(other_person.id)
+  end
+
 end

From beb336d36a73a1e66609f635a5d36d71d4196a58 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 21 Nov 2023 09:54:10 +0000
Subject: [PATCH 03/25] merge group_memberships (work_groups and projects)

---
 lib/seek/merging/person_merge.rb | 20 ++++++++++++++++++++
 test/unit/person_test.rb         | 12 ++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 38448bc8ac..38a67d4d5a 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -4,6 +4,8 @@ module PersonMerge
       def merge(other_person)
         merge_simple_attributes(other_person)
 
+        # Merging group_memberships deals with work_groups and projects
+        merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
         other_person.destroy
       end
 
@@ -32,6 +34,24 @@ def merge_simple_attributes(other_person)
         update_first_letter
       end
 
+      def merge_associations(current_associations, other_associations, check_existing)
+        other_associations.each do |other_association|
+          existing_association = nil
+          if check_existing
+            existing_association = current_associations.find do |assoc|
+              # Check if association already exists
+              assoc.send(check_existing) == other_association.send(check_existing)
+            end
+          end
+
+          next if existing_association
+
+          duplicated_association = other_association.dup
+          duplicated_association.person_id = id
+          current_associations << duplicated_association
+        end
+      end
+
     end
   end
 end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 150e07df25..1e30dcbd83 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1422,6 +1422,18 @@ def test_updated_not_changed_when_adding_notifiee_info
     end
   end
 
+  test 'merge group_memberships without duplication' do
+    person_to_keep = FactoryBot.create(:person_in_multiple_projects)
+    orig_wg_ids = person_to_keep.work_group_ids
+    orig_proj_ids = person_to_keep.project_ids
+    other_person = FactoryBot.create(:max_person)
+    other_person.work_groups << person_to_keep.work_groups[0]
+    person_to_keep.merge(other_person)
+    person_to_keep.reload
+    assert_equal (orig_wg_ids + other_person.work_group_ids).compact.uniq.sort, person_to_keep.work_group_ids.sort
+    assert_equal (orig_proj_ids + other_person.project_ids).compact.uniq.sort, person_to_keep.project_ids.sort
+  end
+
   test 'other person is destroyed after merge' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)

From 8a5f37ce8072476566d23d8a0094e1e48129768e Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 21 Nov 2023 19:48:48 +0000
Subject: [PATCH 04/25] Guard 'destroy' and back-up other person's details in
 tests before deletion

---
 lib/seek/merging/person_merge.rb |  5 ++++-
 test/unit/person_test.rb         | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 38a67d4d5a..2be5c5bd16 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -6,7 +6,10 @@ def merge(other_person)
 
         # Merging group_memberships deals with work_groups and projects
         merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
-        other_person.destroy
+        Person.transaction do
+          save!
+          other_person.destroy
+        end
       end
 
       private
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 1e30dcbd83..c260d48782 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1412,12 +1412,19 @@ def test_updated_not_changed_when_adding_notifiee_info
   test 'merge simple attributes' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)
-    person_to_keep.merge(other_person)
+    other_person_attributes = {}
+    simple_attributes.each do |attribute|
+      other_person_attributes[attribute] = other_person.send(attribute)
+    end
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
     assert_equal 'Minimal', person_to_keep.last_name
     assert_equal 'minimal_person@email.com', person_to_keep.email
     attributes = simple_attributes - %i[last_name email]
     attributes.each do |attribute|
-      assert_equal other_person.send(attribute), person_to_keep.send(attribute),
+      assert_equal other_person_attributes[attribute], person_to_keep.send(attribute),
                    "Should copy #{attribute} if empty in person to keep"
     end
   end
@@ -1428,10 +1435,14 @@ def test_updated_not_changed_when_adding_notifiee_info
     orig_proj_ids = person_to_keep.project_ids
     other_person = FactoryBot.create(:max_person)
     other_person.work_groups << person_to_keep.work_groups[0]
-    person_to_keep.merge(other_person)
+    other_wg_ids = other_person.work_group_ids
+    other_proj_ids = other_person.project_ids
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
-    assert_equal (orig_wg_ids + other_person.work_group_ids).compact.uniq.sort, person_to_keep.work_group_ids.sort
-    assert_equal (orig_proj_ids + other_person.project_ids).compact.uniq.sort, person_to_keep.project_ids.sort
+
+    assert_equal (orig_wg_ids + other_wg_ids).compact.uniq.sort, person_to_keep.work_group_ids.sort
+    assert_equal (orig_proj_ids + other_proj_ids).compact.uniq.sort, person_to_keep.project_ids.sort
   end
 
   test 'other person is destroyed after merge' do

From c58e87b491960ece8f4c3dfdde9fd2e77a65b86d Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 21 Nov 2023 19:51:38 +0000
Subject: [PATCH 05/25] merge annotations

---
 lib/seek/merging/person_merge.rb | 14 ++++++++++++++
 test/unit/person_test.rb         | 26 ++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 2be5c5bd16..b8735b0bfb 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -3,6 +3,7 @@ module Merging
     module PersonMerge
       def merge(other_person)
         merge_simple_attributes(other_person)
+        merge_annotations(other_person)
 
         # Merging group_memberships deals with work_groups and projects
         merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
@@ -37,6 +38,19 @@ def merge_simple_attributes(other_person)
         update_first_letter
       end
 
+      def annotation_types
+        %w[
+          expertise
+          tools
+        ]
+      end
+
+      def merge_annotations(other_person)
+        annotation_types.each do |annotation_type|
+          add_annotations(send(annotation_type)+other_person.send(annotation_type), annotation_type.singularize, self)
+        end
+      end
+
       def merge_associations(current_associations, other_associations, check_existing)
         other_associations.each do |other_association|
           existing_association = nil
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index c260d48782..f1cae95d68 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1429,6 +1429,32 @@ def test_updated_not_changed_when_adding_notifiee_info
     end
   end
 
+  test 'merge annotations' do
+    person_to_keep = FactoryBot.create(:min_person)
+    person_to_keep.annotate_with(['golf','merging'], 'expertise', person_to_keep)
+    person_to_keep.annotate_with(['merger'], 'tool', person_to_keep)
+    person_to_keep.save!
+    person_to_keep.reload
+    orig_annotations = {}
+    annotation_types.each do |annotation_type|
+      orig_annotations[annotation_type] = person_to_keep.send(annotation_type)
+    end
+    other_person = FactoryBot.create(:max_person)
+    other_annotations = {}
+    annotation_types.each do |annotation_type|
+      other_annotations[annotation_type] = other_person.send(annotation_type)
+    end
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    annotation_types.each do |annotation_type|
+      assert_equal (orig_annotations[annotation_type]+other_annotations[annotation_type]).compact.uniq.sort,
+                   person_to_keep.send(annotation_type).sort,
+                   "Should copy #{annotation_type} if not present in person to keep"
+    end
+  end
+
   test 'merge group_memberships without duplication' do
     person_to_keep = FactoryBot.create(:person_in_multiple_projects)
     orig_wg_ids = person_to_keep.work_group_ids

From 7a90b11e5610582fbbb719807854adcb0dc93f15 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Thu, 23 Nov 2023 12:54:12 +0000
Subject: [PATCH 06/25] Programmes and institutions also covered by
 group_memberships

---
 lib/seek/merging/person_merge.rb | 2 +-
 test/unit/person_test.rb         | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index b8735b0bfb..43c12f14ea 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -5,7 +5,7 @@ def merge(other_person)
         merge_simple_attributes(other_person)
         merge_annotations(other_person)
 
-        # Merging group_memberships deals with work_groups and projects
+        # Merging group_memberships deals with work_groups, programmes, institutions and projects
         merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
         Person.transaction do
           save!
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index f1cae95d68..92bd39af73 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1457,17 +1457,26 @@ def test_updated_not_changed_when_adding_notifiee_info
 
   test 'merge group_memberships without duplication' do
     person_to_keep = FactoryBot.create(:person_in_multiple_projects)
+    FactoryBot.create(:programme, project_ids: [person_to_keep.project_ids.last])
     orig_wg_ids = person_to_keep.work_group_ids
+    orig_programme_ids = person_to_keep.programme_ids
+    orig_inst_ids = person_to_keep.institution_ids
     orig_proj_ids = person_to_keep.project_ids
+
     other_person = FactoryBot.create(:max_person)
+    FactoryBot.create(:programme, project_ids: [other_person.project_ids.last])
     other_person.work_groups << person_to_keep.work_groups[0]
     other_wg_ids = other_person.work_group_ids
+    other_programme_ids = other_person.programme_ids
+    other_inst_ids = other_person.institution_ids
     other_proj_ids = other_person.project_ids
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
 
     assert_equal (orig_wg_ids + other_wg_ids).compact.uniq.sort, person_to_keep.work_group_ids.sort
+    assert_equal (orig_programme_ids + other_programme_ids).compact.uniq.sort, person_to_keep.programme_ids.sort
+    assert_equal (orig_inst_ids + other_inst_ids).compact.uniq.sort, person_to_keep.institution_ids.sort
     assert_equal (orig_proj_ids + other_proj_ids).compact.uniq.sort, person_to_keep.project_ids.sort
   end
 

From 525d8f1308e075fb7e9db808d866e3aaf2abec2a Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Thu, 23 Nov 2023 13:58:06 +0000
Subject: [PATCH 07/25] check not merging empty arrays

---
 test/unit/person_test.rb | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 92bd39af73..966d69a383 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1458,26 +1458,27 @@ def test_updated_not_changed_when_adding_notifiee_info
   test 'merge group_memberships without duplication' do
     person_to_keep = FactoryBot.create(:person_in_multiple_projects)
     FactoryBot.create(:programme, project_ids: [person_to_keep.project_ids.last])
-    orig_wg_ids = person_to_keep.work_group_ids
-    orig_programme_ids = person_to_keep.programme_ids
-    orig_inst_ids = person_to_keep.institution_ids
-    orig_proj_ids = person_to_keep.project_ids
-
     other_person = FactoryBot.create(:max_person)
     FactoryBot.create(:programme, project_ids: [other_person.project_ids.last])
     other_person.work_groups << person_to_keep.work_groups[0]
-    other_wg_ids = other_person.work_group_ids
-    other_programme_ids = other_person.programme_ids
-    other_inst_ids = other_person.institution_ids
-    other_proj_ids = other_person.project_ids
+
+    ids = [:work_group_ids, :programme_ids, :institution_ids, :project_ids]
+    expected = {}
+    ids.each do |var|
+      orig_ids = person_to_keep.send(var)
+      other_ids = other_person.send(var)
+      refute_empty orig_ids
+      refute_empty other_ids
+      refute_equal orig_ids, other_ids
+      expected[var] = (orig_ids + other_ids).compact.uniq.sort
+    end
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
 
-    assert_equal (orig_wg_ids + other_wg_ids).compact.uniq.sort, person_to_keep.work_group_ids.sort
-    assert_equal (orig_programme_ids + other_programme_ids).compact.uniq.sort, person_to_keep.programme_ids.sort
-    assert_equal (orig_inst_ids + other_inst_ids).compact.uniq.sort, person_to_keep.institution_ids.sort
-    assert_equal (orig_proj_ids + other_proj_ids).compact.uniq.sort, person_to_keep.project_ids.sort
+    ids.each do |var|
+      assert_equal expected[var], person_to_keep.send(var).sort
+    end
   end
 
   test 'other person is destroyed after merge' do

From c43cdaeb96b4feccfcdb4cc3a0879cd445f87e5b Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Fri, 24 Nov 2023 09:49:36 +0000
Subject: [PATCH 08/25] merge subscriptions

---
 lib/seek/merging/person_merge.rb |  1 +
 test/unit/person_test.rb         | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 43c12f14ea..13ebc874a3 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -7,6 +7,7 @@ def merge(other_person)
 
         # Merging group_memberships deals with work_groups, programmes, institutions and projects
         merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
+        merge_associations(subscriptions, other_person.subscriptions, 'subscribable_id')
         Person.transaction do
           save!
           other_person.destroy
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 966d69a383..5e1034f43a 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1481,6 +1481,28 @@ def test_updated_not_changed_when_adding_notifiee_info
     end
   end
 
+  test 'merge subscriptions without duplication' do
+    sop_keep = FactoryBot.create :sop
+    sop_both = FactoryBot.create :sop
+    sop_other = FactoryBot.create :sop
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:max_person)
+    FactoryBot.create :subscription, person: person_to_keep, subscribable: sop_keep
+    FactoryBot.create :subscription, person: person_to_keep, subscribable: sop_both
+    FactoryBot.create :subscription, person: other_person, subscribable: sop_both
+    FactoryBot.create :subscription, person: other_person, subscribable: sop_other
+    orig_subs = person_to_keep.subscriptions.pluck(:subscribable_id)
+    other_subs = other_person.subscriptions.pluck(:subscribable_id)
+    assert orig_subs.include?(sop_keep.id)
+    assert orig_subs.include?(sop_both.id) && other_subs.include?(sop_both.id)
+    assert other_subs.include?(sop_other.id)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    assert_equal (orig_subs + other_subs).compact.uniq.sort, person_to_keep.subscriptions.pluck(:subscribable_id).sort
+  end
+
   test 'other person is destroyed after merge' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)

From a34e8c0a89e747e2b569399bbe1fbd463eb610c6 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Fri, 24 Nov 2023 12:40:32 +0000
Subject: [PATCH 09/25] Merge resources (contributor and creator)

---
 lib/seek/merging/person_merge.rb | 14 ++++++++++++
 test/unit/person_test.rb         | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 13ebc874a3..ff9197d051 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -8,6 +8,7 @@ def merge(other_person)
         # Merging group_memberships deals with work_groups, programmes, institutions and projects
         merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
         merge_associations(subscriptions, other_person.subscriptions, 'subscribable_id')
+        merge_resources(other_person)
         Person.transaction do
           save!
           other_person.destroy
@@ -70,6 +71,19 @@ def merge_associations(current_associations, other_associations, check_existing)
         end
       end
 
+      def merge_resources(other_person)
+        # Contributed
+        Person::RELATED_RESOURCE_TYPES.each do |resource_type|
+          resource_type.constantize.where(contributor_id: other_person.id).update_all(contributor_id: id)
+        end
+        # Created
+        duplicated = other_person.created_items.pluck(:id) & created_items.pluck(:id)
+        AssetsCreator.where(creator_id: other_person.id, asset_id: duplicated).destroy_all
+        AssetsCreator.where(creator_id: other_person.id).update_all(creator_id: id)
+        # Reload to prevent destruction of unlinked resources
+        other_person.reload
+      end
+
     end
   end
 end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 5e1034f43a..1f011b6ed3 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1503,6 +1503,44 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert_equal (orig_subs + other_subs).compact.uniq.sort, person_to_keep.subscriptions.pluck(:subscribable_id).sort
   end
 
+  test 'merge resources without duplication' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+    s1 = FactoryBot.create(:sop, contributor: person_to_keep)
+    s2 = FactoryBot.create(:sop, contributor: person_to_keep, creator_ids: [person_to_keep.id, other_person.id])
+    s3 = FactoryBot.create(:sop, contributor: other_person)
+    s4 = FactoryBot.create(:sop, contributor: other_person, creator_ids: [person_to_keep.id, other_person.id])
+    s5 = FactoryBot.create(:sop, contributor: FactoryBot.create(:person), creator_ids: [other_person.id])
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    assert_equal [s1, s2, s3, s4].sort, person_to_keep.contributed_sops.sort
+    assert_equal [s2, s4, s5].sort, person_to_keep.created_sops.sort
+  end
+
+  test 'merge all types of resources' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+
+    types_without_creators = ["Event", "Strain"]
+    Person::RELATED_RESOURCE_TYPES.each do |resource_type|
+      sym = resource_type == "SampleType" ? :simple_sample_type : resource_type.underscore.to_sym
+      FactoryBot.create(sym, contributor: other_person)
+      unless types_without_creators.include?(resource_type)
+        FactoryBot.create(sym, contributor: FactoryBot.create(:person), creator_ids: [other_person.id])
+      end
+    end
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    resource_types = Person::RELATED_RESOURCE_TYPES.length
+    added_isa_structure = 3 # An additional inv is created for the study, and inv and study are created for the assay
+    assert_equal resource_types + added_isa_structure, person_to_keep.contributed_items.length
+    assert_equal resource_types - types_without_creators.length, person_to_keep.created_items.length
+  end
+
   test 'other person is destroyed after merge' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)

From 1daacd7d75467673e43be59df82502828e7807c9 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Fri, 24 Nov 2023 17:30:49 +0000
Subject: [PATCH 10/25] Merge permissions

---
 lib/seek/merging/person_merge.rb |  9 ++++++++
 test/unit/person_test.rb         | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index ff9197d051..f6e09993dd 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -9,6 +9,7 @@ def merge(other_person)
         merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
         merge_associations(subscriptions, other_person.subscriptions, 'subscribable_id')
         merge_resources(other_person)
+        merge_permissions(other_person)
         Person.transaction do
           save!
           other_person.destroy
@@ -84,6 +85,14 @@ def merge_resources(other_person)
         other_person.reload
       end
 
+      def merge_permissions(other_person)
+        permissions_other = Permission.where(contributor_type: "Person", contributor_id: other_person.id)
+        permissions_slef = Permission.where(contributor_type: "Person", contributor_id: id)
+        duplicated = permissions_other.pluck(:policy_id) & permissions_slef.pluck(:policy_id)
+        permissions_other.where(policy_id: duplicated).destroy_all
+        permissions_other.update_all(contributor_id: id)
+      end
+
     end
   end
 end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 1f011b6ed3..45a17b3eec 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1541,6 +1541,44 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert_equal resource_types - types_without_creators.length, person_to_keep.created_items.length
   end
 
+  test 'merge permissions without duplication' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+
+    sop_keep = FactoryBot.create(:sop)
+    sop_keep.policy.permissions.build(contributor: person_to_keep, access_type: Policy::MANAGING)
+    sop_keep.policy.save
+    sop_both = FactoryBot.create :sop
+    sop_both.policy.permissions.build(contributor: person_to_keep, access_type: Policy::MANAGING)
+    sop_both.policy.permissions.build(contributor: other_person, access_type: Policy::MANAGING)
+    sop_both.policy.save
+    sop_other = FactoryBot.create :sop
+    sop_other.policy.permissions.build(contributor: other_person, access_type: Policy::MANAGING)
+    sop_other.policy.save
+
+    assert sop_keep.can_manage?(person_to_keep)
+    assert sop_both.can_manage?(person_to_keep)
+    refute sop_other.can_manage?(person_to_keep)
+    refute sop_keep.can_manage?(other_person)
+    assert sop_both.can_manage?(other_person)
+    assert sop_other.can_manage?(other_person)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+    sop_keep.reload
+    sop_both.reload
+    sop_other.reload
+    sop_keep.permission_for = nil
+    sop_both.permission_for = nil
+    sop_other.permission_for = nil
+
+    assert sop_keep.can_manage?(person_to_keep)
+    assert sop_both.can_manage?(person_to_keep)
+    assert sop_other.can_manage?(person_to_keep)
+    assert 1, sop_both.policy.permissions.length
+    assert 3, Permission.where(contributor_type: "Person",contributor_id: person_to_keep.id).length
+  end
+
   test 'other person is destroyed after merge' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)

From f0b8dbf0aaf1e3b9c3e1c583daea114d9dad3469 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Mon, 27 Nov 2023 17:22:23 +0000
Subject: [PATCH 11/25] Merge roles

---
 lib/seek/merging/person_merge.rb | 15 +++++++++++
 test/unit/person_test.rb         | 43 ++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index f6e09993dd..fcdb8aead8 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -10,6 +10,7 @@ def merge(other_person)
         merge_associations(subscriptions, other_person.subscriptions, 'subscribable_id')
         merge_resources(other_person)
         merge_permissions(other_person)
+        merge_roles(other_person)
         Person.transaction do
           save!
           other_person.destroy
@@ -93,6 +94,20 @@ def merge_permissions(other_person)
         permissions_other.update_all(contributor_id: id)
       end
 
+      def merge_roles(other_person)
+        other_roles = other_person.roles.pluck('scope_type', 'scope_id', 'role_type_id')
+        self_roles = roles.pluck('scope_type', 'scope_id', 'role_type_id')
+        duplicated = other_roles & self_roles
+        other_person.roles.where({
+                                   scope_type: duplicated.map { |triple| triple[0] },
+                                   scope_id: duplicated.map { |triple| triple[1] },
+                                   role_type_id: duplicated.map { |triple| triple[2] }
+                                 }).destroy_all
+        other_person.roles.update_all(person_id: id)
+        # Reload to prevent destruction of unlinked roles
+        other_person.reload
+      end
+
     end
   end
 end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 45a17b3eec..1728acd651 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1579,6 +1579,49 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert 3, Permission.where(contributor_type: "Person",contributor_id: person_to_keep.id).length
   end
 
+  test 'merge roles without duplication' do
+    person_to_keep = FactoryBot.create(:person_in_multiple_projects)
+    other_person = FactoryBot.create(:asset_gatekeeper)
+    other_person.work_groups << person_to_keep.work_groups.last
+
+    keep_proj = person_to_keep.projects.first
+    shared_proj = person_to_keep.projects.last
+    other_proj = other_person.projects.last
+
+    refute keep_proj.has_member?(other_person)
+    assert shared_proj.has_member?(person_to_keep)
+    assert shared_proj.has_member?(other_person)
+    refute other_proj.has_member?(person_to_keep)
+
+    person_to_keep.is_asset_housekeeper = true, keep_proj
+    person_to_keep.is_pal = true, shared_proj
+    other_person.is_pal = true, shared_proj
+    other_person.is_project_administrator = true, shared_proj
+    disable_authorization_checks do
+      person_to_keep.save!
+      other_person.save!
+    end
+    person_to_keep.reload
+    other_person.reload
+
+    assert person_to_keep.is_asset_housekeeper?(keep_proj)
+    assert person_to_keep.is_pal?(shared_proj)
+    refute person_to_keep.is_project_administrator?(shared_proj)
+    refute person_to_keep.is_asset_gatekeeper?(other_proj)
+    assert other_person.is_pal?(shared_proj)
+    assert other_person.is_project_administrator?(shared_proj)
+    assert other_person.is_asset_gatekeeper?(other_proj)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    assert person_to_keep.is_asset_housekeeper?(keep_proj)
+    assert person_to_keep.is_pal?(shared_proj)
+    assert person_to_keep.is_project_administrator?(shared_proj)
+    assert person_to_keep.is_asset_gatekeeper?(other_proj)
+    assert_equal 4, person_to_keep.roles.length
+  end
+
   test 'other person is destroyed after merge' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)

From 480209109dbf92f4ddfe3f4e7153863bfd4d615f Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 28 Nov 2023 14:21:59 +0000
Subject: [PATCH 12/25] Refactor merge_associations

---
 lib/seek/merging/person_merge.rb | 57 +++++++++-----------------------
 1 file changed, 15 insertions(+), 42 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index fcdb8aead8..90a3b1acc4 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -4,15 +4,16 @@ module PersonMerge
       def merge(other_person)
         merge_simple_attributes(other_person)
         merge_annotations(other_person)
-
+        # Roles have to be merged before group_memberships
+        merge_associations(other_person, 'roles', %i[scope_type scope_id role_type_id], { person_id: id })
         # Merging group_memberships deals with work_groups, programmes, institutions and projects
-        merge_associations(group_memberships, other_person.group_memberships, 'work_group_id')
-        merge_associations(subscriptions, other_person.subscriptions, 'subscribable_id')
+        merge_associations(other_person, 'group_memberships', [:work_group_id], { person_id: id })
+        merge_associations(other_person, 'subscriptions', [:subscribable_id], { person_id: id })
+        merge_associations(other_person, 'dependent_permissions', [:policy_id], { contributor_id: id })
         merge_resources(other_person)
-        merge_permissions(other_person)
-        merge_roles(other_person)
         Person.transaction do
           save!
+          other_person.reload # To prevent destruction of unlinked roles
           other_person.destroy
         end
       end
@@ -55,24 +56,6 @@ def merge_annotations(other_person)
         end
       end
 
-      def merge_associations(current_associations, other_associations, check_existing)
-        other_associations.each do |other_association|
-          existing_association = nil
-          if check_existing
-            existing_association = current_associations.find do |assoc|
-              # Check if association already exists
-              assoc.send(check_existing) == other_association.send(check_existing)
-            end
-          end
-
-          next if existing_association
-
-          duplicated_association = other_association.dup
-          duplicated_association.person_id = id
-          current_associations << duplicated_association
-        end
-      end
-
       def merge_resources(other_person)
         # Contributed
         Person::RELATED_RESOURCE_TYPES.each do |resource_type|
@@ -86,26 +69,16 @@ def merge_resources(other_person)
         other_person.reload
       end
 
-      def merge_permissions(other_person)
-        permissions_other = Permission.where(contributor_type: "Person", contributor_id: other_person.id)
-        permissions_slef = Permission.where(contributor_type: "Person", contributor_id: id)
-        duplicated = permissions_other.pluck(:policy_id) & permissions_slef.pluck(:policy_id)
-        permissions_other.where(policy_id: duplicated).destroy_all
-        permissions_other.update_all(contributor_id: id)
-      end
+      def merge_associations(other_person, assoc, duplicates_match, update_hash)
+        other_items = other_person.send(assoc).pluck(*duplicates_match)
+        self_items = send(assoc).pluck(*duplicates_match)
+        duplicated = other_items & self_items
+        duplicated = duplicated.map { |item| [item] } if duplicates_match.length == 1
 
-      def merge_roles(other_person)
-        other_roles = other_person.roles.pluck('scope_type', 'scope_id', 'role_type_id')
-        self_roles = roles.pluck('scope_type', 'scope_id', 'role_type_id')
-        duplicated = other_roles & self_roles
-        other_person.roles.where({
-                                   scope_type: duplicated.map { |triple| triple[0] },
-                                   scope_id: duplicated.map { |triple| triple[1] },
-                                   role_type_id: duplicated.map { |triple| triple[2] }
-                                 }).destroy_all
-        other_person.roles.update_all(person_id: id)
-        # Reload to prevent destruction of unlinked roles
-        other_person.reload
+        duplicated_hash = Hash[duplicates_match.zip(duplicated.transpose)]
+        other_person.send(assoc).where(duplicated_hash).destroy_all
+
+        other_person.send(assoc).update_all(update_hash)
       end
 
     end

From 5e1f0c9b1cbdc0f15b117b978b122f4903084c14 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 28 Nov 2023 16:08:44 +0000
Subject: [PATCH 13/25] Merge user

---
 lib/seek/merging/person_merge.rb | 26 ++++++++++
 test/factories/oauth.rb          |  7 +++
 test/unit/person_test.rb         | 81 ++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 90a3b1acc4..6b15885bba 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -11,6 +11,7 @@ def merge(other_person)
         merge_associations(other_person, 'subscriptions', [:subscribable_id], { person_id: id })
         merge_associations(other_person, 'dependent_permissions', [:policy_id], { contributor_id: id })
         merge_resources(other_person)
+        merge_user(other_person)
         Person.transaction do
           save!
           other_person.reload # To prevent destruction of unlinked roles
@@ -81,6 +82,31 @@ def merge_associations(other_person, assoc, duplicates_match, update_hash)
         other_person.send(assoc).update_all(update_hash)
       end
 
+      def merge_user(other_person)
+        return unless user
+
+        merge_user_associations(other_person, 'identities',
+                                %i[provider uid], { user_id: user.id })
+        merge_user_associations(other_person, 'oauth_applications',
+                                %i[redirect_uri scopes], { owner_id: user.id })
+        merge_user_associations(other_person, 'access_tokens',
+                                %i[application_id scopes], { resource_owner_id: user.id })
+        merge_user_associations(other_person, 'access_grants',
+                                %i[application_id redirect_uri scopes], { resource_owner_id: user.id })
+      end
+
+      def merge_user_associations(other_person, assoc, duplicates_match, update_hash)
+        other_items = other_person.user.send(assoc).pluck(*duplicates_match)
+        self_items = user.send(assoc).pluck(*duplicates_match)
+        duplicated = other_items & self_items
+        duplicated = duplicated.map { |item| [item] } if duplicates_match.length == 1
+
+        duplicated_hash = Hash[duplicates_match.zip(duplicated.transpose)]
+        other_person.user.send(assoc).where(duplicated_hash).destroy_all
+
+        other_person.user.send(assoc).update_all(update_hash)
+      end
+
     end
   end
 end
diff --git a/test/factories/oauth.rb b/test/factories/oauth.rb
index 0ad98f7120..64b5079377 100644
--- a/test/factories/oauth.rb
+++ b/test/factories/oauth.rb
@@ -11,4 +11,11 @@
     expires_in { 3600 }
     scopes { 'read' }
   end
+
+  factory(:oauth_access_grant, class: Doorkeeper::AccessGrant) do
+    association :application, factory: :oauth_application
+    sequence(:redirect_uri) { |n| "https://localhost:3000/grant_#{n}" }
+    expires_in { 3600 }
+    scopes { 'read' }
+  end
 end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 1728acd651..5ea0199d5f 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1622,6 +1622,87 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert_equal 4, person_to_keep.roles.length
   end
 
+  test 'merge user identities without duplication' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+
+    FactoryBot.create(:identity, uid: "ldap-keep", user_id: person_to_keep.user.id)
+    FactoryBot.create(:identity, uid: "ldap-shared", user_id: person_to_keep.user.id)
+    FactoryBot.create(:identity, uid: "ldap-shared", user_id: other_person.user.id)
+    FactoryBot.create(:identity, uid: "ldap-other", user_id: other_person.user.id)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    expected = %w[keep shared other]
+    assert_equal (expected.map { |exp| "ldap-#{exp}" }).sort, person_to_keep.user.identities.pluck(:uid).sort
+  end
+
+  test 'merge user oauth_applications without duplication' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+
+    oauth_url = 'https://localhost:3000/oauth'
+    FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_keep", owner: person_to_keep.user)
+    FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_shared", owner: person_to_keep.user)
+    FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_shared", owner: other_person.user)
+    FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_other", owner: other_person.user)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    expected = %w[keep shared other]
+    assert_equal (expected.map { |exp| "#{oauth_url}_#{exp}" }).sort,
+                 person_to_keep.user.oauth_applications.pluck(:redirect_uri).sort
+  end
+
+  test 'merge user access_tokens without duplication' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+
+    # OAuth Applications
+    oauth_url = 'https://localhost:3000/oauth'
+    ap_k = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_keep", owner: person_to_keep.user)
+    ap_s = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_shared", owner: person_to_keep.user)
+    ap_o = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_other", owner: other_person.user)
+    # Access tokens
+    FactoryBot.create(:oauth_access_token, application: ap_k, resource_owner_id: person_to_keep.user.id)
+    FactoryBot.create(:oauth_access_token, application: ap_s, resource_owner_id: person_to_keep.user.id)
+    FactoryBot.create(:oauth_access_token, application: ap_s, resource_owner_id: other_person.user.id)
+    FactoryBot.create(:oauth_access_token, application: ap_s, resource_owner_id: other_person.user.id, scopes: 'write')
+    FactoryBot.create(:oauth_access_token, application: ap_o, resource_owner_id: other_person.user.id)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    assert_equal [[ap_k.id, 'read'], [ap_s.id, 'read'], [ap_s.id, 'write'], [ap_o.id, 'read']].sort,
+                 person_to_keep.user.access_tokens.pluck(:application_id, :scopes).sort
+  end
+
+  test 'merge user access_grants without duplication' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:person)
+
+    # OAuth Applications
+    oauth_url = 'https://localhost:3000/oauth'
+    ap_k = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_keep", owner: person_to_keep.user)
+    ap_s = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_shared", owner: person_to_keep.user)
+    ap_o = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_other", owner: other_person.user)
+    # Access grants
+    grant_url = 'https://localhost:3000/grant'
+    FactoryBot.create(:oauth_access_grant, application: ap_k, redirect_uri: "#{grant_url}_keep", resource_owner_id: person_to_keep.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared", resource_owner_id: person_to_keep.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared", resource_owner_id: other_person.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared", resource_owner_id: other_person.user.id, scopes: 'write')
+    FactoryBot.create(:oauth_access_grant, application: ap_o, redirect_uri: "#{grant_url}_other", resource_owner_id: other_person.user.id)
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    assert_equal [[ap_k.id, 'read'], [ap_s.id, 'read'], [ap_s.id, 'write'], [ap_o.id, 'read']].sort,
+                 person_to_keep.user.access_grants.pluck(:application_id, :scopes).sort
+  end
+
   test 'other person is destroyed after merge' do
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)

From 7a34d007fefc0ac9e4e64c16acd1dff98914cbc6 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 28 Nov 2023 16:39:11 +0000
Subject: [PATCH 14/25] Adds activity log

---
 lib/seek/merging/person_merge.rb |  2 ++
 test/unit/person_test.rb         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 6b15885bba..60ba4a027e 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -16,6 +16,8 @@ def merge(other_person)
           save!
           other_person.reload # To prevent destruction of unlinked roles
           other_person.destroy
+          ActivityLog.create!(action: 'MERGE-person',
+                              data: "Person with id #{other_person.id} was merged into person with id #{id}.")
         end
       end
 
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 5ea0199d5f..81a36a8ab0 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1712,4 +1712,18 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert_nil Person.find_by_id(other_person.id)
   end
 
+  test 'log added after successful merge' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:max_person)
+
+    assert_difference('ActivityLog.count') do
+      disable_authorization_checks { person_to_keep.merge(other_person) }
+    end
+
+    merge_log = ActivityLog.last
+    assert_equal 'MERGE-person', merge_log.action
+    assert_includes merge_log.data, "#{other_person.id} was merged into"
+    assert_includes merge_log.data, "#{person_to_keep.id}."
+  end
+
 end

From 9f7ce4c3478e8c46a7baebf2f30a8c1dfba079f2 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 28 Nov 2023 16:59:09 +0000
Subject: [PATCH 15/25] rubocop

---
 lib/seek/merging/person_merge.rb |  2 +-
 test/unit/person_test.rb         | 37 ++++++++++++++++++--------------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 60ba4a027e..ba64ea64a4 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -55,7 +55,7 @@ def annotation_types
 
       def merge_annotations(other_person)
         annotation_types.each do |annotation_type|
-          add_annotations(send(annotation_type)+other_person.send(annotation_type), annotation_type.singularize, self)
+          add_annotations(send(annotation_type) + other_person.send(annotation_type), annotation_type.singularize, self)
         end
       end
 
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 81a36a8ab0..4249d68609 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1431,8 +1431,8 @@ def test_updated_not_changed_when_adding_notifiee_info
 
   test 'merge annotations' do
     person_to_keep = FactoryBot.create(:min_person)
-    person_to_keep.annotate_with(['golf','merging'], 'expertise', person_to_keep)
-    person_to_keep.annotate_with(['merger'], 'tool', person_to_keep)
+    person_to_keep.annotate_with(%w[golf merging], 'expertise', person_to_keep)
+    person_to_keep.annotate_with(%w[merger], 'tool', person_to_keep)
     person_to_keep.save!
     person_to_keep.reload
     orig_annotations = {}
@@ -1449,7 +1449,7 @@ def test_updated_not_changed_when_adding_notifiee_info
     person_to_keep.reload
 
     annotation_types.each do |annotation_type|
-      assert_equal (orig_annotations[annotation_type]+other_annotations[annotation_type]).compact.uniq.sort,
+      assert_equal (orig_annotations[annotation_type] + other_annotations[annotation_type]).compact.uniq.sort,
                    person_to_keep.send(annotation_type).sort,
                    "Should copy #{annotation_type} if not present in person to keep"
     end
@@ -1462,7 +1462,7 @@ def test_updated_not_changed_when_adding_notifiee_info
     FactoryBot.create(:programme, project_ids: [other_person.project_ids.last])
     other_person.work_groups << person_to_keep.work_groups[0]
 
-    ids = [:work_group_ids, :programme_ids, :institution_ids, :project_ids]
+    ids = %i[work_group_ids programme_ids institution_ids project_ids]
     expected = {}
     ids.each do |var|
       orig_ids = person_to_keep.send(var)
@@ -1523,9 +1523,9 @@ def test_updated_not_changed_when_adding_notifiee_info
     person_to_keep = FactoryBot.create(:person)
     other_person = FactoryBot.create(:person)
 
-    types_without_creators = ["Event", "Strain"]
+    types_without_creators = %w[Event Strain]
     Person::RELATED_RESOURCE_TYPES.each do |resource_type|
-      sym = resource_type == "SampleType" ? :simple_sample_type : resource_type.underscore.to_sym
+      sym = resource_type == 'SampleType' ? :simple_sample_type : resource_type.underscore.to_sym
       FactoryBot.create(sym, contributor: other_person)
       unless types_without_creators.include?(resource_type)
         FactoryBot.create(sym, contributor: FactoryBot.create(:person), creator_ids: [other_person.id])
@@ -1576,7 +1576,7 @@ def test_updated_not_changed_when_adding_notifiee_info
     assert sop_both.can_manage?(person_to_keep)
     assert sop_other.can_manage?(person_to_keep)
     assert 1, sop_both.policy.permissions.length
-    assert 3, Permission.where(contributor_type: "Person",contributor_id: person_to_keep.id).length
+    assert 3, Permission.where(contributor_type: 'Person', contributor_id: person_to_keep.id).length
   end
 
   test 'merge roles without duplication' do
@@ -1626,10 +1626,10 @@ def test_updated_not_changed_when_adding_notifiee_info
     person_to_keep = FactoryBot.create(:person)
     other_person = FactoryBot.create(:person)
 
-    FactoryBot.create(:identity, uid: "ldap-keep", user_id: person_to_keep.user.id)
-    FactoryBot.create(:identity, uid: "ldap-shared", user_id: person_to_keep.user.id)
-    FactoryBot.create(:identity, uid: "ldap-shared", user_id: other_person.user.id)
-    FactoryBot.create(:identity, uid: "ldap-other", user_id: other_person.user.id)
+    FactoryBot.create(:identity, uid: 'ldap-keep', user_id: person_to_keep.user.id)
+    FactoryBot.create(:identity, uid: 'ldap-shared', user_id: person_to_keep.user.id)
+    FactoryBot.create(:identity, uid: 'ldap-shared', user_id: other_person.user.id)
+    FactoryBot.create(:identity, uid: 'ldap-other', user_id: other_person.user.id)
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
@@ -1690,11 +1690,16 @@ def test_updated_not_changed_when_adding_notifiee_info
     ap_o = FactoryBot.create(:oauth_application, redirect_uri: "#{oauth_url}_other", owner: other_person.user)
     # Access grants
     grant_url = 'https://localhost:3000/grant'
-    FactoryBot.create(:oauth_access_grant, application: ap_k, redirect_uri: "#{grant_url}_keep", resource_owner_id: person_to_keep.user.id)
-    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared", resource_owner_id: person_to_keep.user.id)
-    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared", resource_owner_id: other_person.user.id)
-    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared", resource_owner_id: other_person.user.id, scopes: 'write')
-    FactoryBot.create(:oauth_access_grant, application: ap_o, redirect_uri: "#{grant_url}_other", resource_owner_id: other_person.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_k, redirect_uri: "#{grant_url}_keep",
+                                           resource_owner_id: person_to_keep.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared",
+                                           resource_owner_id: person_to_keep.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared",
+                                           resource_owner_id: other_person.user.id)
+    FactoryBot.create(:oauth_access_grant, application: ap_s, redirect_uri: "#{grant_url}_shared",
+                                           resource_owner_id: other_person.user.id, scopes: 'write')
+    FactoryBot.create(:oauth_access_grant, application: ap_o, redirect_uri: "#{grant_url}_other",
+                                           resource_owner_id: other_person.user.id)
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload

From ec4c515698ac6b925db8b5dc1a205a6655f69169 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 28 Nov 2023 16:59:32 +0000
Subject: [PATCH 16/25] Person.transaction wraps all

---
 lib/seek/merging/person_merge.rb | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index ba64ea64a4..8cae279b05 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -2,17 +2,18 @@ module Seek
   module Merging
     module PersonMerge
       def merge(other_person)
-        merge_simple_attributes(other_person)
-        merge_annotations(other_person)
-        # Roles have to be merged before group_memberships
-        merge_associations(other_person, 'roles', %i[scope_type scope_id role_type_id], { person_id: id })
-        # Merging group_memberships deals with work_groups, programmes, institutions and projects
-        merge_associations(other_person, 'group_memberships', [:work_group_id], { person_id: id })
-        merge_associations(other_person, 'subscriptions', [:subscribable_id], { person_id: id })
-        merge_associations(other_person, 'dependent_permissions', [:policy_id], { contributor_id: id })
-        merge_resources(other_person)
-        merge_user(other_person)
         Person.transaction do
+          merge_simple_attributes(other_person)
+          merge_annotations(other_person)
+          # Roles have to be merged before group_memberships
+          merge_associations(other_person, 'roles', %i[scope_type scope_id role_type_id], { person_id: id })
+          # Merging group_memberships deals with work_groups, programmes, institutions and projects
+          merge_associations(other_person, 'group_memberships', [:work_group_id], { person_id: id })
+          merge_associations(other_person, 'subscriptions', [:subscribable_id], { person_id: id })
+          merge_associations(other_person, 'dependent_permissions', [:policy_id], { contributor_id: id })
+          merge_resources(other_person)
+          merge_user(other_person)
+
           save!
           other_person.reload # To prevent destruction of unlinked roles
           other_person.destroy

From 003c0ce1d5ab67163c6c25e9f2692ce03e048350 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Tue, 28 Nov 2023 17:09:08 +0000
Subject: [PATCH 17/25] Use send instead of constantize

---
 lib/seek/merging/person_merge.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 8cae279b05..6e13f6601c 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -63,14 +63,12 @@ def merge_annotations(other_person)
       def merge_resources(other_person)
         # Contributed
         Person::RELATED_RESOURCE_TYPES.each do |resource_type|
-          resource_type.constantize.where(contributor_id: other_person.id).update_all(contributor_id: id)
+          other_person.send("contributed_#{resource_type.underscore.pluralize}").update_all(contributor_id: id)
         end
         # Created
         duplicated = other_person.created_items.pluck(:id) & created_items.pluck(:id)
         AssetsCreator.where(creator_id: other_person.id, asset_id: duplicated).destroy_all
         AssetsCreator.where(creator_id: other_person.id).update_all(creator_id: id)
-        # Reload to prevent destruction of unlinked resources
-        other_person.reload
       end
 
       def merge_associations(other_person, assoc, duplicates_match, update_hash)

From ebb646326f3597696fd28a9e879a50520cbf23c4 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Wed, 29 Nov 2023 16:14:10 +0000
Subject: [PATCH 18/25] Use in_batches for update_all or destroy_all.

---
 lib/seek/merging/person_merge.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 6e13f6601c..d0660644c6 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -63,12 +63,12 @@ def merge_annotations(other_person)
       def merge_resources(other_person)
         # Contributed
         Person::RELATED_RESOURCE_TYPES.each do |resource_type|
-          other_person.send("contributed_#{resource_type.underscore.pluralize}").update_all(contributor_id: id)
+          other_person.send("contributed_#{resource_type.underscore.pluralize}").in_batches.update_all(contributor_id: id)
         end
         # Created
         duplicated = other_person.created_items.pluck(:id) & created_items.pluck(:id)
-        AssetsCreator.where(creator_id: other_person.id, asset_id: duplicated).destroy_all
-        AssetsCreator.where(creator_id: other_person.id).update_all(creator_id: id)
+        AssetsCreator.where(creator_id: other_person.id, asset_id: duplicated).in_batches.destroy_all
+        AssetsCreator.where(creator_id: other_person.id).in_batches.update_all(creator_id: id)
       end
 
       def merge_associations(other_person, assoc, duplicates_match, update_hash)
@@ -78,9 +78,9 @@ def merge_associations(other_person, assoc, duplicates_match, update_hash)
         duplicated = duplicated.map { |item| [item] } if duplicates_match.length == 1
 
         duplicated_hash = Hash[duplicates_match.zip(duplicated.transpose)]
-        other_person.send(assoc).where(duplicated_hash).destroy_all
+        other_person.send(assoc).where(duplicated_hash).in_batches.destroy_all
 
-        other_person.send(assoc).update_all(update_hash)
+        other_person.send(assoc).in_batches.update_all(update_hash)
       end
 
       def merge_user(other_person)
@@ -103,9 +103,9 @@ def merge_user_associations(other_person, assoc, duplicates_match, update_hash)
         duplicated = duplicated.map { |item| [item] } if duplicates_match.length == 1
 
         duplicated_hash = Hash[duplicates_match.zip(duplicated.transpose)]
-        other_person.user.send(assoc).where(duplicated_hash).destroy_all
+        other_person.user.send(assoc).where(duplicated_hash).in_batches.destroy_all
 
-        other_person.user.send(assoc).update_all(update_hash)
+        other_person.user.send(assoc).in_batches.update_all(update_hash)
       end
 
     end

From 40fcf7623e60837a7cebf929aabd2856debbfb88 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Wed, 29 Nov 2023 16:16:38 +0000
Subject: [PATCH 19/25] merge_user also interrupted if no other_person.user

---
 lib/seek/merging/person_merge.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index d0660644c6..c960efed75 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -84,7 +84,7 @@ def merge_associations(other_person, assoc, duplicates_match, update_hash)
       end
 
       def merge_user(other_person)
-        return unless user
+        return unless user && other_person.user
 
         merge_user_associations(other_person, 'identities',
                                 %i[provider uid], { user_id: user.id })

From 8d64fe9b7d6bd86c3eacb81547989a27ec9eb878 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Wed, 29 Nov 2023 16:27:07 +0000
Subject: [PATCH 20/25] Moved simple_attributes and annotation_types lists into
 merge methods.

---
 lib/seek/merging/person_merge.rb | 27 ++++-----------------------
 test/unit/person_test.rb         |  2 ++
 2 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index c960efed75..6f0fc7a353 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -24,37 +24,18 @@ def merge(other_person)
 
       private
 
-      # This attributes are directly copied from other_person if they are empty in the person to which its merging.
-      # first_letter is also updated
-      def simple_attributes
-        %i[
-          first_name
-          last_name
-          email
-          phone
-          skype_name
-          web_page
-          description
-          avatar_id
-          orcid
-        ]
-      end
-
       def merge_simple_attributes(other_person)
+        # This attributes are directly copied from other_person if they are empty in the person to which its merging.
+        # first_letter is also updated
+        simple_attributes = %i[first_name last_name email phone skype_name web_page description avatar_id orcid]
         simple_attributes.each do |attribute|
           send("#{attribute}=", other_person.send(attribute)) if send(attribute).nil?
         end
         update_first_letter
       end
 
-      def annotation_types
-        %w[
-          expertise
-          tools
-        ]
-      end
-
       def merge_annotations(other_person)
+        annotation_types = %w[expertise tools]
         annotation_types.each do |annotation_type|
           add_annotations(send(annotation_type) + other_person.send(annotation_type), annotation_type.singularize, self)
         end
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 4249d68609..21612c5c70 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1413,6 +1413,7 @@ def test_updated_not_changed_when_adding_notifiee_info
     person_to_keep = FactoryBot.create(:min_person)
     other_person = FactoryBot.create(:max_person)
     other_person_attributes = {}
+    simple_attributes = %i[first_name last_name email phone skype_name web_page description avatar_id orcid]
     simple_attributes.each do |attribute|
       other_person_attributes[attribute] = other_person.send(attribute)
     end
@@ -1436,6 +1437,7 @@ def test_updated_not_changed_when_adding_notifiee_info
     person_to_keep.save!
     person_to_keep.reload
     orig_annotations = {}
+    annotation_types = %w[expertise tools]
     annotation_types.each do |annotation_type|
       orig_annotations[annotation_type] = person_to_keep.send(annotation_type)
     end

From 8938b7a356e05f286136d8e987a86711d091727c Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Wed, 29 Nov 2023 16:51:53 +0000
Subject: [PATCH 21/25] Improved merge all types of resources test

---
 test/unit/person_test.rb | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 21612c5c70..cd308efb2d 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1526,21 +1526,25 @@ def test_updated_not_changed_when_adding_notifiee_info
     other_person = FactoryBot.create(:person)
 
     types_without_creators = %w[Event Strain]
+    contributed_items = []
+    created_items = []
     Person::RELATED_RESOURCE_TYPES.each do |resource_type|
       sym = resource_type == 'SampleType' ? :simple_sample_type : resource_type.underscore.to_sym
-      FactoryBot.create(sym, contributor: other_person)
+      co = FactoryBot.create(sym, contributor: other_person)
+      contributed_items << [co.class.to_s, co.id]
+      contributed_items << ['Investigation', co.investigation.id] if %w[Study Assay].include?(resource_type)
+      contributed_items << ['Study', co.study.id] if resource_type == 'Assay'
       unless types_without_creators.include?(resource_type)
-        FactoryBot.create(sym, contributor: FactoryBot.create(:person), creator_ids: [other_person.id])
+        cr = FactoryBot.create(sym, contributor: FactoryBot.create(:person), creator_ids: [other_person.id])
+        created_items << [cr.class.to_s, cr.id]
       end
     end
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
 
-    resource_types = Person::RELATED_RESOURCE_TYPES.length
-    added_isa_structure = 3 # An additional inv is created for the study, and inv and study are created for the assay
-    assert_equal resource_types + added_isa_structure, person_to_keep.contributed_items.length
-    assert_equal resource_types - types_without_creators.length, person_to_keep.created_items.length
+    assert_equal created_items.sort, (person_to_keep.created_items.map { |cr| [cr.class.to_s, cr.id] }).sort
+    assert_equal contributed_items.sort, (person_to_keep.contributed_items.map { |co| [co.class.to_s, co.id] }).sort
   end
 
   test 'merge permissions without duplication' do

From 35536fa5900e158e29735c19d158090849b9d7a0 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Wed, 29 Nov 2023 21:01:50 +0000
Subject: [PATCH 22/25] Merge annotations_by

---
 lib/seek/merging/person_merge.rb |  7 +++++++
 test/unit/person_test.rb         | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index 6f0fc7a353..a2c32b3a7f 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -39,6 +39,13 @@ def merge_annotations(other_person)
         annotation_types.each do |annotation_type|
           add_annotations(send(annotation_type) + other_person.send(annotation_type), annotation_type.singularize, self)
         end
+
+        # Update annotations by other_user
+        return unless user && other_person.user
+
+        merge_user_associations(other_person, 'annotations_by',
+                                %i[annotatable_type annotatable_id value_id],
+                                { source_id: user.id })
       end
 
       def merge_resources(other_person)
diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index cd308efb2d..aeaa81f0f8 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1457,6 +1457,25 @@ def test_updated_not_changed_when_adding_notifiee_info
     end
   end
 
+  test 'merge annotations_by' do
+    person_to_keep = FactoryBot.create(:person)
+    other_person = FactoryBot.create(:max_person)
+    sop = FactoryBot.create(:sop, creator_ids: [person_to_keep.id, other_person.id])
+    expected_a_by = []
+    a_by = FactoryBot.create(:tag, value: 'Tag_1', annotatable: sop, source: person_to_keep.user)
+    expected_a_by << [a_by.id, a_by.value_id]
+    a_by = FactoryBot.create(:tag, value: 'Tag_2', annotatable: sop, source: person_to_keep.user)
+    expected_a_by << [a_by.id, a_by.value_id]
+    FactoryBot.create(:tag, value: a_by.value, annotatable: sop, source: other_person.user)
+    a_by = FactoryBot.create(:tag, value: 'Tag_3', annotatable: sop, source: other_person.user)
+    expected_a_by << [a_by.id, a_by.value_id]
+
+    disable_authorization_checks { person_to_keep.merge(other_person) }
+    person_to_keep.reload
+
+    assert_equal expected_a_by.sort, person_to_keep.user.annotations_by.pluck(:id, :value_id)
+  end
+
   test 'merge group_memberships without duplication' do
     person_to_keep = FactoryBot.create(:person_in_multiple_projects)
     FactoryBot.create(:programme, project_ids: [person_to_keep.project_ids.last])

From f19cc32e6b6b33ffc7a47cd8c101a5e18e967d64 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Thu, 30 Nov 2023 14:21:24 +0000
Subject: [PATCH 23/25] simplify merge annotations test

---
 test/unit/person_test.rb | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index aeaa81f0f8..17a3b91315 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1434,27 +1434,13 @@ def test_updated_not_changed_when_adding_notifiee_info
     person_to_keep = FactoryBot.create(:min_person)
     person_to_keep.annotate_with(%w[golf merging], 'expertise', person_to_keep)
     person_to_keep.annotate_with(%w[merger], 'tool', person_to_keep)
-    person_to_keep.save!
-    person_to_keep.reload
-    orig_annotations = {}
-    annotation_types = %w[expertise tools]
-    annotation_types.each do |annotation_type|
-      orig_annotations[annotation_type] = person_to_keep.send(annotation_type)
-    end
     other_person = FactoryBot.create(:max_person)
-    other_annotations = {}
-    annotation_types.each do |annotation_type|
-      other_annotations[annotation_type] = other_person.send(annotation_type)
-    end
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
 
-    annotation_types.each do |annotation_type|
-      assert_equal (orig_annotations[annotation_type] + other_annotations[annotation_type]).compact.uniq.sort,
-                   person_to_keep.send(annotation_type).sort,
-                   "Should copy #{annotation_type} if not present in person to keep"
-    end
+    assert_equal %w[fishing golf merging].sort, person_to_keep.expertise.sort
+    assert_equal ['fishing rod', 'merger'].sort, person_to_keep.tools.sort
   end
 
   test 'merge annotations_by' do

From 122eb0220d3305143a3492c92d1e0352ff639a6c Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Thu, 30 Nov 2023 14:32:25 +0000
Subject: [PATCH 24/25] Improved merge permissions test

---
 test/unit/person_test.rb | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/test/unit/person_test.rb b/test/unit/person_test.rb
index 17a3b91315..16e48906db 100644
--- a/test/unit/person_test.rb
+++ b/test/unit/person_test.rb
@@ -1560,33 +1560,29 @@ def test_updated_not_changed_when_adding_notifiee_info
     sop_keep.policy.permissions.build(contributor: person_to_keep, access_type: Policy::MANAGING)
     sop_keep.policy.save
     sop_both = FactoryBot.create :sop
-    sop_both.policy.permissions.build(contributor: person_to_keep, access_type: Policy::MANAGING)
-    sop_both.policy.permissions.build(contributor: other_person, access_type: Policy::MANAGING)
+    sop_both.policy.permissions.build(contributor: person_to_keep, access_type: Policy::EDITING)
+    sop_both.policy.permissions.build(contributor: other_person, access_type: Policy::EDITING)
     sop_both.policy.save
     sop_other = FactoryBot.create :sop
-    sop_other.policy.permissions.build(contributor: other_person, access_type: Policy::MANAGING)
+    sop_other.policy.permissions.build(contributor: other_person, access_type: Policy::VISIBLE)
     sop_other.policy.save
 
-    assert sop_keep.can_manage?(person_to_keep)
-    assert sop_both.can_manage?(person_to_keep)
-    refute sop_other.can_manage?(person_to_keep)
-    refute sop_keep.can_manage?(other_person)
-    assert sop_both.can_manage?(other_person)
-    assert sop_other.can_manage?(other_person)
+    assert_equal [[person_to_keep.id, Policy::MANAGING]],
+                 sop_keep.policy.permissions.pluck(:contributor_id, :access_type)
+    assert_equal [[other_person.id, Policy::EDITING], [person_to_keep.id, Policy::EDITING]].sort,
+                 sop_both.policy.permissions.pluck(:contributor_id, :access_type)
+    assert_equal [[other_person.id, Policy::VISIBLE]],
+                 sop_other.policy.permissions.pluck(:contributor_id, :access_type)
 
     disable_authorization_checks { person_to_keep.merge(other_person) }
     person_to_keep.reload
-    sop_keep.reload
-    sop_both.reload
-    sop_other.reload
-    sop_keep.permission_for = nil
-    sop_both.permission_for = nil
-    sop_other.permission_for = nil
-
-    assert sop_keep.can_manage?(person_to_keep)
-    assert sop_both.can_manage?(person_to_keep)
-    assert sop_other.can_manage?(person_to_keep)
-    assert 1, sop_both.policy.permissions.length
+
+    assert_equal [[person_to_keep.id, Policy::MANAGING]],
+                 sop_keep.policy.permissions.pluck(:contributor_id, :access_type)
+    assert_equal [[person_to_keep.id, Policy::EDITING]],
+                 sop_both.policy.permissions.pluck(:contributor_id, :access_type)
+    assert_equal [[person_to_keep.id, Policy::VISIBLE]],
+                 sop_other.policy.permissions.pluck(:contributor_id, :access_type)
     assert 3, Permission.where(contributor_type: 'Person', contributor_id: person_to_keep.id).length
   end
 

From 84926caf3bea43f2f7225ebfe7d0503d6c43db38 Mon Sep 17 00:00:00 2001
From: fherreazcue <francisco.herreazcue@gmail.com>
Date: Thu, 30 Nov 2023 14:34:47 +0000
Subject: [PATCH 25/25] Use destroy!

---
 lib/seek/merging/person_merge.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/seek/merging/person_merge.rb b/lib/seek/merging/person_merge.rb
index a2c32b3a7f..4655c50558 100644
--- a/lib/seek/merging/person_merge.rb
+++ b/lib/seek/merging/person_merge.rb
@@ -16,7 +16,7 @@ def merge(other_person)
 
           save!
           other_person.reload # To prevent destruction of unlinked roles
-          other_person.destroy
+          other_person.destroy!
           ActivityLog.create!(action: 'MERGE-person',
                               data: "Person with id #{other_person.id} was merged into person with id #{id}.")
         end