Commit bef1aca8 authored by Douwe Maan's avatar Douwe Maan

Merge branch '28695-move-all-associated-records-to-ghost-user' into 'master'

Resolve "Deleting a user shouldn't delete associated records"

Closes #28695 and #30514

See merge request !10467
parents 9205caa3 1c42505b
......@@ -3,13 +3,14 @@ class AwardEmoji < ActiveRecord::Base
UPVOTE_NAME = "thumbsup".freeze
include Participable
include GhostUser
belongs_to :awardable, polymorphic: true
belongs_to :user
validates :awardable, :user, presence: true
validates :name, presence: true, inclusion: { in: Gitlab::Emoji.emojis_names }
validates :name, uniqueness: { scope: [:user, :awardable_type, :awardable_id] }
validates :name, uniqueness: { scope: [:user, :awardable_type, :awardable_id] }, unless: :ghost_user?
participant :user
......
module GhostUser
extend ActiveSupport::Concern
def ghost_user?
user && user.ghost?
end
end
......@@ -89,7 +89,8 @@ class User < ActiveRecord::Base
has_many :subscriptions, dependent: :destroy
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
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'
......
......@@ -26,7 +26,7 @@ module Users
::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
end
move_issues_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
......@@ -35,22 +35,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
# 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
---
title: Deleting a user should not delete associated records
merge_request: 10467
author:
......@@ -2,3 +2,4 @@
- [Preferences](../user/profile/preferences.md)
- [Two-factor Authentication (2FA)](../user/profile/account/two_factor_authentication.md)
- [Deleting your account](../user/profile/account/delete_account.md)
# Deleting a User Account
- As a user, you can delete your own account by navigating to **Settings** > **Account** and selecting **Delete account**
- As an admin, you can delete a user account by navigating to the **Admin Area**, selecting the **Users** tab, selecting a user, and clicking on **Remvoe user**
## Associated Records
> Introduced for issues in [GitLab 9.0][ce-7393], and for merge requests, award emoji, notes, and abuse reports in [GitLab 9.1][ce-10467].
When a user account is deleted, not all associated records are deleted with it. Here's a list of things that will not be deleted:
- Issues that the user created
- Merge requests that the user created
- Notes that the user created
- Abuse reports that the user reported
- Award emoji that the user craeted
Instead of being deleted, these records will be moved to a system-wide "Ghost User", whose sole purpose is to act as a container for such records.
[ce-7393]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7393
[ce-10467]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10467
......@@ -25,6 +25,20 @@ describe AwardEmoji, models: true do
expect(new_award).not_to be_valid
end
# Assume User A and User B both created award emoji of the same name
# on the same awardable. When User A is deleted, User A's award emoji
# is moved to the ghost user. When User B is deleted, User B's award emoji
# also needs to be moved to the ghost user - this cannot happen unless
# the uniqueness validation is disabled for ghost users.
it "allows duplicate award emoji for ghost users" do
user = create(:user, :ghost)
issue = create(:issue)
create(:award_emoji, user: user, awardable: issue)
new_award = build(:award_emoji, user: user, awardable: issue)
expect(new_award).to be_valid
end
end
end
end
......@@ -28,7 +28,6 @@ describe User, models: true do
it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
it { is_expected.to have_many(:assigned_merge_requests).dependent(:nullify) }
it { is_expected.to have_many(:identities).dependent(:destroy) }
it { is_expected.to have_one(:abuse_report) }
it { is_expected.to have_many(:spam_logs).dependent(:destroy) }
it { is_expected.to have_many(:todos).dependent(:destroy) }
it { is_expected.to have_many(:award_emoji).dependent(:destroy) }
......@@ -37,6 +36,34 @@ 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) }
let(:other_user) { create(:user) }
it { is_expected.to have_one(:abuse_report) }
it "refers to the abuse report whose user_id is the current user" do
abuse_report = create(:abuse_report, reporter: other_user, user: current_user)
expect(current_user.abuse_report).to eq(abuse_report)
end
it "does not refer to the abuse report whose reporter_id is the current user" do
create(:abuse_report, reporter: current_user, user: other_user)
expect(current_user.abuse_report).to be_nil
end
it "does not update the user_id of an abuse report when the user is updated" do
abuse_report = create(:abuse_report, reporter: current_user, user: other_user)
current_user.block
expect(abuse_report.reload.user).to eq(other_user)
end
end
describe '#group_members' do
it 'does not include group memberships for which user is a requester' do
......
......@@ -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
it 'blocks the user before migrating issues to the "Ghost User' do
expect(user).to be_blocked
end
end
context "for an issue the user was assigned to" do
let!(:issue) { create(:issue, project: project, assignee: user) }
context "a deleted user's merge_requests" do
let(:project) { create(:project) }
before do
project.add_developer(user)
end
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
......@@ -141,5 +145,13 @@ describe Users::DestroyService, services: true do
expect(User.exists?(user.id)).to be(false)
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
service.execute(user)
end
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|
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
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
migrated_record = record_class.find_by_id(record.id)
if migrated_record.respond_to?(:author)
expect(migrated_record.author).to eq(User.ghost)
else
expect(migrated_record.send(author_alias)).to eq(User.ghost)
end
end
it "blocks the user before migrating #{record_class_name}s to the 'Ghost User'" do
service.execute
expect(user).to be_blocked
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