BigW Consortium Gitlab

Deleting a user shouldn't delete associated issues.

- "Associated" issues are issues the user has created + issues that the user is assigned to. - Issues that a user owns are transferred to a "Ghost User" (just a regular user with `state = 'ghost'` that is created when `User.ghost` is called). - Issues that a user is assigned to are moved to the "Unassigned" state. - Fix a spec failure in `profile_spec` — a spec was asserting that when a user is deleted, `User.count` decreases by 1. After this change, deleting a user creates (potentially) a ghost user, causing `User.count` not to change. The spec has been updated to look for the relevant user in the assertion.
parent 29540d6f
...@@ -81,7 +81,6 @@ class User < ActiveRecord::Base ...@@ -81,7 +81,6 @@ class User < ActiveRecord::Base
has_many :authorized_projects, through: :project_authorizations, source: :project has_many :authorized_projects, through: :project_authorizations, source: :project
has_many :snippets, dependent: :destroy, foreign_key: :author_id has_many :snippets, dependent: :destroy, foreign_key: :author_id
has_many :issues, dependent: :destroy, foreign_key: :author_id
has_many :notes, dependent: :destroy, foreign_key: :author_id has_many :notes, dependent: :destroy, foreign_key: :author_id
has_many :merge_requests, dependent: :destroy, foreign_key: :author_id has_many :merge_requests, dependent: :destroy, foreign_key: :author_id
has_many :events, dependent: :destroy, foreign_key: :author_id has_many :events, dependent: :destroy, foreign_key: :author_id
...@@ -99,6 +98,11 @@ class User < ActiveRecord::Base ...@@ -99,6 +98,11 @@ class User < ActiveRecord::Base
has_many :assigned_issues, dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue" has_many :assigned_issues, dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue"
has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest"
# Issues that a user owns are expected to be moved to the "ghost" user before
# the user is destroyed. If the user owns any issues during deletion, this
# should be treated as an exceptional condition.
has_many :issues, dependent: :restrict_with_exception, foreign_key: :author_id
# #
# Validations # Validations
# #
...@@ -181,6 +185,8 @@ class User < ActiveRecord::Base ...@@ -181,6 +185,8 @@ class User < ActiveRecord::Base
"administrator if you think this is an error." "administrator if you think this is an error."
end end
end end
state :ghost
end end
mount_uploader :avatar, AvatarUploader mount_uploader :avatar, AvatarUploader
...@@ -337,6 +343,30 @@ class User < ActiveRecord::Base ...@@ -337,6 +343,30 @@ class User < ActiveRecord::Base
(?<user>#{Gitlab::Regex::NAMESPACE_REF_REGEX_STR}) (?<user>#{Gitlab::Regex::NAMESPACE_REF_REGEX_STR})
}x }x
end end
# Return (create if necessary) the ghost user. The ghost user
# owns records previously belonging to deleted users.
def ghost
ghost_user = User.with_state(:ghost).first
ghost_user ||
begin
users = Enumerator.new do |y|
n = nil
loop do
user = User.new(
username: "ghost#{n}", password: Devise.friendly_token,
email: "ghost#{n}@example.com", name: "Ghost User", state: :ghost
)
y.yield(user)
n = n ? n.next : 0
end
end
users.lazy.select { |user| user.valid? }.first.tap(&:save!)
end
end
end end
# #
......
...@@ -26,6 +26,8 @@ module Users ...@@ -26,6 +26,8 @@ module Users
::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
end end
move_issues_to_ghost_user(user)
# Destroy the namespace after destroying the user since certain methods may depend on the namespace existing # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
namespace = user.namespace namespace = user.namespace
user_data = user.destroy user_data = user.destroy
...@@ -33,5 +35,17 @@ module Users ...@@ -33,5 +35,17 @@ module Users
user_data user_data
end end
private
def move_issues_to_ghost_user(user)
ghost_user = User.ghost
Issue.transaction do
user.issues.each { |issue| issue.update!(author: ghost_user) }
end
user.reload
end
end end
end end
...@@ -4,7 +4,7 @@ describe 'Profile account page', feature: true do ...@@ -4,7 +4,7 @@ describe 'Profile account page', feature: true do
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
login_as :user login_as(user)
end end
describe 'when signup is enabled' do describe 'when signup is enabled' do
...@@ -16,7 +16,7 @@ describe 'Profile account page', feature: true do ...@@ -16,7 +16,7 @@ describe 'Profile account page', feature: true do
it { expect(page).to have_content('Remove account') } it { expect(page).to have_content('Remove account') }
it 'deletes the account' do it 'deletes the account' do
expect { click_link 'Delete account' }.to change { User.count }.by(-1) expect { click_link 'Delete account' }.to change { User.where(id: user.id).count }.by(-1)
expect(current_path).to eq(new_user_session_path) expect(current_path).to eq(new_user_session_path)
end end
end end
......
...@@ -22,7 +22,7 @@ describe User, models: true do ...@@ -22,7 +22,7 @@ describe User, models: true do
it { is_expected.to have_many(:deploy_keys).dependent(:destroy) } it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
it { is_expected.to have_many(:events).dependent(:destroy) } it { is_expected.to have_many(:events).dependent(:destroy) }
it { is_expected.to have_many(:recent_events).class_name('Event') } it { is_expected.to have_many(:recent_events).class_name('Event') }
it { is_expected.to have_many(:issues).dependent(:destroy) } it { is_expected.to have_many(:issues).dependent(:restrict_with_exception) }
it { is_expected.to have_many(:notes).dependent(:destroy) } it { is_expected.to have_many(:notes).dependent(:destroy) }
it { is_expected.to have_many(:assigned_issues).dependent(:nullify) } it { is_expected.to have_many(:assigned_issues).dependent(:nullify) }
it { is_expected.to have_many(:merge_requests).dependent(:destroy) } it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
...@@ -1490,4 +1490,41 @@ describe User, models: true do ...@@ -1490,4 +1490,41 @@ describe User, models: true do
expect(user.admin).to be true expect(user.admin).to be true
end end
end end
describe '.ghost' do
it "creates a ghost user if one isn't already present" do
ghost = User.ghost
expect(ghost).to be_ghost
expect(ghost).to be_persisted
end
it "does not create a second ghost user if one is already present" do
expect do
User.ghost
User.ghost
end.to change { User.count }.by(1)
expect(User.ghost).to eq(User.ghost)
end
context "when a regular user exists with the username 'ghost'" do
it "creates a ghost user with a non-conflicting username" do
create(:user, username: 'ghost')
ghost = User.ghost
expect(ghost).to be_persisted
expect(ghost.username).to eq('ghost0')
end
end
context "when a regular user exists with the email 'ghost@example.com'" do
it "creates a ghost user with a non-conflicting email" do
create(:user, email: 'ghost@example.com')
ghost = User.ghost
expect(ghost).to be_persisted
expect(ghost.email).to eq('ghost0@example.com')
end
end
end
end end
...@@ -24,6 +24,50 @@ describe Users::DestroyService, services: true do ...@@ -24,6 +24,50 @@ describe Users::DestroyService, services: true do
end end
end end
context "a deleted user's issues" do
let(:project) { create :project }
before do
project.add_developer(user)
end
context "for an issue the user has created" do
let!(:issue) { create(:issue, project: project, author: user) }
before do
service.execute(user)
end
it 'does not delete the issue' 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
migrated_issue = Issue.find_by_id(issue.id)
expect(migrated_issue.author).to eq(User.ghost)
end
end
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 issues the user is assigned to' do
expect(Issue.find_by_id(issue.id)).to be_present
end
it 'migrates the issue so that it is "Unassigned"' do
migrated_issue = Issue.find_by_id(issue.id)
expect(migrated_issue.assignee).to be_nil
end
end
end
context "solo owned groups present" do context "solo owned groups present" do
let(:solo_owned) { create(:group) } let(:solo_owned) { create(:group) }
let(:member) { create(:group_member) } let(:member) { create(:group_member) }
......
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