diff options
| -rw-r--r-- | app/services/concerns/users/migrate_to_ghost_user.rb | 38 | ||||
| -rw-r--r-- | app/services/users/destroy_service.rb | 21 | ||||
| -rw-r--r-- | spec/services/users/destroy_service_spec.rb (renamed from spec/services/users/destroy_spec.rb) | 16 | ||||
| -rw-r--r-- | spec/support/services/user_destroy_service_migrate_to_ghost_user_shared_examples.rb | 53 |
4 files changed, 110 insertions, 18 deletions
diff --git a/app/services/concerns/users/migrate_to_ghost_user.rb b/app/services/concerns/users/migrate_to_ghost_user.rb new file mode 100644 index 00000000000..ecbbf3026a0 --- /dev/null +++ b/app/services/concerns/users/migrate_to_ghost_user.rb @@ -0,0 +1,38 @@ +# 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 `move_issues_to_ghost_user` + # runs and before the user is destroyed, the destroy will fail with + # an exception. + user.block + + user.transaction do + @ghost_user = User.ghost + + move_issues_to_ghost_user(user) + move_merge_requests_to_ghost_user(user) + end + + user.reload + end + + private + + def move_issues_to_ghost_user(user) + user.issues.update_all(author_id: ghost_user.id) + end + + def move_merge_requests_to_ghost_user(user) + user.merge_requests.update_all(author_id: ghost_user.id) + end +end diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index a3b32a71a64..e6608e316dc 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -1,5 +1,7 @@ module Users class DestroyService + include MigrateToGhostUser + attr_accessor :current_user def initialize(current_user) @@ -26,7 +28,7 @@ module Users ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute end - move_issues_to_ghost_user(user) + move_associated_records_to_ghost_user(user) # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing namespace = user.namespace @@ -35,22 +37,5 @@ module Users user_data end - - private - - def move_issues_to_ghost_user(user) - # Block the user before moving issues to prevent a data race. - # If the user creates an issue after `move_issues_to_ghost_user` - # runs and before the user is destroyed, the destroy will fail with - # an exception. We block the user so that issues can't be created - # after `move_issues_to_ghost_user` runs and before the destroy happens. - user.block - - ghost_user = User.ghost - - user.issues.update_all(author_id: ghost_user.id) - - user.reload - end end end diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_service_spec.rb index 66c61b7f8ff..a5b69e6ddf4 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -141,5 +141,21 @@ describe Users::DestroyService, services: true do expect(User.exists?(user.id)).to be(false) 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 + end end end diff --git a/spec/support/services/user_destroy_service_migrate_to_ghost_user_shared_examples.rb b/spec/support/services/user_destroy_service_migrate_to_ghost_user_shared_examples.rb new file mode 100644 index 00000000000..8996e3420e6 --- /dev/null +++ b/spec/support/services/user_destroy_service_migrate_to_ghost_user_shared_examples.rb @@ -0,0 +1,53 @@ +require "spec_helper" + +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) } + + before do + project.add_developer(user) + end + + context "for a #{record_class_name} the user has created" do + let!(:record) { created_record } + + it "does not delete the #{record_class_name}" do + service.execute(user) + + 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) + + migrated_record = record_class.find_by_id(record.id) + + expect(migrated_record.author).to eq(User.ghost) + end + + it "blocks the user before migrating #{record_class_name}s to the 'Ghost User'" do + service.execute(user) + + expect(user).to be_blocked + end + end + + 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 |