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.
This commit is contained in:
@@ -36,7 +36,7 @@ class Poll::Answer < ApplicationRecord
|
|||||||
private
|
private
|
||||||
|
|
||||||
def max_votes
|
def max_votes
|
||||||
return if !question || !author || question&.unique? || persisted?
|
return if !question || !author || persisted?
|
||||||
|
|
||||||
author.lock!
|
author.lock!
|
||||||
|
|
||||||
|
|||||||
@@ -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 :question_options, reject_if: :all_blank, allow_destroy: true
|
||||||
accepts_nested_attributes_for :votation_type
|
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) }
|
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?
|
votation_type.nil? || votation_type.unique?
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def max_votes
|
||||||
|
if multiple?
|
||||||
|
votation_type.max_votes
|
||||||
|
else
|
||||||
|
1
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
def find_or_initialize_user_answer(user, title)
|
def find_or_initialize_user_answer(user, title)
|
||||||
answer = answers.find_or_initialize_by(find_by_attributes(user, title))
|
answer = answers.find_or_initialize_by(find_by_attributes(user, title))
|
||||||
answer.answer = title
|
answer.answer = title
|
||||||
|
|||||||
@@ -30,6 +30,17 @@ describe Poll::Answer do
|
|||||||
expect(answer).not_to be_valid
|
expect(answer).not_to be_valid
|
||||||
end
|
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
|
it "is not valid when user already reached multiple answers question max votes" do
|
||||||
author = create(:user)
|
author = create(:user)
|
||||||
question = create(:poll_question_multiple, :abc, max_votes: 2)
|
question = create(:poll_question_multiple, :abc, max_votes: 2)
|
||||||
@@ -40,20 +51,6 @@ describe Poll::Answer do
|
|||||||
expect(answer).not_to be_valid
|
expect(answer).not_to be_valid
|
||||||
end
|
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
|
it "is valid for answers included in the Poll::Question's question_options list" do
|
||||||
question = create(:poll_question)
|
question = create(:poll_question)
|
||||||
create(:poll_question_option, title: "One", question: 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
|
expect(build(:poll_answer, question: question, answer: "Four")).not_to be_valid
|
||||||
end
|
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
|
end
|
||||||
|
|
||||||
describe "#save_and_record_voter_participation" do
|
describe "#save_and_record_voter_participation" do
|
||||||
@@ -92,14 +119,14 @@ describe Poll::Answer do
|
|||||||
|
|
||||||
expect(poll.reload.voters.size).to eq(1)
|
expect(poll.reload.voters.size).to eq(1)
|
||||||
|
|
||||||
answer = create(:poll_answer, question: question, author: author, answer: "No")
|
updated_answer = answer.question.find_or_initialize_user_answer(answer.author, "No")
|
||||||
answer.save_and_record_voter_participation
|
updated_answer.save_and_record_voter_participation
|
||||||
|
|
||||||
expect(poll.reload.voters.size).to eq(1)
|
expect(poll.reload.voters.size).to eq(1)
|
||||||
|
|
||||||
voter = poll.voters.first
|
voter = poll.voters.first
|
||||||
expect(voter.document_number).to eq(answer.author.document_number)
|
expect(voter.document_number).to eq(updated_answer.author.document_number)
|
||||||
expect(voter.poll_id).to eq(answer.poll.id)
|
expect(voter.poll_id).to eq(updated_answer.poll.id)
|
||||||
end
|
end
|
||||||
|
|
||||||
it "does not save the answer if the voter is invalid" do
|
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
|
expect(answer).not_to be_persisted
|
||||||
end
|
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
|
end
|
||||||
|
|
||||||
describe "#destroy_and_remove_voter_participation" do
|
describe "#destroy_and_remove_voter_participation" do
|
||||||
|
|||||||
Reference in New Issue
Block a user