From 9fbd7eec8fa4aa11ae5f8502519747a62cb615b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 14 May 2024 00:59:01 +0200 Subject: [PATCH 1/8] Remove obsolete routes for poll questions The routes for poll questions were accidentally deleted in commit 5bb831e9595 when deleting the `:show` action, and restored in commit 9871503c5e8. However, the deleted code was: ``` resources :questions, only: [:show], controller: 'polls/questions' (...) ``` While the restored code was: ``` resources :questions, controller: 'polls/questions' (...) ``` Meaning we forgot to add the `only: []` option when restoring the routes. We also forgot to remove the `before_action` code when deleting the `:show` action, so we're removing it now. --- app/controllers/polls/questions_controller.rb | 2 -- config/routes/poll.rb | 2 +- spec/routing/polymorphic_routes_spec.rb | 6 ------ 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/app/controllers/polls/questions_controller.rb b/app/controllers/polls/questions_controller.rb index 0e7f9a1c9..3db380663 100644 --- a/app/controllers/polls/questions_controller.rb +++ b/app/controllers/polls/questions_controller.rb @@ -2,8 +2,6 @@ class Polls::QuestionsController < ApplicationController load_and_authorize_resource :poll load_and_authorize_resource :question, class: "Poll::Question" - has_orders %w[most_voted newest oldest], only: :show - def answer answer = @question.find_or_initialize_user_answer(current_user, params[:answer]) answer.save_and_record_voter_participation diff --git a/config/routes/poll.rb b/config/routes/poll.rb index 94cf504b4..d33e75923 100644 --- a/config/routes/poll.rb +++ b/config/routes/poll.rb @@ -4,7 +4,7 @@ resources :polls, only: [:show, :index] do get :results end - resources :questions, controller: "polls/questions", shallow: true do + resources :questions, controller: "polls/questions", shallow: true, only: [] do post :answer, on: :member resources :answers, controller: "polls/answers", only: :destroy, shallow: false end diff --git a/spec/routing/polymorphic_routes_spec.rb b/spec/routing/polymorphic_routes_spec.rb index c39b986d5..cf2e748dd 100644 --- a/spec/routing/polymorphic_routes_spec.rb +++ b/spec/routing/polymorphic_routes_spec.rb @@ -53,12 +53,6 @@ describe "Polymorphic routes" do ) end - it "routes poll questions" do - question = create(:poll_question) - - expect(polymorphic_path(question)).to eq question_path(question) - end - it "routes topics" do community = create(:proposal).community topic = create(:topic, community: community) From 9a840bb8d166c5c0381222489ebf3a39262c7352 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 14 May 2024 01:40:17 +0200 Subject: [PATCH 2/8] Remove unused code in poll questions controller This code wasn't used since commit d9ad65875. --- app/controllers/polls/questions_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/polls/questions_controller.rb b/app/controllers/polls/questions_controller.rb index 3db380663..07810bbfc 100644 --- a/app/controllers/polls/questions_controller.rb +++ b/app/controllers/polls/questions_controller.rb @@ -1,5 +1,4 @@ class Polls::QuestionsController < ApplicationController - load_and_authorize_resource :poll load_and_authorize_resource :question, class: "Poll::Question" def answer From 03f89c9ca2b6b6df4f2482d4999a279641a4adb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 14 May 2024 01:49:56 +0200 Subject: [PATCH 3/8] Move action to create answers to AnswersController It was confusing to have the action to create an answer in `QuestionsController#answer` while the action to destroy it was `AnswersController#destroy`. --- .../questions/options_component.html.erb | 2 +- app/controllers/polls/answers_controller.rb | 21 +++++++++++++++++-- app/controllers/polls/questions_controller.rb | 17 --------------- .../options.js.erb => answers/show.js.erb} | 0 config/routes/poll.rb | 3 +-- 5 files changed, 21 insertions(+), 22 deletions(-) delete mode 100644 app/controllers/polls/questions_controller.rb rename app/views/polls/{questions/options.js.erb => answers/show.js.erb} (100%) diff --git a/app/components/polls/questions/options_component.html.erb b/app/components/polls/questions/options_component.html.erb index b687ed249..ae5d6f2a6 100644 --- a/app/components/polls/questions/options_component.html.erb +++ b/app/components/polls/questions/options_component.html.erb @@ -11,7 +11,7 @@ "aria-pressed": true %> <% else %> <%= button_to question_option.title, - answer_question_path(question, answer: question_option.title), + question_answers_path(question, answer: question_option.title), remote: true, title: t("poll_questions.show.vote_answer", answer: question_option.title), class: "button secondary hollow", diff --git a/app/controllers/polls/answers_controller.rb b/app/controllers/polls/answers_controller.rb index 174cc673c..fc75620ec 100644 --- a/app/controllers/polls/answers_controller.rb +++ b/app/controllers/polls/answers_controller.rb @@ -2,7 +2,24 @@ class Polls::AnswersController < ApplicationController load_and_authorize_resource :question, class: "::Poll::Question" load_and_authorize_resource :answer, class: "::Poll::Answer", through: :question, - through_association: :answers + through_association: :answers, + only: :destroy + + def create + authorize! :answer, @question + + answer = @question.find_or_initialize_user_answer(current_user, params[:answer]) + answer.save_and_record_voter_participation + + respond_to do |format| + format.html do + redirect_to request.referer + end + format.js do + render :show + end + end + end def destroy @answer.destroy_and_remove_voter_participation @@ -12,7 +29,7 @@ class Polls::AnswersController < ApplicationController redirect_to request.referer end format.js do - render "polls/questions/options" + render :show end end end diff --git a/app/controllers/polls/questions_controller.rb b/app/controllers/polls/questions_controller.rb deleted file mode 100644 index 07810bbfc..000000000 --- a/app/controllers/polls/questions_controller.rb +++ /dev/null @@ -1,17 +0,0 @@ -class Polls::QuestionsController < ApplicationController - load_and_authorize_resource :question, class: "Poll::Question" - - def answer - answer = @question.find_or_initialize_user_answer(current_user, params[:answer]) - answer.save_and_record_voter_participation - - respond_to do |format| - format.html do - redirect_to request.referer - end - format.js do - render :options - end - end - end -end diff --git a/app/views/polls/questions/options.js.erb b/app/views/polls/answers/show.js.erb similarity index 100% rename from app/views/polls/questions/options.js.erb rename to app/views/polls/answers/show.js.erb diff --git a/config/routes/poll.rb b/config/routes/poll.rb index d33e75923..f5be6b301 100644 --- a/config/routes/poll.rb +++ b/config/routes/poll.rb @@ -5,8 +5,7 @@ resources :polls, only: [:show, :index] do end resources :questions, controller: "polls/questions", shallow: true, only: [] do - post :answer, on: :member - resources :answers, controller: "polls/answers", only: :destroy, shallow: false + resources :answers, controller: "polls/answers", only: [:create, :destroy], shallow: false end end From 503369166683d0ccd6c99d83e46234a7c8ce5498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 14 May 2024 02:09:09 +0200 Subject: [PATCH 4/8] Avoid duplicate records in poll answers Until now, we've stored the text of the answer somebody replied to. The idea was to handle the scenarios where the user voters for an option but then that option is deleted and restored, or the texts of the options are accidentally edited and so the option "Yes" is now "Now" and vice versa. However, since commit 3a6e99cb8, options can no longer be edited once the poll starts, so there's no risk of the option changing once somebody has voted. This means we can now store the ID of the option that has been voted. That'll also help us deal with a bug introduced int 673ec075e, since answers in different locales are not counted as the same answer. Note we aren't dealing with this bug right now. We're still keeping (and storing) the answer as well. There are two reasons for that. First, we might add an "open answer" type of questions in the future and use this column for it. Second, we've still got logic depending on the answer, and we need to be careful when changing it because there are existing installations where the answer is present but the option_id is not. Note that we're using `dependent: nullify`. The reasoning is that, since we're storing both the option_id and the answer text, we can still use the answer text when removing the option. In practice, this won't matter much, though, since we've got a validation rule that makes it impossible to destroy options once the poll has started. Also note we're still allowing duplicate records when the option is nil. We need to do that until we've removed every duplicate record in the database. --- .../questions/options_component.html.erb | 2 +- app/controllers/polls/answers_controller.rb | 2 +- app/models/poll/answer.rb | 4 +- app/models/poll/question.rb | 13 +++--- app/models/poll/question/option.rb | 1 + ..._add_question_answer_id_to_poll_answers.rb | 9 ++++ db/schema.rb | 6 ++- .../polls/answers_controller_spec.rb | 25 +++++++++++ spec/factories/polls.rb | 1 + spec/models/poll/answer_spec.rb | 43 ++++++++++++++++++- 10 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb create mode 100644 spec/controllers/polls/answers_controller_spec.rb diff --git a/app/components/polls/questions/options_component.html.erb b/app/components/polls/questions/options_component.html.erb index ae5d6f2a6..a183b2aac 100644 --- a/app/components/polls/questions/options_component.html.erb +++ b/app/components/polls/questions/options_component.html.erb @@ -11,7 +11,7 @@ "aria-pressed": true %> <% else %> <%= button_to question_option.title, - question_answers_path(question, answer: question_option.title), + question_answers_path(question, option_id: question_option.id), remote: true, title: t("poll_questions.show.vote_answer", answer: question_option.title), class: "button secondary hollow", diff --git a/app/controllers/polls/answers_controller.rb b/app/controllers/polls/answers_controller.rb index fc75620ec..a969c5edf 100644 --- a/app/controllers/polls/answers_controller.rb +++ b/app/controllers/polls/answers_controller.rb @@ -8,7 +8,7 @@ class Polls::AnswersController < ApplicationController def create authorize! :answer, @question - answer = @question.find_or_initialize_user_answer(current_user, params[:answer]) + answer = @question.find_or_initialize_user_answer(current_user, params[:option_id]) answer.save_and_record_voter_participation respond_to do |format| diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index 039744fd5..b9c52fb38 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -1,12 +1,14 @@ class Poll::Answer < ApplicationRecord belongs_to :question, -> { with_hidden }, inverse_of: :answers - belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :poll_answers + belongs_to :option, class_name: "Poll::Question::Option" + belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :poll_answers delegate :poll, :poll_id, to: :question validates :question, presence: true validates :author, presence: true validates :answer, presence: true + validates :option, uniqueness: { scope: :author_id }, allow_nil: true validate :max_votes validates :answer, inclusion: { in: ->(a) { a.question.possible_answers }}, diff --git a/app/models/poll/question.rb b/app/models/poll/question.rb index 729cdbe46..c55320560 100644 --- a/app/models/poll/question.rb +++ b/app/models/poll/question.rb @@ -98,20 +98,23 @@ class Poll::Question < ApplicationRecord end end - def find_or_initialize_user_answer(user, title) - answer = answers.find_or_initialize_by(find_by_attributes(user, title)) - answer.answer = title + def find_or_initialize_user_answer(user, option_id) + option = question_options.find(option_id) + + answer = answers.find_or_initialize_by(find_by_attributes(user, option)) + answer.option = option + answer.answer = option.title answer end private - def find_by_attributes(user, title) + def find_by_attributes(user, option) case vote_type when "unique", nil { author: user } when "multiple" - { author: user, answer: title } + { author: user, answer: option.title } end end end diff --git a/app/models/poll/question/option.rb b/app/models/poll/question/option.rb index 59e38aa33..c191c5923 100644 --- a/app/models/poll/question/option.rb +++ b/app/models/poll/question/option.rb @@ -11,6 +11,7 @@ class Poll::Question::Option < ApplicationRecord accepts_nested_attributes_for :documents, allow_destroy: true belongs_to :question, class_name: "Poll::Question" + has_many :answers, class_name: "Poll::Answer", dependent: :nullify has_many :videos, class_name: "Poll::Question::Option::Video", dependent: :destroy, foreign_key: "answer_id", diff --git a/db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb b/db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb new file mode 100644 index 000000000..d96d9027e --- /dev/null +++ b/db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb @@ -0,0 +1,9 @@ +class AddQuestionAnswerIdToPollAnswers < ActiveRecord::Migration[7.0] + def change + change_table :poll_answers do |t| + t.references :option, index: true, foreign_key: { to_table: :poll_question_answers } + + t.index [:option_id, :author_id], unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 0dfa54bd7..d565cb35c 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_05_11_141119) do +ActiveRecord::Schema[7.0].define(version: 2024_05_13_135357) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" enable_extension "plpgsql" @@ -1024,7 +1024,10 @@ ActiveRecord::Schema[7.0].define(version: 2024_05_11_141119) do t.string "answer" t.datetime "created_at", precision: nil t.datetime "updated_at", precision: nil + t.bigint "option_id" t.index ["author_id"], name: "index_poll_answers_on_author_id" + t.index ["option_id", "author_id"], name: "index_poll_answers_on_option_id_and_author_id", unique: true + t.index ["option_id"], name: "index_poll_answers_on_option_id" t.index ["question_id", "answer"], name: "index_poll_answers_on_question_id_and_answer" t.index ["question_id"], name: "index_poll_answers_on_question_id" end @@ -1798,6 +1801,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_05_11_141119) do add_foreign_key "moderators", "users" add_foreign_key "notifications", "users" add_foreign_key "organizations", "users" + add_foreign_key "poll_answers", "poll_question_answers", column: "option_id" add_foreign_key "poll_answers", "poll_questions", column: "question_id" add_foreign_key "poll_booth_assignments", "polls" add_foreign_key "poll_officer_assignments", "poll_booth_assignments", column: "booth_assignment_id" diff --git a/spec/controllers/polls/answers_controller_spec.rb b/spec/controllers/polls/answers_controller_spec.rb new file mode 100644 index 000000000..08f129c0b --- /dev/null +++ b/spec/controllers/polls/answers_controller_spec.rb @@ -0,0 +1,25 @@ +require "rails_helper" + +describe Polls::AnswersController do + describe "POST create" do + it "doesn't create duplicate records on simultaneous requests", :race_condition do + question = create(:poll_question_multiple, :abc) + sign_in(create(:user, :level_two)) + + 2.times.map do + Thread.new do + begin + post :create, params: { + question_id: question.id, + option_id: question.question_options.find_by(title: "Answer A").id, + format: :js + } + rescue ActionDispatch::IllegalStateError, ActiveRecord::RecordInvalid + end + end + end.each(&:join) + + expect(Poll::Answer.count).to eq 1 + end + end +end diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 4f61232bc..0d7811784 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -205,6 +205,7 @@ FactoryBot.define do question factory: [:poll_question, :yes_no] author factory: [:user, :level_two] answer { question.question_options.sample.title } + option { question.question_options.find_by(title: answer) } end factory :poll_partial_result, class: "Poll::PartialResult" do diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index 6e76ca44d..b06ccbcd7 100644 --- a/spec/models/poll/answer_spec.rb +++ b/spec/models/poll/answer_spec.rb @@ -51,6 +51,44 @@ describe Poll::Answer do expect(answer).not_to be_valid end + it "is not valid when there are two identical answers" do + author = create(:user) + question = create(:poll_question_multiple, :abc) + option = question.question_options.first + + create(:poll_answer, author: author, question: question, option: option, answer: "Answer A") + + answer = build(:poll_answer, author: author, question: question, option: option, answer: "Answer A") + + expect(answer).not_to be_valid + expect { answer.save(validate: false) }.to raise_error ActiveRecord::RecordNotUnique + end + + it "is not valid when there are two answers with the same option and different answer" do + author = create(:user) + question = create(:poll_question_multiple, :abc) + option = question.question_options.first + + create(:poll_answer, author: author, question: question, option: option, answer: "Answer A") + + answer = build(:poll_answer, author: author, question: question, option: option, answer: "Answer B") + + expect(answer).not_to be_valid + expect { answer.save(validate: false) }.to raise_error ActiveRecord::RecordNotUnique + end + + it "is valid when there are two identical answers and the option is nil" do + author = create(:user) + question = create(:poll_question_multiple, :abc) + + create(:poll_answer, author: author, question: question, option: nil, answer: "Answer A") + + answer = build(:poll_answer, author: author, question: question, option: nil, answer: "Answer A") + + expect(answer).to be_valid + expect { answer.save }.not_to raise_error + 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) @@ -119,7 +157,10 @@ describe Poll::Answer do expect(poll.reload.voters.size).to eq(1) - updated_answer = answer.question.find_or_initialize_user_answer(answer.author, "No") + updated_answer = answer.question.find_or_initialize_user_answer( + answer.author, + answer.question.question_options.excluding(answer.option).sample.id + ) updated_answer.save_and_record_voter_participation expect(poll.reload.voters.size).to eq(1) From 81abbd5021136cee0a7d02bfd1f39de744a19ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 12 Jun 2024 17:02:13 +0200 Subject: [PATCH 5/8] Remove unused block variable in poll ABC factory --- spec/factories/polls.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 0d7811784..4e07dc5f5 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -66,7 +66,7 @@ FactoryBot.define do end trait :abc do - after(:create) do |question, evaluator| + after(:create) do |question| %w[A B C].each do |letter| create(:poll_question_option, question: question, title: "Answer #{letter}") end From d2ec73e92c0496dcb77970d91b5084dc7236a367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 15 May 2024 16:46:05 +0200 Subject: [PATCH 6/8] Add task to delete duplicate poll answers --- lib/tasks/consul.rake | 3 +- lib/tasks/polls.rake | 33 ++++++++++++++++ spec/factories/polls.rb | 8 ++++ spec/lib/tasks/polls_spec.rb | 74 ++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 1 deletion(-) diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 9f56f3c38..0e89a360f 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -8,6 +8,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", - "polls:remove_duplicate_voters" + "polls:remove_duplicate_voters", + "polls:remove_duplicate_answers" ] end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index b6040a127..bda3058eb 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -28,4 +28,37 @@ namespace :polls do end end end + + desc "Removes duplicate poll answers" + task remove_duplicate_answers: :environment do + logger = ApplicationLogger.new + duplicate_records_logger = DuplicateRecordsLogger.new + + logger.info "Removing duplicate answers in polls" + + Tenant.run_on_each do + duplicate_ids = Poll::Answer.where(option_id: nil) + .select(:question_id, :author_id, :answer) + .group(:question_id, :author_id, :answer) + .having("count(*) > 1") + .pluck(:question_id, :author_id, :answer) + + duplicate_ids.each do |question_id, author_id, answer| + poll_answers = Poll::Answer.where(question_id: question_id, author_id: author_id, answer: answer) + + poll_answers.excluding(poll_answers.first).each do |poll_answer| + poll_answer.delete + + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + log_message = "Deleted duplicate record with ID #{poll_answer.id} " \ + "from the #{Poll::Answer.table_name} table " \ + "with question_id #{question_id}, " \ + "author_id #{author_id} " \ + "and answer #{answer}" + tenant_info.to_s + logger.info(log_message) + duplicate_records_logger.info(log_message) + end + end + end + end end diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 4e07dc5f5..735c20cb8 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -73,6 +73,14 @@ FactoryBot.define do end end + trait :abcde do + after(:create) do |question| + %w[A B C D E].each do |letter| + create(:poll_question_option, question: question, title: "Answer #{letter}") + end + end + end + factory :poll_question_unique do after(:create) do |question| create(:votation_type_unique, questionable: question) diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index 2b43c969d..da5050564 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -46,4 +46,78 @@ describe "polls tasks" do end end end + + describe "polls:remove_duplicate_answers" do + before { Rake::Task["polls:remove_duplicate_answers"].reenable } + + it "removes duplicate answers" do + question = create(:poll_question_multiple, :abcde, poll: poll, max_votes: 4) + abc_question = create(:poll_question_multiple, :abc, poll: poll) + + answer_attributes = { + question_id: question.id, + author_id: user.id, + answer: "Answer A", + option_id: nil + } + abc_answer_attributes = answer_attributes.merge(question_id: abc_question.id, answer: "Answer B") + + answer = create(:poll_answer, answer_attributes) + other_answer = create(:poll_answer, answer_attributes.merge(answer: "Answer B")) + other_user_answer = create(:poll_answer, answer_attributes.merge(author_id: create(:user).id)) + abc_answer = create(:poll_answer, abc_answer_attributes) + + 2.times { insert(:poll_answer, answer_attributes) } + insert(:poll_answer, abc_answer_attributes) + + expect(Poll::Answer.count).to eq 7 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 4 + expect(Poll::Answer.all).to match_array [answer, other_answer, other_user_answer, abc_answer] + end + + it "does not remove answers with the same text and different options" do + question = create(:poll_question_multiple, :abcde, max_votes: 4) + option_a = question.question_options.find_by(title: "Answer A") + option_b = question.question_options.find_by(title: "Answer B") + + answer_attributes = { question: question, author: user, answer: "Answer A" } + create(:poll_answer, answer_attributes.merge(option: option_a)) + create(:poll_answer, answer_attributes.merge(option: option_b)) + + expect(Poll::Answer.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 2 + end + + it "removes duplicate answers on tenants" do + create(:tenant, schema: "answers") + + Tenant.switch("answers") do + user = create(:user, :level_two) + question = create(:poll_question_multiple, :abc) + + answer_attributes = { + question_id: question.id, + author_id: user.id, + answer: "Answer A", + option_id: nil + } + create(:poll_answer, answer_attributes) + insert(:poll_answer, answer_attributes) + + expect(Poll::Answer.count).to eq 2 + end + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + Tenant.switch("answers") do + expect(Poll::Answer.count).to eq 1 + end + end + end end From 58f88d6805fc570d3ff26edd5b95787fb14acc89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 15 May 2024 18:49:27 +0200 Subject: [PATCH 7/8] Add task to add option_id to existing answers Note: to avoid confusion, "answer" will mean a row in the poll_answers table and "choice" will mean whatever is in the "answer" column of that table (I'm applying the same convention in the code of the task). In order make this task perform reasonably on installations with millions of votes, we're using `update_all` to update all the answers with the same choice at once. In order to do that, we first need to check the existing choices and what are the possible option_ids for those choices. Note that, in order for this task to work, we need to remote the duplicate answers first. Otherwise, we will run into a RecordNotUnique exception when trying to add the same option_id to two duplicate answers. So we're making this task depend on the one that removes duplicate answers. That means we no longer need to specify the task to remove duplicate answers in the release tasks; it will automatically be executed when running the task to add an option_id. --- lib/tasks/consul.rake | 2 +- lib/tasks/polls.rake | 44 ++++++++++++++++ spec/lib/tasks/polls_spec.rb | 97 ++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 1 deletion(-) diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 0e89a360f..64225f60b 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -9,6 +9,6 @@ namespace :consul do task "execute_release_2.2.0_tasks": [ "db:mask_ips", "polls:remove_duplicate_voters", - "polls:remove_duplicate_answers" + "polls:populate_option_id" ] end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index bda3058eb..7b9472145 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -61,4 +61,48 @@ namespace :polls do end end end + + desc "populates the poll answers option_id column" + task populate_option_id: :remove_duplicate_answers do + logger = ApplicationLogger.new + logger.info "Updating option_id column in poll_answers" + + Tenant.run_on_each do + questions = Poll::Question.select do |question| + # Change this if you've added a custom votation type + # to your Consul Democracy installation that implies + # choosing among a few given options + question.unique? || question.multiple? + end + + questions.each do |question| + options = question.question_options.joins(:translations).reorder(:id) + existing_choices = question.answers.where(option_id: nil).distinct.pluck(:answer) + + choices_map = existing_choices.to_h do |choice| + [choice, options.where("lower(title) = lower(?)", choice).distinct.ids] + end + + manageable_choices, unmanageable_choices = choices_map.partition { |choice, ids| ids.count == 1 } + + manageable_choices.each do |choice, ids| + question.answers.where(option_id: nil, answer: choice).update_all(option_id: ids.first) + end + + unmanageable_choices.each do |choice, ids| + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + + if ids.count == 0 + logger.warn "Skipping poll_answers with the text \"#{choice}\" for the poll_question " \ + "with ID #{question.id}. This question has no poll_question_answers " \ + "containing the text \"#{choice}\"" + tenant_info.to_s + else + logger.warn "Skipping poll_answers with the text \"#{choice}\" for the poll_question " \ + "with ID #{question.id}. The text \"#{choice}\" could refer to any of these " \ + "IDs in the poll_question_answers table: #{ids.join(", ")}" + tenant_info.to_s + end + end + end + end + end end diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index da5050564..22e6ca409 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -120,4 +120,101 @@ describe "polls tasks" do end end end + + describe "polls:populate_option_id" do + before do + Rake::Task["polls:remove_duplicate_answers"].reenable + Rake::Task["polls:populate_option_id"].reenable + end + + it "populates the option_id column of existing answers when there's one valid answer" do + yes_no_question = create(:poll_question, :yes_no, poll: poll) + abc_question = create(:poll_question_multiple, :abc, poll: poll) + option_a = abc_question.question_options.find_by(title: "Answer A") + option_b = abc_question.question_options.find_by(title: "Answer B") + + answer = create(:poll_answer, question: yes_no_question, author: user, answer: "Yes", option: nil) + abc_answer = create(:poll_answer, question: abc_question, author: user, answer: "Answer A", option: nil) + answer_with_inconsistent_option = create(:poll_answer, question: abc_question, + author: user, + answer: "Answer A", + option: option_b) + answer_with_invalid_option = build(:poll_answer, question: abc_question, + author: user, + answer: "Non existing", + option: nil) + answer_with_invalid_option.save!(validate: false) + + Rake.application.invoke_task("polls:populate_option_id") + answer.reload + abc_answer.reload + answer_with_inconsistent_option.reload + answer_with_invalid_option.reload + + expect(answer.option_id).to eq yes_no_question.question_options.find_by(title: "Yes").id + expect(abc_answer.option_id).to eq option_a.id + expect(answer_with_inconsistent_option.option_id).to eq option_b.id + expect(answer_with_invalid_option.option_id).to be nil + end + + it "does not populate the option_id column when there are several valid options" do + question = create(:poll_question, title: "How do you pronounce it?") + + create(:poll_question_option, question: question, title_en: "A", title_es: "EI") + create(:poll_question_option, question: question, title_en: "E", title_es: "I") + create(:poll_question_option, question: question, title_en: "I", title_es: "AI") + + answer = create(:poll_answer, question: question, author: user, answer: "I", option: nil) + + Rake.application.invoke_task("polls:populate_option_id") + answer.reload + + expect(answer.option_id).to be nil + end + + it "removes duplicate answers before populating the option_id column" do + user = create(:user, :level_two) + question = create(:poll_question_multiple, :abc) + + answer_attributes = { + question_id: question.id, + author_id: user.id, + answer: "Answer A", + option_id: nil + } + answer = create(:poll_answer, answer_attributes) + insert(:poll_answer, answer_attributes) + + answer.reload + expect(answer.option_id).to be nil + + Rake.application.invoke_task("polls:populate_option_id") + answer.reload + + expect(Poll::Answer.count).to eq 1 + expect(answer.option_id).to eq question.question_options.find_by(title: "Answer A").id + end + + it "populates the option_id column on tenants" do + create(:tenant, schema: "answers") + + Tenant.switch("answers") do + question = create(:poll_question_multiple, :abc) + + create(:poll_answer, question: question, answer: "Answer A", option: nil) + end + + Rake.application.invoke_task("polls:populate_option_id") + + Tenant.switch("answers") do + expect(Poll::Question.count).to eq 1 + expect(Poll::Answer.count).to eq 1 + + question = Poll::Question.first + option_a = question.question_options.find_by(title: "Answer A") + + expect(Poll::Answer.first.option_id).to eq option_a.id + end + end + end end From 5dbd2ede14e51c5ef81b767c55a7b690efeb9425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 16 May 2024 03:07:54 +0200 Subject: [PATCH 8/8] Delete duplicate records in different languages --- app/lib/poll_option_finder.rb | 31 +++++++++++++++++++ lib/tasks/polls.rake | 56 ++++++++++++++++++----------------- spec/lib/tasks/polls_spec.rb | 50 ++++++++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 28 deletions(-) create mode 100644 app/lib/poll_option_finder.rb diff --git a/app/lib/poll_option_finder.rb b/app/lib/poll_option_finder.rb new file mode 100644 index 000000000..37544926c --- /dev/null +++ b/app/lib/poll_option_finder.rb @@ -0,0 +1,31 @@ +class PollOptionFinder + attr_reader :question + + def initialize(question) + @question = question + end + + def manageable_choices + choices_map.select { |choice, ids| ids.count == 1 } + end + + def unmanageable_choices + choices_map.reject { |choice, ids| ids.count == 1 } + end + + private + + def choices_map + @choices_map ||= existing_choices.to_h do |choice| + [choice, options.where("lower(title) = lower(?)", choice).distinct.ids] + end + end + + def options + question.question_options.joins(:translations).reorder(:id) + end + + def existing_choices + question.answers.where(option_id: nil).distinct.pluck(:answer) + end +end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index 7b9472145..a3ce11e4b 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -37,26 +37,35 @@ namespace :polls do logger.info "Removing duplicate answers in polls" Tenant.run_on_each do - duplicate_ids = Poll::Answer.where(option_id: nil) - .select(:question_id, :author_id, :answer) - .group(:question_id, :author_id, :answer) - .having("count(*) > 1") - .pluck(:question_id, :author_id, :answer) + Poll::Question.find_each do |question| + manageable_titles = PollOptionFinder.new(question).manageable_choices.keys - duplicate_ids.each do |question_id, author_id, answer| - poll_answers = Poll::Answer.where(question_id: question_id, author_id: author_id, answer: answer) + question.question_options.each do |option| + titles = option.translations.where(title: manageable_titles).select(:title).distinct - poll_answers.excluding(poll_answers.first).each do |poll_answer| - poll_answer.delete + author_ids = question.answers + .where(answer: titles) + .select(:author_id) + .group(:author_id) + .having("count(*) > 1") + .pluck(:author_id) - tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? - log_message = "Deleted duplicate record with ID #{poll_answer.id} " \ - "from the #{Poll::Answer.table_name} table " \ - "with question_id #{question_id}, " \ - "author_id #{author_id} " \ - "and answer #{answer}" + tenant_info.to_s - logger.info(log_message) - duplicate_records_logger.info(log_message) + author_ids.each do |author_id| + poll_answers = question.answers.where(option_id: nil, answer: titles, author_id: author_id) + + poll_answers.excluding(poll_answers.first).each do |poll_answer| + poll_answer.delete + + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + log_message = "Deleted duplicate record with ID #{poll_answer.id} " \ + "from the #{Poll::Answer.table_name} table " \ + "with question_id #{question.id}, " \ + "author_id #{author_id} " \ + "and answer #{poll_answer.answer}" + tenant_info.to_s + logger.info(log_message) + duplicate_records_logger.info(log_message) + end + end end end end @@ -76,20 +85,13 @@ namespace :polls do end questions.each do |question| - options = question.question_options.joins(:translations).reorder(:id) - existing_choices = question.answers.where(option_id: nil).distinct.pluck(:answer) + option_finder = PollOptionFinder.new(question) - choices_map = existing_choices.to_h do |choice| - [choice, options.where("lower(title) = lower(?)", choice).distinct.ids] - end - - manageable_choices, unmanageable_choices = choices_map.partition { |choice, ids| ids.count == 1 } - - manageable_choices.each do |choice, ids| + option_finder.manageable_choices.each do |choice, ids| question.answers.where(option_id: nil, answer: choice).update_all(option_id: ids.first) end - unmanageable_choices.each do |choice, ids| + option_finder.unmanageable_choices.each do |choice, ids| tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? if ids.count == 0 diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index 22e6ca409..b3b2eb56b 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -94,6 +94,40 @@ describe "polls tasks" do expect(Poll::Answer.count).to eq 2 end + it "removes duplicate answers in different languages" do + question = create(:poll_question_multiple, max_votes: 2) + + create(:poll_question_option, question: question, title_en: "Yes", title_de: "Ja") + create(:poll_question_option, question: question, title_en: "No", title_de: "Nein") + create(:poll_question_option, question: question, title_en: "Maybe", title_de: "Vielleicht") + + create(:poll_answer, author: user, question: question, answer: "Yes", option: nil) + create(:poll_answer, author: user, question: question, answer: "Ja", option: nil) + + expect(Poll::Answer.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 1 + end + + it "does not remove duplicate answers when many options are possible" do + question = create(:poll_question_multiple, title: "How do you pronounce it?", max_votes: 2) + + create(:poll_question_option, question: question, title_en: "A", title_es: "EI") + create(:poll_question_option, question: question, title_en: "E", title_es: "I") + create(:poll_question_option, question: question, title_en: "I", title_es: "AI") + + create(:poll_answer, question: question, author: user, answer: "I", option: nil) + create(:poll_answer, question: question, author: user, answer: "AI", option: nil) + + expect(Poll::Answer.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 2 + end + it "removes duplicate answers on tenants" do create(:tenant, schema: "answers") @@ -176,6 +210,11 @@ describe "polls tasks" do user = create(:user, :level_two) question = create(:poll_question_multiple, :abc) + localized_question = create(:poll_question_multiple) + create(:poll_question_option, question: localized_question, title_en: "Yes", title_de: "Ja") + create(:poll_question_option, question: localized_question, title_en: "No", title_de: "Nein") + create(:poll_question_option, question: localized_question, title_en: "Maybe", title_de: "Vielleicht") + answer_attributes = { question_id: question.id, author_id: user.id, @@ -185,14 +224,23 @@ describe "polls tasks" do answer = create(:poll_answer, answer_attributes) insert(:poll_answer, answer_attributes) + localized_answer_attributes = { author: user, question: localized_question, option: nil } + localized_answer = create(:poll_answer, localized_answer_attributes.merge(answer: "Yes")) + create(:poll_answer, localized_answer_attributes.merge(answer: "Ja")) + answer.reload + localized_answer.reload + expect(answer.option_id).to be nil + expect(localized_answer.option_id).to be nil Rake.application.invoke_task("polls:populate_option_id") answer.reload + localized_answer.reload - expect(Poll::Answer.count).to eq 1 + expect(Poll::Answer.count).to eq 2 expect(answer.option_id).to eq question.question_options.find_by(title: "Answer A").id + expect(localized_answer.option_id).to eq localized_question.question_options.find_by(title: "Yes").id end it "populates the option_id column on tenants" do