diff --git a/app/models/user.rb b/app/models/user.rb
index e49a39079ef3a0b00d8e310e68723f0888ac858c..fa58455ad358ceb57b5973cc00fa5024525d7b34 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -391,7 +391,7 @@ def blocked?
# 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|
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index 4ec875098fa0210b22d2175e291e0c83eac808d2..46eec08212587b9ba56c91b1e8f207cf607a0d62 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -54,7 +54,7 @@ def execute(user, options = {})
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?
diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
index 515d782141647f243c27c2292d07107a932c8309..575614e8743ca62a538c991121177aadfa47b0e1 100644
--- a/app/services/users/migrate_to_ghost_user_service.rb
+++ b/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 @@ def execute
private
def migrate_records
+ return if hard_delete
+
migrate_issues
migrate_merge_requests
migrate_notes
diff --git a/ee/app/models/resource_iteration_event.rb b/ee/app/models/resource_iteration_event.rb
index 14c36d3029c6319f357a11169c3a92f1c7417f65..69ca9269c8e3ec6b3f9908e38ea13f2927a78677 100644
--- a/ee/app/models/resource_iteration_event.rb
+++ b/ee/app/models/resource_iteration_event.rb
@@ -1,7 +1,10 @@
# 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
diff --git a/ee/app/services/ee/users/migrate_to_ghost_user_service.rb b/ee/app/services/ee/users/migrate_to_ghost_user_service.rb
index ee5172298258f543caf12d98ea1b8a9d79b38d53..02d0209a634281c576f09386eda9d9ba38fbbe30 100644
--- a/ee/app/services/ee/users/migrate_to_ghost_user_service.rb
+++ b/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 @@ def migrate_vulnerabilities_feedback
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
diff --git a/ee/spec/services/ee/users/destroy_service_spec.rb b/ee/spec/services/ee/users/destroy_service_spec.rb
index cdbad571b051984f28b2b047365c5bdf831efe68..c767c83671e8ca5c5ee2edd84778f565c2d05b9d 100644
--- a/ee/spec/services/ee/users/destroy_service_spec.rb
+++ b/ee/spec/services/ee/users/destroy_service_spec.rb
@@ -42,6 +42,20 @@
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) }
diff --git a/ee/spec/services/ee/users/migrate_to_ghost_user_service_spec.rb b/ee/spec/services/ee/users/migrate_to_ghost_user_service_spec.rb
index 79c25dcb03c412108f03db2013d404abcba01175..c3fb1f8d95bd2ca28e5bf0c1a6b36369dcc3617d 100644
--- a/ee/spec/services/ee/users/migrate_to_ghost_user_service_spec.rb
+++ b/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!(: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 @@
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
diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb
index 76b84e3b4aba48b88934398d7cb3a40bf680eedf..602db66dba1708f9090fbcb30e75aee75a46d7a8 100644
--- a/spec/services/users/destroy_service_spec.rb
+++ b/spec/services/users/destroy_service_spec.rb
@@ -215,8 +215,8 @@
end
end
- context "migrating associated records" do
- let!(:issue) { create(:issue, author: user) }
+ 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
expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original
@@ -226,12 +226,14 @@
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)
+ service.execute(user, hard_delete: true)
- expect(Issue.exists?(issue.id)).to be_falsy
+ expect(Issue.exists?(issue.id)).to be_falsy
+ 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
index c36889f20ec4dfa97de8fedec21c63ab13c14192..073ebaae5b07345a8504e7bd6963b69992a93222 100644
--- a/spec/services/users/migrate_to_ghost_user_service_spec.rb
+++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb
@@ -3,9 +3,10 @@
require 'spec_helper'
RSpec.describe Users::MigrateToGhostUserService do
- let!(:user) { create(:user) }
- let!(:project) { create(:project, :repository) }
- let(:service) { described_class.new(user) }
+ 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
diff --git a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
index 2fbc01a9195b1c5e2780a487bd7a3f18752ec7aa..1e291a90163d2910012ded0df8d193e6caf9fdae 100644
--- a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
+++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb
@@ -42,6 +42,18 @@
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