From 4bb39ac3c37fc5319898300c1067bb47443d6041 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 27 Feb 2023 03:31:15 -0500 Subject: [PATCH] Fix single-record invalid condition on PollVote (#23810) --- app/models/poll_vote.rb | 1 + app/validators/vote_validator.rb | 26 +++++++++++++++-- spec/models/poll_vote_spec.rb | 49 ++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/app/models/poll_vote.rb b/app/models/poll_vote.rb index 00eaedd129..92d74a6dbc 100644 --- a/app/models/poll_vote.rb +++ b/app/models/poll_vote.rb @@ -23,6 +23,7 @@ class PollVote < ApplicationRecord after_create_commit :increment_counter_cache delegate :local?, to: :account + delegate :multiple?, :expired?, to: :poll, prefix: true def object_type :vote diff --git a/app/validators/vote_validator.rb b/app/validators/vote_validator.rb index b1692562d4..9c55f9ab6d 100644 --- a/app/validators/vote_validator.rb +++ b/app/validators/vote_validator.rb @@ -2,13 +2,13 @@ class VoteValidator < ActiveModel::Validator def validate(vote) - vote.errors.add(:base, I18n.t('polls.errors.expired')) if vote.poll.expired? + vote.errors.add(:base, I18n.t('polls.errors.expired')) if vote.poll_expired? vote.errors.add(:base, I18n.t('polls.errors.invalid_choice')) if invalid_choice?(vote) - if vote.poll.multiple? && vote.poll.votes.where(account: vote.account, choice: vote.choice).exists? + if vote.poll_multiple? && already_voted_for_same_choice_on_multiple_poll?(vote) vote.errors.add(:base, I18n.t('polls.errors.already_voted')) - elsif !vote.poll.multiple? && vote.poll.votes.where(account: vote.account).exists? + elsif !vote.poll_multiple? && already_voted_on_non_multiple_poll?(vote) vote.errors.add(:base, I18n.t('polls.errors.already_voted')) end end @@ -18,4 +18,24 @@ class VoteValidator < ActiveModel::Validator def invalid_choice?(vote) vote.choice.negative? || vote.choice >= vote.poll.options.size end + + def already_voted_for_same_choice_on_multiple_poll?(vote) + if vote.persisted? + account_votes_on_same_poll(vote).where(choice: vote.choice).where.not(poll_votes: { id: vote }).exists? + else + account_votes_on_same_poll(vote).where(choice: vote.choice).exists? + end + end + + def already_voted_on_non_multiple_poll?(vote) + if vote.persisted? + account_votes_on_same_poll(vote).where.not(poll_votes: { id: vote }).exists? + else + account_votes_on_same_poll(vote).exists? + end + end + + def account_votes_on_same_poll(vote) + vote.poll.votes.where(account: vote.account) + end end diff --git a/spec/models/poll_vote_spec.rb b/spec/models/poll_vote_spec.rb index 563f346993..6886a82aa8 100644 --- a/spec/models/poll_vote_spec.rb +++ b/spec/models/poll_vote_spec.rb @@ -10,4 +10,53 @@ RSpec.describe PollVote, type: :model do expect(poll_vote.object_type).to eq :vote end end + + describe 'validations' do + context 'with a vote on an expired poll' do + it 'marks the vote invalid' do + poll = Fabricate.build(:poll, expires_at: 30.days.ago) + + vote = Fabricate.build(:poll_vote, poll: poll) + expect(vote).to_not be_valid + end + end + + context 'with invalid choices' do + it 'marks vote invalid with negative choice' do + poll = Fabricate.build(:poll) + + vote = Fabricate.build(:poll_vote, poll: poll, choice: -100) + expect(vote).to_not be_valid + end + + it 'marks vote invalid with choice in excess of options' do + poll = Fabricate.build(:poll, options: %w(a b c)) + + vote = Fabricate.build(:poll_vote, poll: poll, choice: 10) + expect(vote).to_not be_valid + end + end + + context 'with a poll where multiple is true' do + it 'does not allow a second vote on same choice from same account' do + poll = Fabricate(:poll, multiple: true, options: %w(a b c)) + first_vote = Fabricate(:poll_vote, poll: poll, choice: 1) + expect(first_vote).to be_valid + + second_vote = Fabricate.build(:poll_vote, account: first_vote.account, poll: poll, choice: 1) + expect(second_vote).to_not be_valid + end + end + + context 'with a poll where multiple is false' do + it 'does not allow a second vote from same account' do + poll = Fabricate(:poll, multiple: false, options: %w(a b c)) + first_vote = Fabricate(:poll_vote, poll: poll) + expect(first_vote).to be_valid + + second_vote = Fabricate.build(:poll_vote, account: first_vote.account, poll: poll) + expect(second_vote).to_not be_valid + end + end + end end