Implement review comments from @DouweM for !10467.28695-move-all-associated-records-to-ghost-user

1. Have `MigrateToGhostUser` be a service rather than a mixed-in module, to keep
   things explicit. Specs testing the behavior of this class are moved into a
   separate service spec file.

2. Add a `user.reported_abuse_reports` association to make the
   `migrate_abuse_reports` method more consistent with the other `migrate_`
   methods.

8 files changed, 153 insertions, 148 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 8c2f0011be8..50d7687c255 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -90,6 +90,7 @@ class User < ActiveRecord::Base
   has_many :recent_events, -> { order "id DESC" }, foreign_key: :author_id,   class_name: "Event"
   has_many :oauth_applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy
   has_one  :abuse_report,             dependent: :destroy, foreign_key: :user_id
+  has_many :reported_abuse_reports,   dependent: :destroy, foreign_key: :reporter_id, class_name: "AbuseReport"
   has_many :spam_logs,                dependent: :destroy
   has_many :builds,                   dependent: :nullify, class_name: 'Ci::Build'
   has_many :pipelines,                dependent: :nullify, class_name: 'Ci::Pipeline'
diff --git a/app/services/concerns/users/migrate_to_ghost_user.rb b/app/services/concerns/users/migrate_to_ghost_user.rb
deleted file mode 100644
index 76335e45e54..00000000000
--- a/app/services/concerns/users/migrate_to_ghost_user.rb
+++ /dev/null
@@ -1,53 +0,0 @@
-# When a user is destroyed, some of their associated records are
-# moved to a "Ghost User", to prevent these associated records from
-# being destroyed.
-#
-# For example, all the issues/MRs a user has created are _not_ destroyed
-# when the user is destroyed.
-module Users::MigrateToGhostUser
-  extend ActiveSupport::Concern
-
-  attr_reader :ghost_user
-
-  def move_associated_records_to_ghost_user(user)
-    # Block the user before moving records to prevent a data race.
-    # For example, if the user creates an issue after `migrate_issues`
-    # runs and before the user is destroyed, the destroy will fail with
-    # an exception.
-    user.block
-
-    user.transaction do
-      @ghost_user = User.ghost
-
-      migrate_issues(user)
-      migrate_merge_requests(user)
-      migrate_notes(user)
-      migrate_abuse_reports(user)
-      migrate_award_emoji(user)
-    end
-
-    user.reload
-  end
-
-  private
-
-  def migrate_issues(user)
-    user.issues.update_all(author_id: ghost_user.id)
-  end
-
-  def migrate_merge_requests(user)
-    user.merge_requests.update_all(author_id: ghost_user.id)
-  end
-
-  def migrate_notes(user)
-    user.notes.update_all(author_id: ghost_user.id)
-  end
-
-  def migrate_abuse_reports(user)
-    AbuseReport.where(reporter_id: user.id).update_all(reporter_id: ghost_user.id)
-  end
-
-  def migrate_award_emoji(user)
-    user.award_emoji.update_all(user_id: ghost_user.id)
-  end
-end
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index e6608e316dc..ba58b174cc0 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -1,7 +1,5 @@
 module Users
   class DestroyService
-    include MigrateToGhostUser
-
     attr_accessor :current_user
 
     def initialize(current_user)
@@ -28,7 +26,7 @@ module Users
         ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
       end
 
-      move_associated_records_to_ghost_user(user)
+      MigrateToGhostUserService.new(user).execute
 
       # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
       namespace = user.namespace
diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
new file mode 100644
index 00000000000..1e1ed1791ec
--- /dev/null
+++ b/app/services/users/migrate_to_ghost_user_service.rb
@@ -0,0 +1,59 @@
+# When a user is destroyed, some of their associated records are
+# moved to a "Ghost User", to prevent these associated records from
+# being destroyed.
+#
+# For example, all the issues/MRs a user has created are _not_ destroyed
+# when the user is destroyed.
+module Users
+  class MigrateToGhostUserService
+    extend ActiveSupport::Concern
+
+    attr_reader :ghost_user, :user
+
+    def initialize(user)
+      @user = user
+    end
+
+    def execute
+      # Block the user before moving records to prevent a data race.
+      # For example, if the user creates an issue after `migrate_issues`
+      # runs and before the user is destroyed, the destroy will fail with
+      # an exception.
+      user.block
+
+      user.transaction do
+        @ghost_user = User.ghost
+
+        migrate_issues
+        migrate_merge_requests
+        migrate_notes
+        migrate_abuse_reports
+        migrate_award_emoji
+      end
+
+      user.reload
+    end
+
+    private
+
+    def migrate_issues
+      user.issues.update_all(author_id: ghost_user.id)
+    end
+
+    def migrate_merge_requests
+      user.merge_requests.update_all(author_id: ghost_user.id)
+    end
+
+    def migrate_notes
+      user.notes.update_all(author_id: ghost_user.id)
+    end
+
+    def migrate_abuse_reports
+      user.reported_abuse_reports.update_all(reporter_id: ghost_user.id)
+    end
+
+    def migrate_award_emoji
+      user.award_emoji.update_all(user_id: ghost_user.id)
+    end
+  end
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 6a787f6b0df..3f9c013ce37 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -36,6 +36,7 @@ describe User, models: true do
     it { is_expected.to have_many(:pipelines).dependent(:nullify) }
     it { is_expected.to have_many(:chat_names).dependent(:destroy) }
     it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:reported_abuse_reports).dependent(:destroy).class_name('AbuseReport') }
 
     describe "#abuse_report" do
       let(:current_user) { create(:user) }
diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb
index d3463185ebe..43c18992d1a 100644
--- a/spec/services/users/destroy_service_spec.rb
+++ b/spec/services/users/destroy_service_spec.rb
@@ -46,43 +46,47 @@ describe Users::DestroyService, services: true do
         project.add_developer(user)
       end
 
-      context "for an issue the user has created" do
-        let!(:issue) { create(:issue, project: project, author: user) }
+      context "for an issue the user was assigned to" do
+        let!(:issue) { create(:issue, project: project, assignee: user) }
 
         before do
           service.execute(user)
         end
 
-        it 'does not delete the issue' do
+        it 'does not delete issues the user is assigned to' do
           expect(Issue.find_by_id(issue.id)).to be_present
         end
 
-        it 'migrates the issue so that the "Ghost User" is the issue owner' do
+        it 'migrates the issue so that it is "Unassigned"' do
           migrated_issue = Issue.find_by_id(issue.id)
 
-          expect(migrated_issue.author).to eq(User.ghost)
+          expect(migrated_issue.assignee).to be_nil
         end
+      end
+    end
 
-        it 'blocks the user before migrating issues to the "Ghost User' do
-          expect(user).to be_blocked
-        end
+    context "a deleted user's merge_requests" do
+      let(:project) { create(:project) }
+
+      before do
+        project.add_developer(user)
       end
 
-      context "for an issue the user was assigned to" do
-        let!(:issue) { create(:issue, project: project, assignee: user) }
+      context "for an merge request the user was assigned to" do
+        let!(:merge_request) { create(:merge_request, source_project: project, assignee: user) }
 
         before do
           service.execute(user)
         end
 
-        it 'does not delete issues the user is assigned to' do
-          expect(Issue.find_by_id(issue.id)).to be_present
+        it 'does not delete merge requests the user is assigned to' do
+          expect(MergeRequest.find_by_id(merge_request.id)).to be_present
         end
 
-        it 'migrates the issue so that it is "Unassigned"' do
-          migrated_issue = Issue.find_by_id(issue.id)
+        it 'migrates the merge request so that it is "Unassigned"' do
+          migrated_merge_request = MergeRequest.find_by_id(merge_request.id)
 
-          expect(migrated_issue.assignee).to be_nil
+          expect(migrated_merge_request.assignee).to be_nil
         end
       end
     end
@@ -142,60 +146,11 @@ describe Users::DestroyService, services: true do
       end
     end
 
-    context 'migrating associated records to the ghost user' do
-      context 'issues'  do
-        include_examples "migrating a deleted user's associated records to the ghost user", Issue, {} do
-          let(:created_record) { create(:issue, project: project, author: user) }
-          let(:assigned_record) { create(:issue, project: project, assignee: user) }
-        end
-      end
-
-      context 'merge requests' do
-        include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest, {} do
-          let(:created_record) { create(:merge_request, source_project: project, author: user, target_branch: "first") }
-          let(:assigned_record) { create(:merge_request, source_project: project, assignee: user, target_branch: 'second') }
-        end
-      end
-
-      context 'notes' do
-        include_examples "migrating a deleted user's associated records to the ghost user", Note, { skip_assignee_specs: true } do
-          let(:created_record) { create(:note, project: project, author: user) }
-        end
-      end
-
-      context 'abuse reports' do
-        include_examples "migrating a deleted user's associated records to the ghost user", AbuseReport, { skip_assignee_specs: true } do
-          let(:created_record) { create(:abuse_report, reporter: user, user: create(:user)) }
-        end
-      end
+    context "migrating associated records" do
+      it 'delegates to the `MigrateToGhostUser` service to move associated records to the ghost user' do
+        expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once
 
-      context 'award emoji' do
-        include_examples "migrating a deleted user's associated records to the ghost user", AwardEmoji, { skip_assignee_specs: true } do
-          let(:created_record) { create(:award_emoji, user: user) }
-          let(:author_alias) { :user }
-
-          context "when the awardable already has an award emoji of the same name assigned to the ghost user" do
-            let(:awardable) { create(:issue) }
-            let!(:existing_award_emoji) { create(:award_emoji, user: User.ghost, name: "thumbsup", awardable: awardable) }
-            let!(:award_emoji) { create(:award_emoji, user: user, name: "thumbsup", awardable: awardable) }
-
-            it "migrates the award emoji regardless" do
-              service.execute(user)
-
-              migrated_record = AwardEmoji.find_by_id(award_emoji.id)
-
-              expect(migrated_record.user).to eq(User.ghost)
-            end
-
-            it "does not leave the migrated award emoji in an invalid state" do
-              service.execute(user)
-
-              migrated_record = AwardEmoji.find_by_id(award_emoji.id)
-
-              expect(migrated_record).to be_valid
-            end
-          end
-        end
+        service.execute(user)
       end
     end
   end
diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb
new file mode 100644
index 00000000000..8c5b7e41c15
--- /dev/null
+++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb
@@ -0,0 +1,64 @@
+require 'spec_helper'
+
+describe Users::MigrateToGhostUserService, services: true do
+  let!(:user)      { create(:user) }
+  let!(:project)   { create(:project) }
+  let(:service)    { described_class.new(user) }
+
+  context "migrating a user's associated records to the ghost user" do
+    context 'issues'  do
+      include_examples "migrating a deleted user's associated records to the ghost user", Issue do
+        let(:created_record) { create(:issue, project: project, author: user) }
+        let(:assigned_record) { create(:issue, project: project, assignee: user) }
+      end
+    end
+
+    context 'merge requests' do
+      include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest do
+        let(:created_record) { create(:merge_request, source_project: project, author: user, target_branch: "first") }
+        let(:assigned_record) { create(:merge_request, source_project: project, assignee: user, target_branch: 'second') }
+      end
+    end
+
+    context 'notes' do
+      include_examples "migrating a deleted user's associated records to the ghost user", Note do
+        let(:created_record) { create(:note, project: project, author: user) }
+      end
+    end
+
+    context 'abuse reports' do
+      include_examples "migrating a deleted user's associated records to the ghost user", AbuseReport do
+        let(:created_record) { create(:abuse_report, reporter: user, user: create(:user)) }
+      end
+    end
+
+    context 'award emoji' do
+      include_examples "migrating a deleted user's associated records to the ghost user", AwardEmoji do
+        let(:created_record) { create(:award_emoji, user: user) }
+        let(:author_alias) { :user }
+
+        context "when the awardable already has an award emoji of the same name assigned to the ghost user" do
+          let(:awardable) { create(:issue) }
+          let!(:existing_award_emoji) { create(:award_emoji, user: User.ghost, name: "thumbsup", awardable: awardable) }
+          let!(:award_emoji) { create(:award_emoji, user: user, name: "thumbsup", awardable: awardable) }
+
+          it "migrates the award emoji regardless" do
+            service.execute
+
+            migrated_record = AwardEmoji.find_by_id(award_emoji.id)
+
+            expect(migrated_record.user).to eq(User.ghost)
+          end
+
+          it "does not leave the migrated award emoji in an invalid state" do
+            service.execute
+
+            migrated_record = AwardEmoji.find_by_id(award_emoji.id)
+
+            expect(migrated_record).to be_valid
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/support/services/user_destroy_service_migrate_to_ghost_user_shared_examples.rb b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
index 3820ffc283f..0eac587e973 100644
--- a/spec/support/services/user_destroy_service_migrate_to_ghost_user_shared_examples.rb
+++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
@@ -1,6 +1,6 @@
 require "spec_helper"
 
-shared_examples "migrating a deleted user's associated records to the ghost user" do |record_class, options|
+shared_examples "migrating a deleted user's associated records to the ghost user" do |record_class|
   record_class_name = record_class.to_s.titleize.downcase
 
   let(:project) { create(:project) }
@@ -13,13 +13,13 @@ shared_examples "migrating a deleted user's associated records to the ghost user
     let!(:record) { created_record }
 
     it "does not delete the #{record_class_name}" do
-      service.execute(user)
+      service.execute
 
       expect(record_class.find_by_id(record.id)).to be_present
     end
 
     it "migrates the #{record_class_name} so that the 'Ghost User' is the #{record_class_name} owner" do
-      service.execute(user)
+      service.execute
 
       migrated_record = record_class.find_by_id(record.id)
 
@@ -31,29 +31,9 @@ shared_examples "migrating a deleted user's associated records to the ghost user
     end
 
     it "blocks the user before migrating #{record_class_name}s to the 'Ghost User'" do
-      service.execute(user)
+      service.execute
 
       expect(user).to be_blocked
     end
   end
-
-  unless options[:skip_assignee_specs]
-    context "for a #{record_class_name} the user was assigned to" do
-      let!(:record) { assigned_record }
-
-      before do
-        service.execute(user)
-      end
-
-      it "does not delete #{record_class_name}s the user is assigned to" do
-        expect(record_class.find_by_id(record.id)).to be_present
-      end
-
-      it "migrates the #{record_class_name} so that it is 'Unassigned'" do
-        migrated_record = record_class.find_by_id(record.id)
-
-        expect(migrated_record.assignee).to be_nil
-      end
-    end
-  end
 end