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