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] 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)