From 24ccf23ed87da0348070fc02b8433c5f5664513c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 6 Aug 2020 21:44:09 +0200 Subject: [PATCH 1/3] Don't save the answer if the voter is not recorded Up until now, we were assuming the voter was valid, but were not raising an exception if it wasn't. And in the user interface everything seemed to be working properly. We were having this issue when skipping verification, when there could be voters without a document number, which would be considered invalid. Raising an exception when failing to save the voter and making sure the answer and the voter are saved inside a transaction solves the problem. --- app/controllers/polls/questions_controller.rb | 4 +--- app/models/poll/answer.rb | 8 ++++++-- spec/models/poll/answer_spec.rb | 19 +++++++++++++++---- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/app/controllers/polls/questions_controller.rb b/app/controllers/polls/questions_controller.rb index 4236f7f52..3f2809b75 100644 --- a/app/controllers/polls/questions_controller.rb +++ b/app/controllers/polls/questions_controller.rb @@ -9,9 +9,7 @@ class Polls::QuestionsController < ApplicationController token = params[:token] answer.answer = params[:answer] - answer.touch if answer.persisted? - answer.save! - answer.record_voter_participation(token) + answer.save_and_record_voter_participation(token) @answers_by_question_id = { @question.id => params[:answer] } end diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index 4beb3b73a..8742b7d64 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -14,7 +14,11 @@ class Poll::Answer < ApplicationRecord scope :by_author, ->(author_id) { where(author_id: author_id) } scope :by_question, ->(question_id) { where(question_id: question_id) } - def record_voter_participation(token) - Poll::Voter.find_or_create_by(user: author, poll: poll, origin: "web", token: token) + def save_and_record_voter_participation(token) + transaction do + touch if persisted? + save! + Poll::Voter.find_or_create_by!(user: author, poll: poll, origin: "web", token: token) + end end end diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index 6ffb5e3d1..8c0598734 100644 --- a/spec/models/poll/answer_spec.rb +++ b/spec/models/poll/answer_spec.rb @@ -37,7 +37,7 @@ describe Poll::Answer do end end - describe "#record_voter_participation" do + describe "#save_and_record_voter_participation" do let(:author) { create(:user, :level_two) } let(:poll) { create(:poll) } let(:question) { create(:poll_question, :yes_no, poll: poll) } @@ -46,7 +46,7 @@ describe Poll::Answer do answer = create(:poll_answer, question: question, author: author, answer: "Yes") expect(answer.poll.voters).to be_blank - answer.record_voter_participation("token") + answer.save_and_record_voter_participation("token") expect(poll.reload.voters.size).to eq(1) voter = poll.voters.first @@ -57,12 +57,12 @@ describe Poll::Answer do it "updates a poll_voter with user and poll data" do answer = create(:poll_answer, question: question, author: author, answer: "Yes") - answer.record_voter_participation("token") + answer.save_and_record_voter_participation("token") expect(poll.reload.voters.size).to eq(1) answer = create(:poll_answer, question: question, author: author, answer: "No") - answer.record_voter_participation("token") + answer.save_and_record_voter_participation("token") expect(poll.reload.voters.size).to eq(1) @@ -70,5 +70,16 @@ describe Poll::Answer do expect(voter.document_number).to eq(answer.author.document_number) expect(voter.poll_id).to eq(answer.poll.id) end + + it "does not save the answer if the voter is invalid" do + allow_any_instance_of(Poll::Voter).to receive(:valid?).and_return(false) + answer = build(:poll_answer) + + expect do + answer.save_and_record_voter_participation("token") + end.to raise_error(ActiveRecord::RecordInvalid) + + expect(answer).not_to be_persisted + end end end From 4367b2054a6cb66c885d50d3c76ad23523ac0b9c Mon Sep 17 00:00:00 2001 From: decabeza Date: Thu, 18 Jun 2020 10:05:14 +0200 Subject: [PATCH 2/3] Allow voting when skip verification is enabled --- app/models/poll/voter.rb | 6 +++++- spec/models/poll/voter_spec.rb | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/poll/voter.rb b/app/models/poll/voter.rb index d760f0ccd..66395aa67 100644 --- a/app/models/poll/voter.rb +++ b/app/models/poll/voter.rb @@ -14,7 +14,7 @@ class Poll validates :booth_assignment_id, presence: true, if: ->(voter) { voter.origin == "booth" } validates :officer_assignment_id, presence: true, if: ->(voter) { voter.origin == "booth" } - validates :document_number, presence: true, uniqueness: { scope: [:poll_id, :document_type], message: :has_voted } + validates :document_number, presence: true, uniqueness: { scope: [:poll_id, :document_type], message: :has_voted }, unless: :skip_user_verification? validates :origin, inclusion: { in: VALID_ORIGINS } before_validation :set_demographic_info, :set_document_info, :set_denormalized_booth_assignment_id @@ -38,6 +38,10 @@ class Poll self.document_number = user.document_number end + def skip_user_verification? + Setting["feature.user.skip_verification"].present? + end + private def set_denormalized_booth_assignment_id diff --git a/spec/models/poll/voter_spec.rb b/spec/models/poll/voter_spec.rb index 8cf664d68..c919e971f 100644 --- a/spec/models/poll/voter_spec.rb +++ b/spec/models/poll/voter_spec.rb @@ -172,5 +172,14 @@ describe Poll::Voter do expect(voter.document_type).to eq("1") expect(voter.token).to eq("1234abcd") end + + it "sets user info with skip verification enabled" do + Setting["feature.user.skip_verification"] = true + user = create(:user) + voter = build(:poll_voter, user: user, token: "1234abcd") + voter.save! + + expect(voter.token).to eq("1234abcd") + end end end From 096f546c24bc41d8846b0808f218f9430bd9ba8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 6 Aug 2020 22:05:39 +0200 Subject: [PATCH 3/3] Make sure users only vote once in the same poll When skipping verification, we cannot apply the validation rule saying the document number and document type must be unique, because they'll be `nil` in many cases. So we were skipping the rule, but that makes it possible for the same user to vote several times (for instance, once in a booth and once via web). So we're changing the scope of the uniqueness rule: instead of being unique per document number, voters are unique per user. The reason we made them unique per document number was that back in commit 900563e3 (when we added the rule), we hadn't added the relation between users and poll voters yet. --- app/models/poll/voter.rb | 3 ++- config/locales/en/activerecord.yml | 1 + config/locales/es/activerecord.yml | 1 + spec/models/poll/voter_spec.rb | 32 ++++++++++++++++++++++++++---- 4 files changed, 32 insertions(+), 5 deletions(-) diff --git a/app/models/poll/voter.rb b/app/models/poll/voter.rb index 66395aa67..2ea150718 100644 --- a/app/models/poll/voter.rb +++ b/app/models/poll/voter.rb @@ -14,7 +14,8 @@ class Poll validates :booth_assignment_id, presence: true, if: ->(voter) { voter.origin == "booth" } validates :officer_assignment_id, presence: true, if: ->(voter) { voter.origin == "booth" } - validates :document_number, presence: true, uniqueness: { scope: [:poll_id, :document_type], message: :has_voted }, unless: :skip_user_verification? + validates :document_number, presence: true, unless: :skip_user_verification? + validates :user_id, uniqueness: { scope: [:poll_id], message: :has_voted } validates :origin, inclusion: { in: VALID_ORIGINS } before_validation :set_demographic_info, :set_document_info, :set_denormalized_booth_assignment_id diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 9ea6e1c51..dd024d838 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -508,6 +508,7 @@ en: attributes: document_number: not_in_census: "Document not in census" + user_id: has_voted: "User has already voted" legislation/process: attributes: diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index c3bc2c3e9..3d324a27c 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -510,6 +510,7 @@ es: attributes: document_number: not_in_census: "Este documento no aparece en el censo" + user_id: has_voted: "Este usuario ya ha votado" legislation/process: attributes: diff --git a/spec/models/poll/voter_spec.rb b/spec/models/poll/voter_spec.rb index c919e971f..9ed82be67 100644 --- a/spec/models/poll/voter_spec.rb +++ b/spec/models/poll/voter_spec.rb @@ -34,7 +34,7 @@ describe Poll::Voter do voter = build(:poll_voter, user: user, poll: poll) expect(voter).not_to be_valid - expect(voter.errors.messages[:document_number]).to eq(["User has already voted"]) + expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) end it "is not valid if the user has already voted in the same poll/booth" do @@ -43,7 +43,7 @@ describe Poll::Voter do voter = build(:poll_voter, user: user, poll: poll, booth_assignment: booth_assignment) expect(voter).not_to be_valid - expect(voter.errors.messages[:document_number]).to eq(["User has already voted"]) + expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) end it "is not valid if the user has already voted in different booth in the same poll" do @@ -52,7 +52,7 @@ describe Poll::Voter do voter = build(:poll_voter, :from_booth, user: user, poll: poll, booth: booth) expect(voter).not_to be_valid - expect(voter.errors.messages[:document_number]).to eq(["User has already voted"]) + expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) end it "is valid if the user has already voted in the same booth in different poll" do @@ -69,7 +69,31 @@ describe Poll::Voter do voter = build(:poll_voter, poll: answer.question.poll, user: answer.author) expect(voter).not_to be_valid - expect(voter.errors.messages[:document_number]).to eq(["User has already voted"]) + expect(voter.errors.messages[:user_id]).to eq(["User has already voted"]) + end + + context "Skip verification is enabled" do + before do + Setting["feature.user.skip_verification"] = true + user.update!(document_number: nil, document_type: nil) + end + + it "is not valid if the user has already voted in the same poll" do + create(:poll_voter, user: user, poll: poll) + + voter = build(:poll_voter, user: user, poll: poll) + + expect(voter).not_to be_valid + end + + it "is valid if other users have voted in the same poll" do + another_user = create(:user, :level_two, document_number: nil, document_type: nil) + create(:poll_voter, user: another_user, poll: poll) + + voter = build(:poll_voter, user: user, poll: poll) + + expect(voter).to be_valid + end end context "origin" do