diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-04-06 21:54:57 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-04-06 21:54:57 +0300 |
commit | bef1aca8837f2306ecbd8ff70deb2204bf1da539 (patch) | |
tree | 8b8e1e2edaca871756f98bfa991aeff0a69bb106 | |
parent | 9205caa3361fb38233588c8a18d27288b1e076ee (diff) | |
parent | 1c42505b026d922df50c59d5f9e85073b5f5345f (diff) |
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
-rw-r--r-- | app/models/award_emoji.rb | 3 | ||||
-rw-r--r-- | app/models/concerns/ghost_user.rb | 7 | ||||
-rw-r--r-- | app/models/user.rb | 3 | ||||
-rw-r--r-- | app/services/users/destroy_service.rb | 19 | ||||
-rw-r--r-- | app/services/users/migrate_to_ghost_user_service.rb | 59 | ||||
-rw-r--r-- | changelogs/unreleased/28695-move-all-associated-records-to-ghost-user.yml | 4 | ||||
-rw-r--r-- | doc/profile/README.md | 1 | ||||
-rw-r--r-- | doc/user/profile/account/delete_account.md | 25 | ||||
-rw-r--r-- | spec/models/award_emoji_spec.rb | 14 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 29 | ||||
-rw-r--r-- | spec/services/users/destroy_service_spec.rb (renamed from spec/services/users/destroy_spec.rb) | 42 | ||||
-rw-r--r-- | spec/services/users/migrate_to_ghost_user_service_spec.rb | 64 | ||||
-rw-r--r-- | spec/support/services/migrate_to_ghost_user_service_shared_examples.rb | 39 |
13 files changed, 273 insertions, 36 deletions
diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb index 6937ad3bdd9..6ada6fae4eb 100644 --- a/app/models/award_emoji.rb +++ b/app/models/award_emoji.rb @@ -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 diff --git a/app/models/concerns/ghost_user.rb b/app/models/concerns/ghost_user.rb new file mode 100644 index 00000000000..da696127a80 --- /dev/null +++ b/app/models/concerns/ghost_user.rb @@ -0,0 +1,7 @@ +module GhostUser + extend ActiveSupport::Concern + + def ghost_user? + user && user.ghost? + end +end diff --git a/app/models/user.rb b/app/models/user.rb index c358b1b8d2b..87eeee204f8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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' diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index a3b32a71a64..ba58b174cc0 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -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 diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb new file mode 100644 index 00000000000..1e1ed1791ec --- /dev/null +++ b/app/services/users/migrate_to_ghost_user_service.rb @@ -0,0 +1,59 @@ +# 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 diff --git a/changelogs/unreleased/28695-move-all-associated-records-to-ghost-user.yml b/changelogs/unreleased/28695-move-all-associated-records-to-ghost-user.yml new file mode 100644 index 00000000000..c5dcde48028 --- /dev/null +++ b/changelogs/unreleased/28695-move-all-associated-records-to-ghost-user.yml @@ -0,0 +1,4 @@ +--- +title: Deleting a user should not delete associated records +merge_request: 10467 +author: diff --git a/doc/profile/README.md b/doc/profile/README.md index 54e44d65959..aed64ac1228 100644 --- a/doc/profile/README.md +++ b/doc/profile/README.md @@ -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) diff --git a/doc/user/profile/account/delete_account.md b/doc/user/profile/account/delete_account.md new file mode 100644 index 00000000000..505248536c8 --- /dev/null +++ b/doc/user/profile/account/delete_account.md @@ -0,0 +1,25 @@ +# 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 + + diff --git a/spec/models/award_emoji_spec.rb b/spec/models/award_emoji_spec.rb index cb3c592f8cd..2a9a27752c1 100644 --- a/spec/models/award_emoji_spec.rb +++ b/spec/models/award_emoji_spec.rb @@ -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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3977af55176..6f7b9c2388a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -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 diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_service_spec.rb index 66c61b7f8ff..43c18992d1a 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -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 @@ -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 diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb new file mode 100644 index 00000000000..8c5b7e41c15 --- /dev/null +++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -0,0 +1,64 @@ +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 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 new file mode 100644 index 00000000000..0eac587e973 --- /dev/null +++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb @@ -0,0 +1,39 @@ +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 |