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/lib/duplicate_records_logger.rb b/app/lib/duplicate_records_logger.rb new file mode 100644 index 000000000..6fda1a5d5 --- /dev/null +++ b/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 diff --git a/app/models/budget/ballot/line.rb b/app/models/budget/ballot/line.rb index 8b0849a19..0268ecc74 100644 --- a/app/models/budget/ballot/line.rb +++ b/app/models/budget/ballot/line.rb @@ -18,10 +18,10 @@ class Budget before_validation :set_denormalized_ids def check_enough_resources - ballot.lock! - - unless ballot.enough_resources?(investment) - errors.add(:resources, ballot.not_enough_resources_error) + ballot.with_lock do + unless ballot.enough_resources?(investment) + errors.add(:resources, ballot.not_enough_resources_error) + end end end diff --git a/app/models/concerns/questionable.rb b/app/models/concerns/questionable.rb deleted file mode 100644 index 1d0606fc6..000000000 --- a/app/models/concerns/questionable.rb +++ /dev/null @@ -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 diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index 41d76e925..039744fd5 100644 --- a/app/models/poll/answer.rb +++ b/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 - touch if persisted? + author.with_lock do 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! - - if question.answers.by_author(author).count >= question.max_votes - errors.add(:answer, "Maximum number of votes per user exceeded") + 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 diff --git a/app/models/poll/question.rb b/app/models/poll/question.rb index b860195c5..729cdbe46 100644 --- a/app/models/poll/question.rb +++ b/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 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/db/migrate/20240511141119_remove_answer_id_from_poll_voters.rb b/db/migrate/20240511141119_remove_answer_id_from_poll_voters.rb new file mode 100644 index 000000000..0e62d618c --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index d330136cc..0dfa54bd7 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.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" diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 73296660a..9f56f3c38 100644 --- a/lib/tasks/consul.rake +++ b/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 diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake new file mode 100644 index 000000000..b6040a127 --- /dev/null +++ b/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 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/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb new file mode 100644 index 000000000..2b43c969d --- /dev/null +++ b/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 diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index fd77d5666..6e76ca44d 100644 --- a/spec/models/poll/answer_spec.rb +++ b/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") - answer.save_and_record_voter_participation + updated_answer = answer.question.find_or_initialize_user_answer(answer.author, "No") + 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.poll_id).to eq(answer.poll.id) + expect(voter.document_number).to eq(updated_answer.author.document_number) + 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 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