From 1384ba5a4ef835932aae7f7882e5c2bb9c96c46f Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Tue, 27 Jun 2023 18:58:16 +0800 Subject: [PATCH] FIX: Cater for polls that can have multiple votes per user (#22297) Cater for polls that can have multiple votes per user. This fixes an older UserMerge and migration which was intended to de-duplicate poll votes but did not account for "multiple" type polls. --- ...30614041219_delete_duplicate_poll_votes.rb | 18 ++++-- plugins/poll/plugin.rb | 12 ++-- ...041219_delete_duplicate_poll_votes_spec.rb | 50 ++++++++++++++++ .../poll/spec/fabricators/poll_fabricator.rb | 4 ++ .../poll/spec/integration/user_merger_spec.rb | 57 +++++++++++++++---- 5 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 plugins/poll/spec/20230614041219_delete_duplicate_poll_votes_spec.rb diff --git a/plugins/poll/db/migrate/20230614041219_delete_duplicate_poll_votes.rb b/plugins/poll/db/migrate/20230614041219_delete_duplicate_poll_votes.rb index 4b1fb71a195..e65e1b0fb37 100644 --- a/plugins/poll/db/migrate/20230614041219_delete_duplicate_poll_votes.rb +++ b/plugins/poll/db/migrate/20230614041219_delete_duplicate_poll_votes.rb @@ -3,12 +3,20 @@ 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 + DELETE FROM poll_votes + WHERE (poll_id, user_id, poll_option_id) IN ( + SELECT pv.poll_id, pv.user_id, pv.poll_option_id + FROM poll_votes pv + JOIN polls p ON pv.poll_id = p.id + WHERE p.type = 0 + AND EXISTS ( + SELECT 1 + FROM poll_votes pv2 + WHERE pv.poll_id = pv2.poll_id + AND pv.user_id = pv2.user_id + AND pv.created_at < pv2.created_at ) + ); SQL end diff --git a/plugins/poll/plugin.rb b/plugins/poll/plugin.rb index 9efaa93f2ef..b656b7b4600 100644 --- a/plugins/poll/plugin.rb +++ b/plugins/poll/plugin.rb @@ -171,15 +171,13 @@ after_initialize do on(:merging_users) do |source_user, target_user| 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 + WHERE user_id = :source_user_id + AND EXISTS ( + SELECT 1 FROM poll_votes WHERE user_id = :target_user_id - ) AND user_id = :source_user_id; + AND poll_votes.poll_id = poll_votes.poll_id + ); UPDATE poll_votes SET user_id = :target_user_id diff --git a/plugins/poll/spec/20230614041219_delete_duplicate_poll_votes_spec.rb b/plugins/poll/spec/20230614041219_delete_duplicate_poll_votes_spec.rb new file mode 100644 index 00000000000..2435893dcf6 --- /dev/null +++ b/plugins/poll/spec/20230614041219_delete_duplicate_poll_votes_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require Rails.root.join("plugins/poll/db/migrate/20230614041219_delete_duplicate_poll_votes.rb") + +describe DeleteDuplicatePollVotes do + subject(:up) { described_class.new.up } + + fab!(:user) { Fabricate(:user, username: "galahad", email: "galahad@knights.com") } + + fab!(:poll_regular) { Fabricate(:poll_regular) } + fab!(:poll_regular_option1) { Fabricate(:poll_option, poll: poll_regular, html: "Option 1") } + fab!(:poll_regular_option2) { Fabricate(:poll_option, poll: poll_regular, html: "Option 2") } + + fab!(:poll_multiple) { Fabricate(:poll_multiple) } + fab!(:poll_multiple_optionA) { Fabricate(:poll_option, poll: poll_multiple, html: "Option A") } + fab!(:poll_multiple_optionB) { Fabricate(:poll_option, poll: poll_multiple, html: "Option B") } + + it "deletes the older duplicate poll vote for regular polls" do + Fabricate( + :poll_vote, + poll: poll_regular, + user: user, + poll_option: poll_regular_option1, + created_at: 2.years.ago, + ) + Fabricate( + :poll_vote, + poll: poll_regular, + user: user, + poll_option: poll_regular_option2, + created_at: 1.year.ago, + ) + + expect { up }.to change { PollVote.count }.from(2).to(1) + expect(PollVote.first.poll_option).to eq(poll_regular_option2) + end + + it "keeps non-duplicates" do + Fabricate(:poll_vote, poll: poll_regular, user: user, poll_option: poll_regular_option1) + + expect { up }.not_to change { PollVote.count } + end + + it "keeps votes on polls with type multiple" do + Fabricate(:poll_vote, poll: poll_multiple, user: user, poll_option: poll_multiple_optionA) + Fabricate(:poll_vote, poll: poll_multiple, user: user, poll_option: poll_multiple_optionB) + + expect { up }.not_to change { PollVote.count } + end +end diff --git a/plugins/poll/spec/fabricators/poll_fabricator.rb b/plugins/poll/spec/fabricators/poll_fabricator.rb index 33049dfada0..c78968d8a8d 100644 --- a/plugins/poll/spec/fabricators/poll_fabricator.rb +++ b/plugins/poll/spec/fabricators/poll_fabricator.rb @@ -5,6 +5,10 @@ Fabricator(:poll) do name { sequence(:name) { |i| "Poll #{i}" } } end +Fabricator(:poll_regular, from: :poll) { type "regular" } + +Fabricator(:poll_multiple, from: :poll) { type "multiple" } + Fabricator(:poll_option) do poll html { sequence(:html) { |i| "Poll Option #{i}" } } diff --git a/plugins/poll/spec/integration/user_merger_spec.rb b/plugins/poll/spec/integration/user_merger_spec.rb index 8c5f14f9ffd..d644bdcc39b 100644 --- a/plugins/poll/spec/integration/user_merger_spec.rb +++ b/plugins/poll/spec/integration/user_merger_spec.rb @@ -3,28 +3,63 @@ 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) + fab!(:poll_regular) { Fabricate(:poll) } + fab!(:poll_regular_option1) { Fabricate(:poll_option, poll: poll_regular, html: "Option 1") } + fab!(:poll_regular_option2) { Fabricate(:poll_option, poll: poll_regular, html: "Option 2") } + + fab!(:poll_multiple) { Fabricate(:poll) } + fab!(:poll_multiple_optionA) { Fabricate(:poll_option, poll: poll_multiple, html: "Option A") } + fab!(:poll_multiple_optionB) { Fabricate(:poll_option, poll: poll_multiple, html: "Option B") } + fab!(:poll_multiple_optionC) { Fabricate(:poll_option, poll: poll_multiple, html: "Option C") } + + it "will end up with no votes from source user" do + Fabricate(:poll_vote, poll: poll_regular, user: source_user, poll_option: poll_regular_option2) + Fabricate( + :poll_vote, + poll: poll_multiple, + user: source_user, + poll_option: poll_multiple_optionB, + ) 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) + it "will not use source user's vote if target_user already voted in the same poll" do + Fabricate(:poll_vote, poll: poll_regular, user: target_user, poll_option: poll_regular_option1) + Fabricate(:poll_vote, poll: poll_regular, user: source_user, poll_option: poll_regular_option2) + + Fabricate( + :poll_vote, + poll: poll_multiple, + user: target_user, + poll_option: poll_multiple_optionA, + ) + Fabricate( + :poll_vote, + poll: poll_multiple, + user: source_user, + poll_option: poll_multiple_optionB, + ) + Fabricate( + :poll_vote, + poll: poll_multiple, + user: source_user, + poll_option: poll_multiple_optionC, + ) DiscourseEvent.trigger(:merging_users, source_user, target_user) - expect(PollVote.where(user: target_user).to_json).to eq([target_vote].to_json) + expect(PollVote.where(user: target_user).pluck(:poll_option_id)).to contain_exactly( + poll_multiple_optionA.id, + poll_regular_option1.id, + ) end - it "reassigns source_user vote to target_user if not duplicate" do - Fabricate(:poll_vote, poll: poll, user: source_user) + it "reassigns source_user vote to target_user if target user has never voted in the poll" do + Fabricate(:poll_vote, poll: poll_regular, user: source_user) expect { DiscourseEvent.trigger(:merging_users, source_user, target_user) }.to change( PollVote.where(user: target_user), @@ -33,7 +68,7 @@ RSpec.describe UserMerger do end it "keeps any existing target_user votes" do - Fabricate(:poll_vote, poll: poll, user: target_user) + Fabricate(:poll_vote, poll: poll_regular, user: target_user) expect { DiscourseEvent.trigger(:merging_users, source_user, target_user) }.to not_change( PollVote.where(user: target_user),