diff --git a/app/controllers/officing/voters_controller.rb b/app/controllers/officing/voters_controller.rb index f1c042770..f3394d431 100644 --- a/app/controllers/officing/voters_controller.rb +++ b/app/controllers/officing/voters_controller.rb @@ -14,18 +14,14 @@ class Officing::VotersController < Officing::BaseController @poll = Poll.find(voter_params[:poll_id]) @user = User.find(voter_params[:user_id]) - @user.with_lock do - @voter = Poll::Voter.new(document_type: @user.document_type, - document_number: @user.document_number, - user: @user, - poll: @poll, - origin: "booth", - officer: current_user.poll_officer, - booth_assignment: current_booth.booth_assignments.find_by(poll: @poll), - officer_assignment: officer_assignment(@poll)) - - @voter.save! - end + @voter = Poll::Voter.create_with( + document_type: @user.document_type, + document_number: @user.document_number, + origin: "booth", + officer: current_user.poll_officer, + booth_assignment: current_booth.booth_assignments.find_by(poll: @poll), + officer_assignment: officer_assignment(@poll) + ).find_or_create_by!(user: @user, poll: @poll) end private diff --git a/app/models/poll/voter.rb b/app/models/poll/voter.rb index ab6e3a0a6..5a00c6dd6 100644 --- a/app/models/poll/voter.rb +++ b/app/models/poll/voter.rb @@ -15,7 +15,6 @@ class Poll validates :officer_assignment_id, presence: true, if: ->(voter) { voter.origin == "booth" } validates :document_number, presence: true, unless: :skip_user_verification? - validates :user_id, uniqueness: { scope: [:poll_id], message: :has_voted } validates :origin, inclusion: { in: ->(*) { VALID_ORIGINS }} before_validation :set_demographic_info, :set_document_info, :set_denormalized_booth_assignment_id diff --git a/app/models/user.rb b/app/models/user.rb index 970cc8312..30cee72c1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -289,14 +289,10 @@ class User < ApplicationRecord def take_votes_from(other_user) return if other_user.blank? - 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 + Poll::Voter.where(user_id: other_user.id).find_each do |poll_voter| + transaction(requires_new: true) { poll_voter.update_column(:user_id, id) } + rescue ActiveRecord::RecordNotUnique + poll_voter.delete end Budget::Ballot.where(user_id: other_user.id).update_all(user_id: id) diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 47f5fe23a..951fa9168 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -576,8 +576,6 @@ en: attributes: document_number: not_in_census: "Document not in census" - user_id: - has_voted: "User has already voted" legislation/process: attributes: end_date: diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 3fe8f2540..8148cefbd 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -576,8 +576,6 @@ es: attributes: document_number: not_in_census: "Este documento no aparece en el censo" - user_id: - has_voted: "Este usuario ya ha votado" legislation/process: attributes: end_date: diff --git a/db/migrate/20250814122241_add_unique_index_to_poll_voters.rb b/db/migrate/20250814122241_add_unique_index_to_poll_voters.rb new file mode 100644 index 000000000..adacb4efe --- /dev/null +++ b/db/migrate/20250814122241_add_unique_index_to_poll_voters.rb @@ -0,0 +1,5 @@ +class AddUniqueIndexToPollVoters < ActiveRecord::Migration[7.1] + def change + add_index :poll_voters, [:user_id, :poll_id], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 4735d48d6..42c7e3ecd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2025_03_13_014205) do +ActiveRecord::Schema[7.1].define(version: 2025_08_14_122241) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" enable_extension "plpgsql" @@ -1209,6 +1209,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_03_13_014205) do t.index ["officer_assignment_id"], name: "index_poll_voters_on_officer_assignment_id" t.index ["poll_id", "document_number", "document_type"], name: "doc_by_poll" t.index ["poll_id"], name: "index_poll_voters_on_poll_id" + t.index ["user_id", "poll_id"], name: "index_poll_voters_on_user_id_and_poll_id", unique: true t.index ["user_id"], name: "index_poll_voters_on_user_id" end diff --git a/spec/controllers/officing/voters_controller_spec.rb b/spec/controllers/officing/voters_controller_spec.rb index c35ba974d..b869cac1f 100644 --- a/spec/controllers/officing/voters_controller_spec.rb +++ b/spec/controllers/officing/voters_controller_spec.rb @@ -2,20 +2,19 @@ require "rails_helper" describe Officing::VotersController do describe "POST create" do + let(:officer) { create(:poll_officer) } + before { sign_in(officer.user) } + 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 post :create, params: { voter: { poll_id: poll.id, user_id: user.id }, format: :js } - rescue ActiveRecord::RecordInvalid end end.each(&:join) @@ -24,7 +23,6 @@ describe Officing::VotersController do end it "stores officer and booth information" do - officer = create(:poll_officer) user = create(:user, :in_census) poll1 = create(:poll, name: "Would you be interested in XYZ?") poll2 = create(:poll, name: "Testing polls") @@ -37,7 +35,6 @@ describe Officing::VotersController do validate_officer set_officing_booth(booth) - sign_in(officer.user) post :create, params: { voter: { poll_id: poll1.id, user_id: user.id }, @@ -61,5 +58,20 @@ describe Officing::VotersController do expect(voter2.booth_assignment).to eq(assignment2) expect(voter2.officer_assignment).to eq(assignment2.officer_assignments.first) end + + it "does not overwrite non key attributes when a web voter already exists" do + user = create(:user, :level_two, document_number: "11223344Z") + poll = create(:poll, officers: [officer]) + existing = create(:poll_voter, poll: poll, user: user, origin: "web") + + expect do + post :create, params: { voter: { poll_id: poll.id, user_id: user.id }, format: :js } + expect(response).to be_successful + end.not_to change { Poll::Voter.count } + + existing.reload + expect(existing.origin).to eq "web" + expect(existing.document_number).to eq "11223344Z" + end end end diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index b3b2eb56b..abe5f6130 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -4,49 +4,6 @@ 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 - describe "polls:remove_duplicate_answers" do before { Rake::Task["polls:remove_duplicate_answers"].reenable } diff --git a/spec/models/poll/voter_spec.rb b/spec/models/poll/voter_spec.rb index e33711ff1..0ad4a053b 100644 --- a/spec/models/poll/voter_spec.rb +++ b/spec/models/poll/voter_spec.rb @@ -33,8 +33,7 @@ describe Poll::Voter do voter = build(:poll_voter, user: user, poll: poll) - expect(voter).not_to be_valid - expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) + expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique end it "is not valid if the user has already voted in the same poll/booth" do @@ -42,8 +41,7 @@ describe Poll::Voter do voter = build(:poll_voter, user: user, poll: poll, booth_assignment: booth_assignment) - expect(voter).not_to be_valid - expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) + expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique end it "is not valid if the user has already voted in different booth in the same poll" do @@ -51,8 +49,7 @@ describe Poll::Voter do voter = build(:poll_voter, :from_booth, user: user, poll: poll, booth: booth) - expect(voter).not_to be_valid - expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) + expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique end it "is valid if the user has already voted in the same booth in different poll" do @@ -68,8 +65,8 @@ describe Poll::Voter do create(:poll_voter, :from_web, user: answer.author, poll: answer.poll) voter = build(:poll_voter, poll: answer.question.poll, user: answer.author) - expect(voter).not_to be_valid - expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) + + expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique end context "Skip verification is enabled" do @@ -83,7 +80,7 @@ describe Poll::Voter do voter = build(:poll_voter, user: user, poll: poll) - expect(voter).not_to be_valid + expect { voter.save }.to raise_error ActiveRecord::RecordNotUnique end it "is valid if other users have voted in the same poll" do @@ -146,6 +143,15 @@ describe Poll::Voter do expect(voter).to be_valid end end + + describe ".create_or_find_by!" do + it "finds the voter when it already exists instead of failing the validation" do + existing_voter = Poll::Voter.create_with(origin: "web") + .create_or_find_by!(user: voter.user, poll: voter.poll) + + expect(existing_voter).to eq voter + end + end end describe "scopes" do diff --git a/spec/models/poll/web_vote_spec.rb b/spec/models/poll/web_vote_spec.rb index f150b7a12..aaa235597 100644 --- a/spec/models/poll/web_vote_spec.rb +++ b/spec/models/poll/web_vote_spec.rb @@ -73,6 +73,18 @@ describe Poll::WebVote do expect(question.reload.answers.size).to eq 0 end + it "raises an exception when the user has already voted in a booth" do + create(:poll_voter, :from_booth, poll: poll, user: user) + + expect do + web_vote.update(question.id.to_s => { option_id: option_yes.id.to_s }) + end.to raise_error(ActiveRecord::RecordNotFound) + + expect(poll.reload.voters.size).to eq 1 + expect(poll.voters.first.user).to eq user + expect(question.reload.answers.size).to eq 0 + end + context "creating answers at the same time", :race_condition do it "does not create two voters or two answers for two different answers" do [option_yes, option_no].map do |option|