Commit 1c42505b authored by Timothy Andrew's avatar Timothy Andrew

Implement review comments from @DouweM for !10467.

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.
parent e152f3f3
......@@ -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'
......
# 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
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
......
# 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
......@@ -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) }
......
......@@ -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
......
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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment