From 9a8bfac5bd94df75d7b0b9fefbd860b590ab971f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 11 May 2024 18:13:35 +0200
Subject: [PATCH] Prevent creation of duplicate poll voters

Note that, when taking votes from an erased user, since poll answers
don't belong to poll voters, we were not migrating them in the
`take_votes_from` method (and we aren't migrating them now either).
---
 app/controllers/officing/voters_controller.rb |  3 ++-
 app/models/poll/answer.rb                     |  2 +-
 app/models/user.rb                            | 11 +++++++-
 .../officing/voters_controller_spec.rb        | 27 +++++++++++++++++++
 spec/models/poll/answer_spec.rb               | 10 +++++++
 spec/models/user_spec.rb                      | 18 +++++++++++++
 6 files changed, 68 insertions(+), 3 deletions(-)
 create mode 100644 spec/controllers/officing/voters_controller_spec.rb

diff --git a/app/controllers/officing/voters_controller.rb b/app/controllers/officing/voters_controller.rb
index da2210d13..de321b96b 100644
--- a/app/controllers/officing/voters_controller.rb
+++ b/app/controllers/officing/voters_controller.rb
@@ -21,7 +21,8 @@ class Officing::VotersController < Officing::BaseController
                              officer: current_user.poll_officer,
                              booth_assignment: current_booth.booth_assignments.find_by(poll: @poll),
                              officer_assignment: officer_assignment(@poll))
-    @voter.save!
+
+    @user.with_lock { @voter.save! }
   end
 
   private
diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb
index 932fe966b..039744fd5 100644
--- a/app/models/poll/answer.rb
+++ b/app/models/poll/answer.rb
@@ -16,7 +16,7 @@ class Poll::Answer < ApplicationRecord
   scope :by_question, ->(question_id) { where(question_id: question_id) }
 
   def save_and_record_voter_participation
-    transaction do
+    author.with_lock do
       save!
       Poll::Voter.find_or_create_by!(user: author, poll: poll, origin: "web")
     end
diff --git a/app/models/user.rb b/app/models/user.rb
index f9abd364c..dd24f20f6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -289,7 +289,16 @@ class User < ApplicationRecord
   def take_votes_from(other_user)
     return if other_user.blank?
 
-    Poll::Voter.where(user_id: other_user.id).update_all(user_id: id)
+    with_lock do
+      Poll::Voter.where(user_id: other_user.id).find_each do |poll_voter|
+        if Poll::Voter.where(poll: poll_voter.poll, user_id: id).any?
+          poll_voter.delete
+        else
+          poll_voter.update_column(:user_id, id)
+        end
+      end
+    end
+
     Budget::Ballot.where(user_id: other_user.id).update_all(user_id: id)
     Vote.where("voter_id = ? AND voter_type = ?", other_user.id, "User").update_all(voter_id: id)
     data_log = "id: #{other_user.id} - #{Time.current.strftime("%Y-%m-%d %H:%M:%S")}"
diff --git a/spec/controllers/officing/voters_controller_spec.rb b/spec/controllers/officing/voters_controller_spec.rb
new file mode 100644
index 000000000..81c9c6120
--- /dev/null
+++ b/spec/controllers/officing/voters_controller_spec.rb
@@ -0,0 +1,27 @@
+require "rails_helper"
+
+describe Officing::VotersController do
+  describe "POST create" do
+    it "does not create two records with two simultaneous requests", :race_condition do
+      officer = create(:poll_officer)
+      poll = create(:poll, officers: [officer])
+      user = create(:user, :level_two)
+
+      sign_in(officer.user)
+
+      2.times.map do
+        Thread.new do
+          begin
+            post :create, params: {
+              voter: { poll_id: poll.id, user_id: user.id },
+              format: :js
+            }
+          rescue ActionDispatch::IllegalStateError
+          end
+        end
+      end.each(&:join)
+
+      expect(Poll::Voter.count).to eq 1
+    end
+  end
+end
diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb
index 9c217851f..6e76ca44d 100644
--- a/spec/models/poll/answer_spec.rb
+++ b/spec/models/poll/answer_spec.rb
@@ -155,6 +155,16 @@ describe Poll::Answer do
 
       expect(Poll::Voter.count).to be 1
     end
+
+    it "does not create two voters when calling the method twice at the same time", :race_condition do
+      answer = create(:poll_answer, question: question, author: author, answer: "Yes")
+
+      2.times.map do
+        Thread.new { answer.save_and_record_voter_participation }
+      end.each(&:join)
+
+      expect(Poll::Voter.count).to be 1
+    end
   end
 
   describe "#destroy_and_remove_voter_participation" do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bd92a2292..b29c9a589 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -744,6 +744,24 @@ describe User do
       expect(Poll::Voter.where(user: other_user).count).to eq(0)
       expect(Poll::Voter.where(user: user)).to match_array [v1, v2]
     end
+
+    it "does not reassign votes if the user has already voted" do
+      poll = create(:poll)
+      user = create(:user, :level_three)
+      other_user = create(:user, :level_three)
+
+      voter = create(:poll_voter, poll: poll, user: user)
+      other_voter = create(:poll_voter, poll: poll, user: other_user)
+      other_poll_voter = create(:poll_voter, poll: create(:poll), user: other_user)
+
+      expect(Poll::Voter.where(user: user)).to eq [voter]
+      expect(Poll::Voter.where(user: other_user)).to match_array [other_voter, other_poll_voter]
+
+      user.take_votes_from(other_user)
+
+      expect(Poll::Voter.where(user: user)).to match_array [voter, other_poll_voter]
+      expect(Poll::Voter.where(user: other_user)).to eq []
+    end
   end
 
   describe "#take_votes_if_erased_document" do