From 0c650c423dc649d1c86587300b403e8bf39e88c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 10 May 2024 00:56:05 +0200 Subject: [PATCH 1/9] Fix exception creating an answer without an author We were getting `undefined method `lock!' for nil:NilClass` when the question allowed multiple answers. --- app/models/poll/answer.rb | 2 +- spec/models/poll/answer_spec.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index 41d76e925..ad1827e59 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -36,7 +36,7 @@ class Poll::Answer < ApplicationRecord private def max_votes - return if !question || question&.unique? || persisted? + return if !question || !author || question&.unique? || persisted? author.lock! diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index fd77d5666..dc3dee8f5 100644 --- a/spec/models/poll/answer_spec.rb +++ b/spec/models/poll/answer_spec.rb @@ -18,6 +18,13 @@ 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 From 6aafc107aea2d38b2fd8ffc91cf0f5430b7e4d08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 14 May 2024 02:05:03 +0200 Subject: [PATCH 2/9] Remove Questionable concern Since we were only using it in one place, it made the code harder to follow. We'll extract it again if we ever find a way to reuse it. --- app/models/concerns/questionable.rb | 30 ----------------------------- app/models/poll/question.rb | 26 ++++++++++++++++++++++++- 2 files changed, 25 insertions(+), 31 deletions(-) delete mode 100644 app/models/concerns/questionable.rb 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/question.rb b/app/models/poll/question.rb index b860195c5..813b7882e 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 :max_votes, :multiple?, :vote_type, to: :votation_type, allow_nil: true scope :by_poll_id, ->(poll_id) { where(poll_id: poll_id) } @@ -82,4 +85,25 @@ 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 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 From a54d424aed328ae3cd4abdf4ee45b26786c316e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 10 May 2024 01:44:23 +0200 Subject: [PATCH 3/9] Add missing validation rule to poll answers We were checking we didn't have more votes than allowed in the case of questions with multiple answers, but we weren't checking it in the case of questions with a single answer. This made it possible to create more than one answer to the same question. This could happen because the method `find_or_initialize_user_answer` might initialize two answers in different threads, due to a race condition. --- app/models/poll/answer.rb | 2 +- app/models/poll/question.rb | 10 ++++- spec/models/poll/answer_spec.rb | 79 +++++++++++++++++++++++++-------- 3 files changed, 71 insertions(+), 20 deletions(-) diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index ad1827e59..b99ffcef6 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -36,7 +36,7 @@ class Poll::Answer < ApplicationRecord private def max_votes - return if !question || !author || question&.unique? || persisted? + return if !question || !author || persisted? author.lock! diff --git a/app/models/poll/question.rb b/app/models/poll/question.rb index 813b7882e..729cdbe46 100644 --- a/app/models/poll/question.rb +++ b/app/models/poll/question.rb @@ -28,7 +28,7 @@ class Poll::Question < ApplicationRecord accepts_nested_attributes_for :question_options, reject_if: :all_blank, allow_destroy: true accepts_nested_attributes_for :votation_type - delegate :max_votes, :multiple?, :vote_type, to: :votation_type, allow_nil: true + delegate :multiple?, :vote_type, to: :votation_type, allow_nil: true scope :by_poll_id, ->(poll_id) { where(poll_id: poll_id) } @@ -90,6 +90,14 @@ class Poll::Question < ApplicationRecord 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 diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index dc3dee8f5..9c217851f 100644 --- a/spec/models/poll/answer_spec.rb +++ b/spec/models/poll/answer_spec.rb @@ -30,6 +30,17 @@ describe Poll::Answer do 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) @@ -40,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) @@ -66,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 @@ -92,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 @@ -112,6 +139,22 @@ 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 end describe "#destroy_and_remove_voter_participation" do From fb9156f9b8d59da2e55f29f1671d144eed11e4ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 10 May 2024 02:00:37 +0200 Subject: [PATCH 4/9] Use with_lock instead of lock! That way the record is only locked while necessary. --- app/models/budget/ballot/line.rb | 8 ++++---- app/models/poll/answer.rb | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) 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/poll/answer.rb b/app/models/poll/answer.rb index b99ffcef6..e3fbef6f7 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -38,10 +38,10 @@ class Poll::Answer < ApplicationRecord def max_votes 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 From 5f12db899f66ff345ad5cf0e575e1af1c857f38f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 11 May 2024 04:02:30 +0200 Subject: [PATCH 5/9] Remove no longer needed call to Poll::Answer#touch This call was added in commit 81f65f1ac, and the test for its need was added in commit cb1542874. However, both the test and the helper method relying on the `touch` call were removed in commit f90d0d9c4. --- app/models/poll/answer.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index e3fbef6f7..932fe966b 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -17,7 +17,6 @@ class Poll::Answer < ApplicationRecord def save_and_record_voter_participation transaction do - touch if persisted? save! Poll::Voter.find_or_create_by!(user: author, poll: poll, origin: "web") end From 175e990bb4c630b026aaf547c16dec2313753dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 11 May 2024 15:14:39 +0200 Subject: [PATCH 6/9] Remove unused answer_id field in poll_voters table This field isn't used since commit 51be80eed, right after being added in commit 5806d86e3. --- .../20240511141119_remove_answer_id_from_poll_voters.rb | 5 +++++ db/schema.rb | 3 +-- 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20240511141119_remove_answer_id_from_poll_voters.rb 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" From 9a8bfac5bd94df75d7b0b9fefbd860b590ab971f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 11 May 2024 18:13:35 +0200 Subject: [PATCH 7/9] 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 From b013a5b1b61121d88c8ec1c35fae41454dcfbfb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 11 May 2024 18:49:11 +0200 Subject: [PATCH 8/9] Add task to delete duplicate voters Note that, since poll answers belong to a user and not to a voter, we aren't doing anything regarding poll answers. This is a separate topic that might be dealt with in a separate pull request. Also note that, since there are no records belonging to poll voters, and poll voters don't use `acts_as_paranoia` and don't have any callbacks on destroy, it doesn't really matter whether we call `destroy!` or `delete`. We're using `delete` so there are no unintended side-effects that might affect voters with the same `user_id` and `poll_id` on Consul Democracy installations customizing this behavior. --- lib/tasks/consul.rake | 3 ++- lib/tasks/polls.rake | 27 ++++++++++++++++++++ spec/lib/tasks/polls_spec.rb | 49 ++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 lib/tasks/polls.rake create mode 100644 spec/lib/tasks/polls_spec.rb 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..689ecc335 --- /dev/null +++ b/lib/tasks/polls.rake @@ -0,0 +1,27 @@ +namespace :polls do + desc "Removes duplicate poll voters" + task remove_duplicate_voters: :environment do + logger = ApplicationLogger.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? + logger.info "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 + end + end + 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 From b327275d186282b4e184e3516a2d42eaf5628f60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 11 May 2024 18:58:43 +0200 Subject: [PATCH 9/9] Add a log file to track deleted duplicate records It might be interesting in some cases to check the information related to those records. --- app/lib/duplicate_records_logger.rb | 21 +++++++++++++++++++++ lib/tasks/polls.rake | 12 ++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 app/lib/duplicate_records_logger.rb 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/lib/tasks/polls.rake b/lib/tasks/polls.rake index 689ecc335..b6040a127 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -2,6 +2,8 @@ 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 @@ -16,10 +18,12 @@ namespace :polls do voter.delete tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? - logger.info "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 + 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