From 8deb1964bd5c952428bc6dd9c95350741db1b026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 25 Jun 2025 15:57:45 +0200 Subject: [PATCH] Show errors when submitting too many answers This could be the case when JavaScript is disabled. Note that, in `Poll/WebVote` we're calling `given_answers` inside a transaction. Putting this code before the transaction resulted in a test failing sometimes, probably because of a bug that might be possible to reproduce by doing simultaneous requests. --- app/components/polls/form_component.html.erb | 4 +- .../questions/question_component.html.erb | 1 + .../polls/questions/question_component.rb | 8 +-- app/models/poll/web_vote.rb | 64 +++++++++++++++---- config/locales/en/general.yml | 1 + config/locales/es/general.yml | 1 + spec/components/polls/form_component_spec.rb | 39 ++++++----- .../questions/question_component_spec.rb | 17 +++-- spec/models/poll/web_vote_spec.rb | 4 +- spec/system/polls/votation_types_spec.rb | 29 +++++++++ 10 files changed, 126 insertions(+), 42 deletions(-) diff --git a/app/components/polls/form_component.html.erb b/app/components/polls/form_component.html.erb index f3d83701b..8b0d1fe78 100644 --- a/app/components/polls/form_component.html.erb +++ b/app/components/polls/form_component.html.erb @@ -1,6 +1,6 @@ -<%= form_for poll, form_attributes do |f| %> +<%= form_for web_vote, form_attributes do |f| %> <% questions.each do |question| %> - <%= render Polls::Questions::QuestionComponent.new(question, disabled: disabled?) %> + <%= render Polls::Questions::QuestionComponent.new(question, form: f, disabled: disabled?) %> <% end %> <%= f.submit(class: "button", value: t("polls.form.vote"), disabled: disabled?) %> diff --git a/app/components/polls/questions/question_component.html.erb b/app/components/polls/questions/question_component.html.erb index 89b90f9bd..b4c5ea671 100644 --- a/app/components/polls/questions/question_component.html.erb +++ b/app/components/polls/questions/question_component.html.erb @@ -19,4 +19,5 @@

<%= options_read_more_links %>

<% end %> + <%= form.error_for(:"question_#{question.id}") %> diff --git a/app/components/polls/questions/question_component.rb b/app/components/polls/questions/question_component.rb index 2669a8688..cd74b39fc 100644 --- a/app/components/polls/questions/question_component.rb +++ b/app/components/polls/questions/question_component.rb @@ -1,10 +1,10 @@ class Polls::Questions::QuestionComponent < ApplicationComponent - attr_reader :question, :disabled + attr_reader :question, :form, :disabled alias_method :disabled?, :disabled - use_helpers :current_user - def initialize(question, disabled: false) + def initialize(question, form:, disabled: false) @question = question + @form = form @disabled = disabled end @@ -69,6 +69,6 @@ class Polls::Questions::QuestionComponent < ApplicationComponent end def checked?(option) - question.answers.where(author: current_user, option: option).any? + form.object.answers[question.id].find { |answer| answer.option_id == option.id } end end diff --git a/app/models/poll/web_vote.rb b/app/models/poll/web_vote.rb index 669ad8f1e..9ceda20e5 100644 --- a/app/models/poll/web_vote.rb +++ b/app/models/poll/web_vote.rb @@ -1,5 +1,9 @@ class Poll::WebVote + include ActiveModel::Validations attr_reader :poll, :user + delegate :t, to: "ApplicationController.helpers" + + validate :max_answers def initialize(poll, user) @poll = poll @@ -10,30 +14,68 @@ class Poll::WebVote poll.questions.for_render.sort_for_list end + def answers + @answers ||= questions.to_h do |question| + [question.id, question.answers.where(author: user)] + end + end + def update(params) all_valid = true user.with_lock do + self.answers = given_answers(params) + questions.each do |question| - question.answers.where(author: user).destroy_all - next unless params[question.id.to_s] + question.answers.where(author: user).where.not(id: answers[question.id].map(&:id)).destroy_all - option_ids = params[question.id.to_s][:option_id] - - answers = Array(option_ids).map do |option_id| - question.find_or_initialize_user_answer(user, option_id) - end - - if answers.map(&:valid?).all?(true) + if valid? && answers[question.id].all?(&:valid?) Poll::Voter.find_or_create_by!(user: user, poll: poll, origin: "web") - answers.each(&:save!) + answers[question.id].each(&:save!) else all_valid = false - raise ActiveRecord::Rollback end end + + raise ActiveRecord::Rollback unless all_valid end all_valid end + + def to_key + end + + def persisted? + Poll::Voter.where(user: user, poll: poll, origin: "web").exists? + end + + private + + attr_writer :answers + + def given_answers(params) + questions.to_h do |question| + [question.id, answers_for_question(question, params[question.id.to_s])] + end + end + + def answers_for_question(question, question_params) + return [] unless question_params + + Array(question_params[:option_id]).map do |option_id| + question.find_or_initialize_user_answer(user, option_id) + end + end + + def max_answers + questions.each do |question| + if answers[question.id].count > question.max_votes + errors.add( + :"question_#{question.id}", + t("polls.form.maximum_exceeded", maximum: question.max_votes, given: answers[question.id].count) + ) + end + end + end end diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 1298ca00a..0c4347877 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -579,6 +579,7 @@ en: final_date: "Final recounts/Results" form: vote: "Vote" + maximum_exceeded: "you've selected %{given} answers, but the maximum you can select is %{maximum}" index: filters: current: "Open" diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 9cd99d0be..2347e58e2 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -579,6 +579,7 @@ es: final_date: "Recuento final/Resultados" form: vote: "Votar" + maximum_exceeded: "has seleccionado %{given} respuestas, pero el máximo que puedes seleccionar es %{maximum}" index: filters: current: "Abiertas" diff --git a/spec/components/polls/form_component_spec.rb b/spec/components/polls/form_component_spec.rb index c6dd44b0b..43f7eeea0 100644 --- a/spec/components/polls/form_component_spec.rb +++ b/spec/components/polls/form_component_spec.rb @@ -53,31 +53,36 @@ describe Polls::FormComponent do context "geozone restricted poll" do let(:poll) { create(:poll, geozone_restricted: true) } let(:geozone) { create(:geozone) } + before { poll.geozones << geozone } - it "renders disabled fields for users from another geozone" do - poll.geozones << geozone - sign_in(user) + context "user from another geozone" do + let(:user) { create(:user, :level_two) } + before { sign_in(user) } - render_inline Polls::FormComponent.new(web_vote) + it "renders disabled fields" do + render_inline Polls::FormComponent.new(web_vote) - page.find("fieldset[disabled]") do |fieldset| - expect(fieldset).to have_field "Yes" - expect(fieldset).to have_field "No" + page.find("fieldset[disabled]") do |fieldset| + expect(fieldset).to have_field "Yes" + expect(fieldset).to have_field "No" + end + + expect(page).to have_button "Vote", disabled: true end - - expect(page).to have_button "Vote", disabled: true end - it "renders enabled fields for same-geozone users" do - poll.geozones << geozone - sign_in(create(:user, :level_two, geozone: geozone)) + context "user from the same geozone" do + let(:user) { create(:user, :level_two, geozone: geozone) } + before { sign_in(user) } - render_inline Polls::FormComponent.new(web_vote) + it "renders enabled answers" do + render_inline Polls::FormComponent.new(web_vote) - expect(page).not_to have_css "fieldset[disabled]" - expect(page).to have_field "Yes" - expect(page).to have_field "No" - expect(page).to have_button "Vote" + expect(page).not_to have_css "fieldset[disabled]" + expect(page).to have_field "Yes" + expect(page).to have_field "No" + expect(page).to have_button "Vote" + end end end end diff --git a/spec/components/polls/questions/question_component_spec.rb b/spec/components/polls/questions/question_component_spec.rb index dd76fd0fc..1b0e7c219 100644 --- a/spec/components/polls/questions/question_component_spec.rb +++ b/spec/components/polls/questions/question_component_spec.rb @@ -5,11 +5,14 @@ describe Polls::Questions::QuestionComponent do let(:question) { create(:poll_question, :yes_no, poll: poll) } let(:option_yes) { question.question_options.find_by(title: "Yes") } let(:option_no) { question.question_options.find_by(title: "No") } + let(:user) { User.new } + let(:web_vote) { Poll::WebVote.new(poll, user) } + let(:form) { ConsulFormBuilder.new(:web_vote, web_vote, ApplicationController.new.view_context, {}) } it "renders more information links when any question option has additional information" do allow_any_instance_of(Poll::Question::Option).to receive(:with_read_more?).and_return(true) - render_inline Polls::Questions::QuestionComponent.new(question) + render_inline Polls::Questions::QuestionComponent.new(question, form: form) page.find("#poll_question_#{question.id}") do |poll_question| expect(poll_question).to have_content "Read more about" @@ -20,13 +23,13 @@ describe Polls::Questions::QuestionComponent do end it "renders answers in given order" do - render_inline Polls::Questions::QuestionComponent.new(question) + render_inline Polls::Questions::QuestionComponent.new(question, form: form) expect("Yes").to appear_before("No") end it "renders disabled answers when given the disabled parameter" do - render_inline Polls::Questions::QuestionComponent.new(question, disabled: true) + render_inline Polls::Questions::QuestionComponent.new(question, form: form, disabled: true) page.find("fieldset[disabled]") do |fieldset| expect(fieldset).to have_field "Yes" @@ -39,7 +42,7 @@ describe Polls::Questions::QuestionComponent do before { sign_in(user) } it "renders radio buttons for single-choice questions" do - render_inline Polls::Questions::QuestionComponent.new(question) + render_inline Polls::Questions::QuestionComponent.new(question, form: form) expect(page).to have_field "Yes", type: :radio expect(page).to have_field "No", type: :radio @@ -47,7 +50,9 @@ describe Polls::Questions::QuestionComponent do end it "renders checkboxes for multiple-choice questions" do - render_inline Polls::Questions::QuestionComponent.new(create(:poll_question_multiple, :abc)) + question = create(:poll_question_multiple, :abc, poll: poll) + + render_inline Polls::Questions::QuestionComponent.new(question, form: form) expect(page).to have_field "Answer A", type: :checkbox expect(page).to have_field "Answer B", type: :checkbox @@ -59,7 +64,7 @@ describe Polls::Questions::QuestionComponent do it "selects the option when users have already voted" do create(:poll_answer, author: user, question: question, option: option_yes) - render_inline Polls::Questions::QuestionComponent.new(question) + render_inline Polls::Questions::QuestionComponent.new(question, form: form) expect(page).to have_field "Yes", type: :radio, checked: true expect(page).to have_field "No", type: :radio, checked: false diff --git a/spec/models/poll/web_vote_spec.rb b/spec/models/poll/web_vote_spec.rb index caa6205bd..f150b7a12 100644 --- a/spec/models/poll/web_vote_spec.rb +++ b/spec/models/poll/web_vote_spec.rb @@ -28,15 +28,15 @@ describe Poll::WebVote do end it "updates a poll_voter with user and poll data" do - create(:poll_answer, question: question, author: user, option: option_yes) + answer = create(:poll_answer, question: question, author: user, option: option_yes) web_vote.update(question.id.to_s => { option_id: option_no.id.to_s }) expect(poll.reload.voters.size).to eq 1 expect(question.reload.answers.size).to eq 1 + expect(question.answers.first).to eq answer.reload voter = poll.voters.first - answer = question.answers.first expect(answer.author).to eq user expect(answer.option).to eq option_no diff --git a/spec/system/polls/votation_types_spec.rb b/spec/system/polls/votation_types_spec.rb index a4de3edf9..9ccbcbd97 100644 --- a/spec/system/polls/votation_types_spec.rb +++ b/spec/system/polls/votation_types_spec.rb @@ -71,4 +71,33 @@ describe "Poll Votation Type" do expect(page).to have_field "Answer B", type: :checkbox, checked: false expect(page).to have_field "Answer C", type: :checkbox, checked: true end + + scenario "Too many answers", :no_js do + create(:poll_question_multiple, :abcde, poll: poll, max_votes: 2, title: "Which ones are correct?") + + visit poll_path(poll) + check "Answer A" + check "Answer B" + check "Answer D" + click_button "Vote" + + within_fieldset("Which ones are correct?") do + expect(page).to have_content "you've selected 3 answers, but the maximum you can select is 2" + expect(page).to have_field "Answer A", type: :checkbox, checked: true + expect(page).to have_field "Answer B", type: :checkbox, checked: true + expect(page).to have_field "Answer C", type: :checkbox, checked: false + expect(page).to have_field "Answer D", type: :checkbox, checked: true + expect(page).to have_field "Answer E", type: :checkbox, checked: false + end + + expect(page).not_to have_content "Thank you for voting!" + + visit poll_path(poll) + + expect(page).not_to have_content "but the maximum you can select" + + within_fieldset("Which ones are correct?") do + expect(page).to have_field type: :checkbox, checked: false, count: 5 + end + end end