Merge pull request #5532 from consuldemocracy/poll_duplicate_voters

Avoid creating duplicate voters in polls

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

app/lib/duplicate_records_logger.rb

@@ -0,0 +1,21 @@
+class DuplicateRecordsLogger
+  def info(message)
+    logger.info(message)
+  end
+
+  def logger
+    @logger ||= ActiveSupport::Logger.new(log_file).tap do |logger|
+      logger.formatter = Rails.application.config.log_formatter
+    end
+  end
+
+  private
+
+    def log_file
+      File.join(File.dirname(Rails.application.config.default_log_file), log_filename)
+    end
+
+    def log_filename
+      "duplicate_records.log"
+    end
+end

app/models/budget/ballot/line.rb

@@ -18,12 +18,12 @@ class Budget
       before_validation :set_denormalized_ids
 
       def check_enough_resources
-        ballot.lock!
+        ballot.with_lock do
-
           unless ballot.enough_resources?(investment)
             errors.add(:resources, ballot.not_enough_resources_error)
           end
         end
+      end
 
       def check_valid_heading
         return if ballot.valid_heading?(heading)

app/models/concerns/questionable.rb

@@ -1,30 +0,0 @@
-module Questionable
-  extend ActiveSupport::Concern
-
-  included do
-    has_one :votation_type, as: :questionable, dependent: :destroy
-    accepts_nested_attributes_for :votation_type
-    delegate :max_votes, :multiple?, :vote_type, to: :votation_type, allow_nil: true
-  end
-
-  def unique?
-    votation_type.nil? || votation_type.unique?
-  end
-
-  def find_or_initialize_user_answer(user, title)
-    answer = answers.find_or_initialize_by(find_by_attributes(user, title))
-    answer.answer = title
-    answer
-  end
-
-  private
-
-    def find_by_attributes(user, title)
-      case vote_type
-      when "unique", nil
-        { author: user }
-      when "multiple"
-        { author: user, answer: title }
-      end
-    end
-end

app/models/poll/answer.rb

@@ -16,8 +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
-      touch if persisted?
       save!
       Poll::Voter.find_or_create_by!(user: author, poll: poll, origin: "web")
     end
@@ -36,12 +35,12 @@ class Poll::Answer < ApplicationRecord
   private
 
     def max_votes
-      return if !question || question&.unique? || persisted?
+      return if !question || !author || persisted?
-
-      author.lock!
 
+      author.with_lock do
         if question.answers.by_author(author).count >= question.max_votes
           errors.add(:answer, "Maximum number of votes per user exceeded")
         end
       end
+    end
 end

app/models/poll/question.rb

@@ -1,7 +1,6 @@
 class Poll::Question < ApplicationRecord
   include Measurable
   include Searchable
-  include Questionable
 
   acts_as_paranoid column: :hidden_at
   include ActsAsParanoidAliases
@@ -19,6 +18,7 @@ class Poll::Question < ApplicationRecord
            inverse_of: :question,
            dependent: :destroy
   has_many :partial_results
+  has_one :votation_type, as: :questionable, dependent: :destroy
   belongs_to :proposal
 
   validates_translation :title, presence: true, length: { minimum: 4 }
@@ -26,6 +26,9 @@ class Poll::Question < ApplicationRecord
   validates :poll_id, presence: true, if: proc { |question| question.poll.nil? }
 
   accepts_nested_attributes_for :question_options, reject_if: :all_blank, allow_destroy: true
+  accepts_nested_attributes_for :votation_type
+
+  delegate :multiple?, :vote_type, to: :votation_type, allow_nil: true
 
   scope :by_poll_id,    ->(poll_id) { where(poll_id: poll_id) }
 
@@ -82,4 +85,33 @@ class Poll::Question < ApplicationRecord
   def options_with_read_more
     question_options.select(&:with_read_more?)
   end
+
+  def unique?
+    votation_type.nil? || votation_type.unique?
+  end
+
+  def max_votes
+    if multiple?
+      votation_type.max_votes
+    else
+      1
+    end
+  end
+
+  def find_or_initialize_user_answer(user, title)
+    answer = answers.find_or_initialize_by(find_by_attributes(user, title))
+    answer.answer = title
+    answer
+  end
+
+  private
+
+    def find_by_attributes(user, title)
+      case vote_type
+      when "unique", nil
+        { author: user }
+      when "multiple"
+        { author: user, answer: title }
+      end
+    end
 end

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

db/migrate/20240511141119_remove_answer_id_from_poll_voters.rb

@@ -0,0 +1,5 @@
+class RemoveAnswerIdFromPollVoters < ActiveRecord::Migration[7.0]
+  def change
+    remove_column :poll_voters, :answer_id, :integer
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_04_24_013913) do
+ActiveRecord::Schema[7.0].define(version: 2024_05_11_141119) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
   enable_extension "plpgsql"
@@ -1208,7 +1208,6 @@ ActiveRecord::Schema[7.0].define(version: 2024_04_24_013913) do
     t.integer "age"
     t.string "gender"
     t.integer "geozone_id"
-    t.integer "answer_id"
     t.integer "officer_assignment_id"
     t.integer "user_id"
     t.string "origin"

lib/tasks/consul.rake

@@ -7,6 +7,7 @@ namespace :consul do
 
   desc "Runs tasks needed to upgrade from 2.1.1 to 2.2.0"
   task "execute_release_2.2.0_tasks": [
-    "db:mask_ips"
+    "db:mask_ips",
+    "polls:remove_duplicate_voters"
   ]
 end

lib/tasks/polls.rake

@@ -0,0 +1,31 @@
+namespace :polls do
+  desc "Removes duplicate poll voters"
+  task remove_duplicate_voters: :environment do
+    logger = ApplicationLogger.new
+    duplicate_records_logger = DuplicateRecordsLogger.new
+
+    logger.info "Removing duplicate voters in polls"
+
+    Tenant.run_on_each do
+      duplicate_ids = Poll::Voter.select(:user_id, :poll_id)
+                                 .group(:user_id, :poll_id)
+                                 .having("count(*) > 1")
+                                 .pluck(:user_id, :poll_id)
+
+      duplicate_ids.each do |user_id, poll_id|
+        voters = Poll::Voter.where(user_id: user_id, poll_id: poll_id)
+        voters.excluding(voters.first).each do |voter|
+          voter.delete
+
+          tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default?
+          log_message = "Deleted duplicate record with ID #{voter.id} " \
+                        "from the #{Poll::Voter.table_name} table " \
+                        "with user_id #{user_id} " \
+                        "and poll_id #{poll_id}" + tenant_info.to_s
+          logger.info(log_message)
+          duplicate_records_logger.info(log_message)
+        end
+      end
+    end
+  end
+end

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

spec/lib/tasks/polls_spec.rb

@@ -0,0 +1,49 @@
+require "rails_helper"
+
+describe "polls tasks" do
+  let(:poll) { create(:poll) }
+  let(:user) { create(:user, :level_two) }
+
+  describe "polls:remove_duplicate_voters" do
+    before { Rake::Task["polls:remove_duplicate_voters"].reenable }
+
+    it "removes duplicate voters" do
+      second_user = create(:user, :level_two)
+
+      voter = create(:poll_voter, poll: poll, user: user)
+      second_voter = create(:poll_voter, poll: poll, user: second_user)
+      other_user_voter = create(:poll_voter, poll: poll, user: create(:user, :level_two))
+      other_poll_voter = create(:poll_voter, poll: create(:poll), user: user)
+
+      2.times { insert(:poll_voter, poll_id: poll.id, user_id: user.id) }
+      insert(:poll_voter, poll_id: poll.id, user_id: second_user.id)
+
+      expect(Poll::Voter.count).to eq 7
+
+      Rake.application.invoke_task("polls:remove_duplicate_voters")
+
+      expect(Poll::Voter.count).to eq 4
+      expect(Poll::Voter.all).to match_array [voter, second_voter, other_user_voter, other_poll_voter]
+    end
+
+    it "removes duplicate voters on tenants" do
+      create(:tenant, schema: "voters")
+
+      Tenant.switch("voters") do
+        poll = create(:poll)
+        user = create(:user, :level_two)
+
+        create(:poll_voter, poll: poll, user: user)
+        insert(:poll_voter, poll_id: poll.id, user_id: user.id)
+
+        expect(Poll::Voter.count).to eq 2
+      end
+
+      Rake.application.invoke_task("polls:remove_duplicate_voters")
+
+      Tenant.switch("voters") do
+        expect(Poll::Voter.count).to eq 1
+      end
+    end
+  end
+end

spec/models/poll/answer_spec.rb

@@ -18,11 +18,29 @@ describe Poll::Answer do
       expect(answer).not_to be_valid
     end
 
+    it "is not valid without an author when multiple answers are allowed" do
+      answer.author = nil
+      answer.question = create(:poll_question_multiple)
+
+      expect(answer).not_to be_valid
+    end
+
     it "is not valid without an answer" do
       answer.answer = nil
       expect(answer).not_to be_valid
     end
 
+    it "is not valid if there's already an answer to that question" do
+      author = create(:user)
+      question = create(:poll_question, :yes_no)
+
+      create(:poll_answer, author: author, question: question)
+
+      answer = build(:poll_answer, author: author, question: question)
+
+      expect(answer).not_to be_valid
+    end
+
     it "is not valid when user already reached multiple answers question max votes" do
       author = create(:user)
       question = create(:poll_question_multiple, :abc, max_votes: 2)
@@ -33,20 +51,6 @@ describe Poll::Answer do
       expect(answer).not_to be_valid
     end
 
-    it "validates max votes when creating answers at the same time", :race_condition do
-      author = create(:user, :level_two)
-      question = create(:poll_question_multiple, :abc, max_votes: 2)
-      create(:poll_answer, question: question, answer: "Answer A", author: author)
-      answer = build(:poll_answer, question: question, answer: "Answer B", author: author)
-      other_answer = build(:poll_answer, question: question, answer: "Answer C", author: author)
-
-      [answer, other_answer].map do |a|
-        Thread.new { a.save }
-      end.each(&:join)
-
-      expect(Poll::Answer.count).to be 2
-    end
-
     it "is valid for answers included in the Poll::Question's question_options list" do
       question = create(:poll_question)
       create(:poll_question_option, title: "One", question: question)
@@ -59,6 +63,36 @@ describe Poll::Answer do
 
       expect(build(:poll_answer, question: question, answer: "Four")).not_to be_valid
     end
+
+    context "creating answers at the same time", :race_condition do
+      it "validates max votes on single-answer questions" do
+        author = create(:user)
+        question = create(:poll_question, :yes_no)
+
+        answer = build(:poll_answer, author: author, question: question, answer: "Yes")
+        other_answer = build(:poll_answer, author: author, question: question, answer: "No")
+
+        [answer, other_answer].map do |poll_answer|
+          Thread.new { poll_answer.save }
+        end.each(&:join)
+
+        expect(Poll::Answer.count).to be 1
+      end
+
+      it "validates max votes on multiple-answer questions" do
+        author = create(:user, :level_two)
+        question = create(:poll_question_multiple, :abc, max_votes: 2)
+        create(:poll_answer, question: question, answer: "Answer A", author: author)
+        answer = build(:poll_answer, question: question, answer: "Answer B", author: author)
+        other_answer = build(:poll_answer, question: question, answer: "Answer C", author: author)
+
+        [answer, other_answer].map do |poll_answer|
+          Thread.new { poll_answer.save }
+        end.each(&:join)
+
+        expect(Poll::Answer.count).to be 2
+      end
+    end
   end
 
   describe "#save_and_record_voter_participation" do
@@ -85,14 +119,14 @@ describe Poll::Answer do
 
       expect(poll.reload.voters.size).to eq(1)
 
-      answer = create(:poll_answer, question: question, author: author, answer: "No")
+      updated_answer = answer.question.find_or_initialize_user_answer(answer.author, "No")
-      answer.save_and_record_voter_participation
+      updated_answer.save_and_record_voter_participation
 
       expect(poll.reload.voters.size).to eq(1)
 
       voter = poll.voters.first
-      expect(voter.document_number).to eq(answer.author.document_number)
+      expect(voter.document_number).to eq(updated_answer.author.document_number)
-      expect(voter.poll_id).to eq(answer.poll.id)
+      expect(voter.poll_id).to eq(updated_answer.poll.id)
     end
 
     it "does not save the answer if the voter is invalid" do
@@ -105,6 +139,32 @@ describe Poll::Answer do
 
       expect(answer).not_to be_persisted
     end
+
+    it "does not create two voters when creating two answers at the same time", :race_condition do
+      answer = build(:poll_answer, question: question, author: author, answer: "Yes")
+      other_answer = build(:poll_answer, question: question, author: author, answer: "No")
+
+      [answer, other_answer].map do |poll_answer|
+        Thread.new do
+          begin
+            poll_answer.save_and_record_voter_participation
+          rescue ActiveRecord::RecordInvalid
+          end
+        end
+      end.each(&:join)
+
+      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

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