From 32aa83e9d79c5a11ea39c90ba3d6f67f091e6499 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 28 Jan 2025 15:38:18 +0100 Subject: [PATCH] Fix polls not being validated on edition (#33755) --- app/models/poll.rb | 3 +- app/serializers/rest/instance_serializer.rb | 8 ++-- .../rest/v1/instance_serializer.rb | 8 ++-- app/validators/poll_expiration_validator.rb | 13 ++++++ ...validator.rb => poll_options_validator.rb} | 8 +--- spec/requests/api/v2/instance_spec.rb | 2 +- ...c.rb => poll_expiration_validator_spec.rb} | 16 +++++-- .../validators/poll_options_validator_spec.rb | 45 +++++++++++++++++++ 8 files changed, 82 insertions(+), 21 deletions(-) create mode 100644 app/validators/poll_expiration_validator.rb rename app/validators/{poll_validator.rb => poll_options_validator.rb} (57%) rename spec/validators/{poll_validator_spec.rb => poll_expiration_validator_spec.rb} (64%) create mode 100644 spec/validators/poll_options_validator_spec.rb diff --git a/app/models/poll.rb b/app/models/poll.rb index 93ef0cc589..7c59339b7e 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -37,7 +37,8 @@ class Poll < ApplicationRecord validates :options, presence: true validates :expires_at, presence: true, if: :local? - validates_with PollValidator, on: :create, if: :local? + validates_with PollOptionsValidator, if: :local? + validates_with PollExpirationValidator, if: -> { local? && expires_at_changed? } before_validation :prepare_options, if: :local? before_validation :prepare_votes_count diff --git a/app/serializers/rest/instance_serializer.rb b/app/serializers/rest/instance_serializer.rb index 936748707f..e15cba7cc8 100644 --- a/app/serializers/rest/instance_serializer.rb +++ b/app/serializers/rest/instance_serializer.rb @@ -87,10 +87,10 @@ class REST::InstanceSerializer < ActiveModel::Serializer }, polls: { - max_options: PollValidator::MAX_OPTIONS, - max_characters_per_option: PollValidator::MAX_OPTION_CHARS, - min_expiration: PollValidator::MIN_EXPIRATION, - max_expiration: PollValidator::MAX_EXPIRATION, + max_options: PollOptionsValidator::MAX_OPTIONS, + max_characters_per_option: PollOptionsValidator::MAX_OPTION_CHARS, + min_expiration: PollExpirationValidator::MIN_EXPIRATION, + max_expiration: PollExpirationValidator::MAX_EXPIRATION, }, translation: { diff --git a/app/serializers/rest/v1/instance_serializer.rb b/app/serializers/rest/v1/instance_serializer.rb index db83af4907..a1d51f50e4 100644 --- a/app/serializers/rest/v1/instance_serializer.rb +++ b/app/serializers/rest/v1/instance_serializer.rb @@ -70,10 +70,10 @@ class REST::V1::InstanceSerializer < ActiveModel::Serializer }, polls: { - max_options: PollValidator::MAX_OPTIONS, - max_characters_per_option: PollValidator::MAX_OPTION_CHARS, - min_expiration: PollValidator::MIN_EXPIRATION, - max_expiration: PollValidator::MAX_EXPIRATION, + max_options: PollOptionsValidator::MAX_OPTIONS, + max_characters_per_option: PollOptionsValidator::MAX_OPTION_CHARS, + min_expiration: PollExpirationValidator::MIN_EXPIRATION, + max_expiration: PollExpirationValidator::MAX_EXPIRATION, }, } end diff --git a/app/validators/poll_expiration_validator.rb b/app/validators/poll_expiration_validator.rb new file mode 100644 index 0000000000..ea8b08e186 --- /dev/null +++ b/app/validators/poll_expiration_validator.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class PollExpirationValidator < ActiveModel::Validator + MAX_EXPIRATION = 1.month.freeze + MIN_EXPIRATION = 5.minutes.freeze + + def validate(poll) + current_time = Time.now.utc + + poll.errors.add(:expires_at, I18n.t('polls.errors.duration_too_long')) if poll.expires_at.nil? || poll.expires_at - current_time > MAX_EXPIRATION + poll.errors.add(:expires_at, I18n.t('polls.errors.duration_too_short')) if poll.expires_at.present? && (poll.expires_at - current_time).ceil < MIN_EXPIRATION + end +end diff --git a/app/validators/poll_validator.rb b/app/validators/poll_options_validator.rb similarity index 57% rename from app/validators/poll_validator.rb rename to app/validators/poll_options_validator.rb index a327277963..0ac84f93f4 100644 --- a/app/validators/poll_validator.rb +++ b/app/validators/poll_options_validator.rb @@ -1,19 +1,13 @@ # frozen_string_literal: true -class PollValidator < ActiveModel::Validator +class PollOptionsValidator < ActiveModel::Validator MAX_OPTIONS = 4 MAX_OPTION_CHARS = 50 - MAX_EXPIRATION = 1.month.freeze - MIN_EXPIRATION = 5.minutes.freeze def validate(poll) - current_time = Time.now.utc - poll.errors.add(:options, I18n.t('polls.errors.too_few_options')) unless poll.options.size > 1 poll.errors.add(:options, I18n.t('polls.errors.too_many_options', max: MAX_OPTIONS)) if poll.options.size > MAX_OPTIONS poll.errors.add(:options, I18n.t('polls.errors.over_character_limit', max: MAX_OPTION_CHARS)) if poll.options.any? { |option| option.mb_chars.grapheme_length > MAX_OPTION_CHARS } poll.errors.add(:options, I18n.t('polls.errors.duplicate_options')) unless poll.options.uniq.size == poll.options.size - poll.errors.add(:expires_at, I18n.t('polls.errors.duration_too_long')) if poll.expires_at.nil? || poll.expires_at - current_time > MAX_EXPIRATION - poll.errors.add(:expires_at, I18n.t('polls.errors.duration_too_short')) if poll.expires_at.present? && (poll.expires_at - current_time).ceil < MIN_EXPIRATION end end diff --git a/spec/requests/api/v2/instance_spec.rb b/spec/requests/api/v2/instance_spec.rb index bdccfdb626..788d30fa69 100644 --- a/spec/requests/api/v2/instance_spec.rb +++ b/spec/requests/api/v2/instance_spec.rb @@ -59,7 +59,7 @@ RSpec.describe 'Instances' do description_limit: MediaAttachment::MAX_DESCRIPTION_LENGTH ), polls: include( - max_options: PollValidator::MAX_OPTIONS + max_options: PollOptionsValidator::MAX_OPTIONS ) ) ) diff --git a/spec/validators/poll_validator_spec.rb b/spec/validators/poll_expiration_validator_spec.rb similarity index 64% rename from spec/validators/poll_validator_spec.rb rename to spec/validators/poll_expiration_validator_spec.rb index f2a2534898..41b8c96211 100644 --- a/spec/validators/poll_validator_spec.rb +++ b/spec/validators/poll_expiration_validator_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe PollValidator do +RSpec.describe PollExpirationValidator do describe '#validate' do before do validator.validate(poll) @@ -14,16 +14,24 @@ RSpec.describe PollValidator do let(:options) { %w(foo bar) } let(:expires_at) { 1.day.from_now } - it 'have no errors' do + it 'has no errors' do expect(errors).to_not have_received(:add) end - context 'when expires is just 5 min ago' do + context 'when the poll expires in 5 min from now' do let(:expires_at) { 5.minutes.from_now } - it 'not calls errors add' do + it 'has no errors' do expect(errors).to_not have_received(:add) end end + + context 'when the poll expires in the past' do + let(:expires_at) { 5.minutes.ago } + + it 'has errors' do + expect(errors).to have_received(:add) + end + end end end diff --git a/spec/validators/poll_options_validator_spec.rb b/spec/validators/poll_options_validator_spec.rb new file mode 100644 index 0000000000..9e4ec744db --- /dev/null +++ b/spec/validators/poll_options_validator_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe PollOptionsValidator do + describe '#validate' do + before do + validator.validate(poll) + end + + let(:validator) { described_class.new } + let(:poll) { instance_double(Poll, options: options, expires_at: expires_at, errors: errors) } + let(:errors) { instance_double(ActiveModel::Errors, add: nil) } + let(:options) { %w(foo bar) } + let(:expires_at) { 1.day.from_now } + + it 'has no errors' do + expect(errors).to_not have_received(:add) + end + + context 'when the poll has duplicate options' do + let(:options) { %w(foo foo) } + + it 'adds errors' do + expect(errors).to have_received(:add) + end + end + + context 'when the poll has no options' do + let(:options) { [] } + + it 'adds errors' do + expect(errors).to have_received(:add) + end + end + + context 'when the poll has too many options' do + let(:options) { Array.new(described_class::MAX_OPTIONS + 1) { |i| "option #{i}" } } + + it 'adds errors' do + expect(errors).to have_received(:add) + end + end + end +end