From 3e1a1242c67781fb52940433c5ad1bbefd346216 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 6 Apr 2017 15:36:36 +0530 Subject: Move a user's award emoji to the ghost user ... when the user is destroyed. 1. Normally, for a given awardable and award emoji name, a user is only allowed to create a single award emoji. 2. This validation needs to be removed for ghost users, since: - User A and User B have created award emoji - with the same name and against the same awardable - User A is deleted. Their award emoji is moved to the ghost user - User B is deleted. Their award emoji needs to be moved to the ghost user. However, this breaks the uniqueness validation, since the ghost user is only allowed to have one award emoji of a given name for a given awardable --- app/models/award_emoji.rb | 3 ++- app/models/concerns/ghost_user.rb | 7 +++++ .../concerns/users/migrate_to_ghost_user.rb | 5 ++++ spec/models/award_emoji_spec.rb | 14 ++++++++++ spec/services/users/destroy_service_spec.rb | 30 +++++++++++++++++++++- ...ervice_migrate_to_ghost_user_shared_examples.rb | 6 ++++- 6 files changed, 62 insertions(+), 3 deletions(-) create mode 100644 app/models/concerns/ghost_user.rb 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/services/concerns/users/migrate_to_ghost_user.rb b/app/services/concerns/users/migrate_to_ghost_user.rb index 5d1f0ff57d1..76335e45e54 100644 --- a/app/services/concerns/users/migrate_to_ghost_user.rb +++ b/app/services/concerns/users/migrate_to_ghost_user.rb @@ -23,6 +23,7 @@ module Users::MigrateToGhostUser migrate_merge_requests(user) migrate_notes(user) migrate_abuse_reports(user) + migrate_award_emoji(user) end user.reload @@ -45,4 +46,8 @@ module Users::MigrateToGhostUser 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 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/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 028de62e4ca..d3463185ebe 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -166,7 +166,35 @@ describe Users::DestroyService, services: true do 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)) } - let(:author_method) { :reporter } + end + end + + 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 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 index add3dd3d5bc..3820ffc283f 100644 --- 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 @@ -23,7 +23,11 @@ shared_examples "migrating a deleted user's associated records to the ghost user migrated_record = record_class.find_by_id(record.id) - expect(migrated_record.author).to eq(User.ghost) + 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 -- cgit v1.2.1