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.

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
 

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

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

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}" } }

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),