Merge branch '349411-migrate-resource-iteration-event-to-ghost-user' into 'master'

Fix handling of resource iteration events when deleting a User

See merge request gitlab-org/gitlab!82060

app/models/user.rb

@@ -391,7 +391,7 @@ class User < ApplicationRecord
 
     # rubocop: disable CodeReuse/ServiceClass
     # Ideally we should not call a service object here but user.block
-    # is also bcalled by Users::MigrateToGhostUserService which references
+    # is also called by Users::MigrateToGhostUserService which references
     # this state transition object in order to do a rollback.
     # For this reason the tradeoff is to disable this cop.
     after_transition any => :blocked do |user|

app/services/users/destroy_service.rb

@@ -54,7 +54,7 @@ module Users
 
       yield(user) if block_given?
 
-      MigrateToGhostUserService.new(user).execute unless options[:hard_delete]
+      MigrateToGhostUserService.new(user).execute(hard_delete: options[:hard_delete])
 
       response = Snippets::BulkDestroyService.new(current_user, user.snippets).execute(options)
       raise DestroyError, response.message if response.error?

app/services/users/migrate_to_ghost_user_service.rb

@@ -10,14 +10,21 @@ module Users
   class MigrateToGhostUserService
     extend ActiveSupport::Concern
 
-    attr_reader :ghost_user, :user
+    attr_reader :ghost_user, :user, :hard_delete
 
     def initialize(user)
       @user = user
       @ghost_user = User.ghost
     end
 
-    def execute
+    # If an admin attempts to hard delete a user, in some cases associated
+    # records may have a NOT NULL constraint on the user ID that prevent that record
+    # from being destroyed. In such situations we must assign the record to the ghost user.
+    # Passing in `hard_delete: true` will ensure these records get assigned to
+    # the ghost user before the user is destroyed. Other associated records will be destroyed.
+    # letting the other associated records be destroyed.
+    def execute(hard_delete: false)
+      @hard_delete = hard_delete
       transition = user.block_transition
 
       # Block the user before moving records to prevent a data race.
@@ -46,6 +53,8 @@ module Users
     private
 
     def migrate_records
+      return if hard_delete
+
       migrate_issues
       migrate_merge_requests
       migrate_notes

ee/app/models/resource_iteration_event.rb

 # frozen_string_literal: true
 
 class ResourceIterationEvent < ResourceTimeboxEvent
+  include EachBatch
+
   belongs_to :iteration
 
   scope :with_api_entity_associations, -> { preload(:iteration, :user) }
+  scope :by_user, -> (user) { where(user_id: user ) }
 end

ee/app/services/ee/users/migrate_to_ghost_user_service.rb

@@ -3,9 +3,16 @@
 module EE
   module Users
     module MigrateToGhostUserService
+      BATCH_SIZE = 1000
+
       private
 
       def migrate_records
+        # these should always be ghosted
+        migrate_resource_iteration_events
+
+        return super if hard_delete
+
         migrate_epics
         migrate_requirements_management_requirements
         migrate_vulnerabilities_feedback
@@ -27,6 +34,12 @@ module EE
         user.vulnerability_feedback.update_all(author_id: ghost_user.id)
         user.commented_vulnerability_feedback.update_all(comment_author_id: ghost_user.id)
       end
+
+      def migrate_resource_iteration_events
+        ResourceIterationEvent.by_user(user).each_batch(of: BATCH_SIZE) do |batch|
+          batch.update_all(user_id: ghost_user.id)
+        end
+      end
     end
   end
 end

ee/spec/services/ee/users/destroy_service_spec.rb

@@ -42,6 +42,20 @@ RSpec.describe Users::DestroyService do
         end
       end
 
+      context 'migrating associated records' do
+        context 'when hard_delete option is given' do
+          let!(:resource_iteration_event) { create(:resource_iteration_event, user: user) }
+
+          it 'will ghost certain records' do
+            expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original
+
+            service.execute(user, hard_delete: true)
+
+            expect(resource_iteration_event.reload.user).to be_ghost
+          end
+        end
+      end
+
       context 'when user has oncall rotations' do
         let(:schedule) { create(:incident_management_oncall_schedule, project: project) }
         let(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule) }

ee/spec/services/ee/users/migrate_to_ghost_user_service_spec.rb

@@ -3,10 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Users::MigrateToGhostUserService do
-  context 'epics' do
   let!(:user)   { create(:user) }
   let(:service) { described_class.new(user) }
+  let(:always_ghost) { false }
 
+  context 'epics' do
     context 'deleted user is present as both author and edited_user' do
       include_examples "migrating a deleted user's associated records to the ghost user", Epic, [:author, :last_edited_by] do
         let(:created_record) do
@@ -23,29 +24,28 @@ RSpec.describe Users::MigrateToGhostUserService do
   end
 
   context 'vulnerability_feedback author' do
-    let!(:user)   { create(:user) }
-    let(:service) { described_class.new(user) }
-
     include_examples "migrating a deleted user's associated records to the ghost user", Vulnerabilities::Feedback, [:author] do
       let(:created_record) { create(:vulnerability_feedback, author: user) }
     end
   end
 
   context 'vulnerability_feedback comment author' do
-    let!(:user)   { create(:user) }
-    let(:service) { described_class.new(user) }
-
     include_examples "migrating a deleted user's associated records to the ghost user", Vulnerabilities::Feedback, [:comment_author] do
       let(:created_record) { create(:vulnerability_feedback, comment_author: user) }
     end
   end
 
   context 'requirements' do
-    let!(:user)   { create(:user) }
-    let(:service) { described_class.new(user) }
-
     include_examples "migrating a deleted user's associated records to the ghost user", RequirementsManagement::Requirement, [:author] do
       let(:created_record) { create(:requirement, author: user) }
     end
   end
+
+  context 'resource_iteration_events' do
+    let(:always_ghost) { true }
+
+    include_examples "migrating a deleted user's associated records to the ghost user", ResourceIterationEvent, [:user] do
+      let(:created_record) { create(:resource_iteration_event, issue: create(:issue), user: user, iteration: create(:iteration)) }
+    end
+  end
 end

spec/services/users/destroy_service_spec.rb

@@ -215,7 +215,7 @@ RSpec.describe Users::DestroyService do
       end
     end
 
-    context "migrating associated records" do
+    context 'migrating associated records' do
       let!(:issue) { create(:issue, author: user) }
 
       it 'delegates to the `MigrateToGhostUser` service to move associated records to the ghost user' do
@@ -226,14 +226,16 @@ RSpec.describe Users::DestroyService do
         expect(issue.reload.author).to be_ghost
       end
 
-      it 'does not run `MigrateToGhostUser` if hard_delete option is given' do
-        expect_any_instance_of(Users::MigrateToGhostUserService).not_to receive(:execute)
+      context 'when hard_delete option is given' do
+        it 'will not ghost certain records' do
+          expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original
 
           service.execute(user, hard_delete: true)
 
           expect(Issue.exists?(issue.id)).to be_falsy
         end
       end
+    end
 
     describe "user personal's repository removal" do
       context 'storages' do

spec/services/users/migrate_to_ghost_user_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Users::MigrateToGhostUserService do
   let!(:user)        { create(:user) }
   let!(:project)     { create(:project, :repository) }
   let(:service)      { described_class.new(user) }
+  let(:always_ghost) { false }
 
   context "migrating a user's associated records to the ghost user" do
     context 'issues' do

spec/support/services/migrate_to_ghost_user_service_shared_examples.rb

@@ -42,6 +42,18 @@ RSpec.shared_examples "migrating a deleted user's associated records to the ghos
       end
     end
 
+    it 'will only migrate specific records during a hard_delete' do
+      service.execute(hard_delete: true)
+
+      migrated_record = record_class.find_by_id(record.id)
+
+      check_user = always_ghost ? User.ghost : user
+
+      migrated_fields.each do |field|
+        expect(migrated_record.public_send(field)).to eq(check_user)
+      end
+    end
+
     context "race conditions" do
       context "when #{record_class_name} migration fails and is rolled back" do
         before do