FIX: De-duplicate poll vote on user merge (#22107)

When merging users, polls may error out if the source and target users have both voted on the same poll before. 😢 

There is no constraint on the `poll_votes` table either to support this. Ideally a composite primary key can be used `(poll_id, user_id)`, but alas there is no support yet, which is probably why it wasn't created in the first place.

This fix ensures that merging is successful by only keeping the target poll votes if duplicates exist.

This fix also runs a migration on older poll votes where failed merges would have caused a single user to have voted twice on a single poll. e.g. this weird edge case
This commit is contained in:
Natalie Tay 2023-06-15 11:18:51 +08:00 committed by GitHub
parent ec31eb4c7b
commit fcaefc9f2f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 95 additions and 1 deletions

View File

@ -0,0 +1,18 @@
# frozen_string_literal: true
class DeleteDuplicatePollVotes < ActiveRecord::Migration[7.0]
def up
execute <<~SQL
DELETE FROM poll_votes
WHERE (poll_id, user_id, updated_at) NOT IN (
SELECT poll_id, user_id, MAX(updated_at)
FROM poll_votes
GROUP BY poll_id, user_id
)
SQL
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

View File

@ -169,7 +169,22 @@ after_initialize do
end
on(:merging_users) do |source_user, target_user|
PollVote.where(user_id: source_user.id).update_all(user_id: target_user.id)
DB.exec(<<-SQL, source_user_id: source_user.id, target_user_id: target_user.id)
DELETE FROM poll_votes
WHERE poll_id IN (
SELECT poll_id
FROM poll_votes
WHERE user_id = :source_user_id
INTERSECT
SELECT poll_id
FROM poll_votes
WHERE user_id = :target_user_id
) AND user_id = :source_user_id;
UPDATE poll_votes
SET user_id = :target_user_id
WHERE user_id = :source_user_id;
SQL
end
add_to_class(:topic_view, :polls) do

View File

@ -0,0 +1,18 @@
# frozen_string_literal: true
Fabricator(:poll) do
post
name { sequence(:name) { |i| "Poll #{i}" } }
end
Fabricator(:poll_option) do
poll
html { sequence(:html) { |i| "Poll Option #{i}" } }
digest { sequence(:digest) { |i| "#{i}" } }
end
Fabricator(:poll_vote) do
poll
poll_option
user
end

View File

@ -0,0 +1,43 @@
# frozen_string_literal: true
RSpec.describe UserMerger do
fab!(:target_user) { Fabricate(:user, username: "galahad", email: "galahad@knights.com") }
fab!(:source_user) { Fabricate(:user, username: "lancelot", email: "lancelot@knights.com") }
fab!(:poll) { Fabricate(:poll) }
it "deletes source_user vote if target_user already voted" do
Fabricate(:poll_vote, poll: poll, user: target_user)
Fabricate(:poll_vote, poll: poll, user: source_user)
DiscourseEvent.trigger(:merging_users, source_user, target_user)
expect(PollVote.where(user: source_user).count).to eq(0)
end
it "keeps only target_user vote if duplicate" do
target_vote = Fabricate(:poll_vote, poll: poll, user: target_user)
Fabricate(:poll_vote, poll: poll, user: source_user)
DiscourseEvent.trigger(:merging_users, source_user, target_user)
expect(PollVote.where(user: target_user).to_json).to eq([target_vote].to_json)
end
it "reassigns source_user vote to target_user if not duplicate" do
Fabricate(:poll_vote, poll: poll, user: source_user)
expect { DiscourseEvent.trigger(:merging_users, source_user, target_user) }.to change(
PollVote.where(user: target_user),
:count,
).from(0).to(1)
end
it "keeps any existing target_user votes" do
Fabricate(:poll_vote, poll: poll, user: target_user)
expect { DiscourseEvent.trigger(:merging_users, source_user, target_user) }.to not_change(
PollVote.where(user: target_user),
:count,
)
end
end