From 8a26954bc5f870faf724779123a4bf53e86c424f Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Thu, 15 Sep 2022 16:36:50 +0200 Subject: [PATCH] Don't allow to modify questions for started polls Adding, modifiying, and/or deleting questions for an already started poll is far away from being democratic and can lead to unwanted side effects like missing votes in the results or stats. So, from now on, only modifiying questions will be possible only if the poll has not started yet. --- .../poll/questions/form_component.html.erb | 4 +- .../admin/poll/questions/form_component.rb | 9 ++ .../table_actions_component.html.erb | 3 + .../poll/questions/table_actions_component.rb | 14 +++ .../admin/poll/questions_controller.rb | 5 +- app/models/abilities/administrator.rb | 9 +- app/models/poll.rb | 4 + .../admin/poll/polls/_poll_header.html.erb | 4 + .../admin/poll/polls/_questions.html.erb | 10 +- app/views/admin/poll/questions/show.html.erb | 6 +- config/locales/en/admin.yml | 3 + config/locales/es/admin.yml | 3 + .../poll/questions/form_component_spec.rb | 37 ++++++ .../questions/table_actions_component_spec.rb | 25 +++++ .../admin/poll/questions_controller_spec.rb | 105 ++++++++++++++++++ spec/models/abilities/administrator_spec.rb | 20 ++-- spec/system/admin/poll/questions_spec.rb | 61 +++++----- spec/system/admin/translatable_spec.rb | 2 +- 18 files changed, 277 insertions(+), 47 deletions(-) create mode 100644 app/components/admin/poll/questions/table_actions_component.html.erb create mode 100644 app/components/admin/poll/questions/table_actions_component.rb create mode 100644 spec/components/admin/poll/questions/form_component_spec.rb create mode 100644 spec/components/admin/poll/questions/table_actions_component_spec.rb create mode 100644 spec/controllers/admin/poll/questions_controller_spec.rb diff --git a/app/components/admin/poll/questions/form_component.html.erb b/app/components/admin/poll/questions/form_component.html.erb index 34193725f..fc183f186 100644 --- a/app/components/admin/poll/questions/form_component.html.erb +++ b/app/components/admin/poll/questions/form_component.html.erb @@ -12,10 +12,10 @@ <%= f.hidden_field :poll_id, value: question.poll.id %> <% else %>
- <% select_options = Poll.all.map { |p| [p.name, p.id] } %> <%= f.select :poll_id, options_for_select(select_options), - prompt: t("admin.questions.index.select_poll") %> + prompt: t("admin.questions.index.select_poll"), + hint: t("admin.questions.form.poll_help") %>
<% end %> diff --git a/app/components/admin/poll/questions/form_component.rb b/app/components/admin/poll/questions/form_component.rb index 4930c984e..5f54d84e2 100644 --- a/app/components/admin/poll/questions/form_component.rb +++ b/app/components/admin/poll/questions/form_component.rb @@ -2,9 +2,18 @@ class Admin::Poll::Questions::FormComponent < ApplicationComponent include TranslatableFormHelper include GlobalizeHelper attr_reader :question, :url + delegate :can?, to: :helpers def initialize(question, url:) @question = question @url = url end + + private + + def select_options + Poll.all.select { |poll| can?(:create, Poll::Question.new(poll: poll)) }.map do |poll| + [poll.name, poll.id] + end + end end diff --git a/app/components/admin/poll/questions/table_actions_component.html.erb b/app/components/admin/poll/questions/table_actions_component.html.erb new file mode 100644 index 000000000..5858ad82c --- /dev/null +++ b/app/components/admin/poll/questions/table_actions_component.html.erb @@ -0,0 +1,3 @@ +<%= render Admin::TableActionsComponent.new(question, actions: actions) do |table_actions| %> + <%= table_actions.action(:answers, text: t("admin.polls.show.edit_answers")) %> +<% end %> diff --git a/app/components/admin/poll/questions/table_actions_component.rb b/app/components/admin/poll/questions/table_actions_component.rb new file mode 100644 index 000000000..85c7bb279 --- /dev/null +++ b/app/components/admin/poll/questions/table_actions_component.rb @@ -0,0 +1,14 @@ +class Admin::Poll::Questions::TableActionsComponent < ApplicationComponent + attr_reader :question + delegate :can?, to: :helpers + + def initialize(question) + @question = question + end + + private + + def actions + [:edit, :destroy].select { |action| can?(action, question) } + end +end diff --git a/app/controllers/admin/poll/questions_controller.rb b/app/controllers/admin/poll/questions_controller.rb index ea133c65f..c0c8540f8 100644 --- a/app/controllers/admin/poll/questions_controller.rb +++ b/app/controllers/admin/poll/questions_controller.rb @@ -3,7 +3,8 @@ class Admin::Poll::QuestionsController < Admin::Poll::BaseController include Translatable load_and_authorize_resource :poll - load_and_authorize_resource :question, class: "Poll::Question" + load_resource class: "Poll::Question" + authorize_resource except: [:new, :index] def index @polls = Poll.not_budget @@ -17,6 +18,8 @@ class Admin::Poll::QuestionsController < Admin::Poll::BaseController proposal = Proposal.find(params[:proposal_id]) if params[:proposal_id].present? @question.copy_attributes_from_proposal(proposal) @question.poll = @poll + + authorize! :create, @question end def create diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index e9a455c3c..437b678a1 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -88,8 +88,13 @@ module Abilities can [:search, :create, :index, :destroy], ::Poll::Officer can [:create, :destroy, :manage], ::Poll::BoothAssignment can [:create, :destroy], ::Poll::OfficerAssignment - can [:read, :create, :update], Poll::Question - can :destroy, Poll::Question + can :read, Poll::Question + can [:create], Poll::Question do |question| + question.poll.blank? || !question.poll.started? + end + can [:update, :destroy], Poll::Question do |question| + !question.poll.started? + end can :manage, Poll::Question::Answer can :manage, Poll::Question::Answer::Video can [:create, :destroy], Image do |image| diff --git a/app/models/poll.rb b/app/models/poll.rb index eafa7d14c..18cba6d9b 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -71,6 +71,10 @@ class Poll < ApplicationRecord name end + def started?(timestamp = Time.current) + starts_at.present? && starts_at < timestamp + end + def current?(timestamp = Time.current) starts_at <= timestamp && timestamp <= ends_at end diff --git a/app/views/admin/poll/polls/_poll_header.html.erb b/app/views/admin/poll/polls/_poll_header.html.erb index c6dcd0e7c..fafabcbfc 100644 --- a/app/views/admin/poll/polls/_poll_header.html.erb +++ b/app/views/admin/poll/polls/_poll_header.html.erb @@ -23,3 +23,7 @@ <% end %> + +
+ <%= t("admin.questions.no_edit") %> +
diff --git a/app/views/admin/poll/polls/_questions.html.erb b/app/views/admin/poll/polls/_questions.html.erb index e7f1fc7a0..acae1cf4f 100644 --- a/app/views/admin/poll/polls/_questions.html.erb +++ b/app/views/admin/poll/polls/_questions.html.erb @@ -1,7 +1,9 @@

<%= t("admin.polls.show.questions_title") %>

-<%= link_to t("admin.questions.index.create"), new_admin_question_path(poll_id: @poll.id), - class: "button float-right" %> +<% if can?(:create, Poll::Question.new(poll: @poll)) %> + <%= link_to t("admin.questions.index.create"), new_admin_question_path(poll_id: @poll.id), + class: "button float-right" %> +<% end %> <% if @poll.questions.empty? %>
@@ -28,9 +30,7 @@ <% end %> - <%= render Admin::TableActionsComponent.new(question) do |actions| %> - <%= actions.action(:answers, text: t("admin.polls.show.edit_answers")) %> - <% end %> + <%= render Admin::Poll::Questions::TableActionsComponent.new(question) %> <% end %> diff --git a/app/views/admin/poll/questions/show.html.erb b/app/views/admin/poll/questions/show.html.erb index 954767747..bfeff9f6d 100644 --- a/app/views/admin/poll/questions/show.html.erb +++ b/app/views/admin/poll/questions/show.html.erb @@ -1,7 +1,9 @@ <%= back_link_to admin_poll_path(@question.poll) %> -<%= link_to t("admin.questions.show.edit_question"), edit_admin_question_path(@question), - class: "button hollow float-right" %> +<% if can?(:update, @question) %> + <%= link_to t("admin.questions.show.edit_question"), edit_admin_question_path(@question), + class: "button hollow float-right" %> +<% end %>
diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 2663a3436..e1a1bd5cf 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -1123,6 +1123,8 @@ en: poll_not_assigned: "Poll not assigned" edit: title: "Edit Question" + form: + poll_help: "You can only select polls that have not started yet" new: title: "Create question to poll %{poll}" title_proposal: "Create question" @@ -1151,6 +1153,7 @@ en: documents_list: Documents list document_title: Title document_actions: Actions + no_edit: "Once the poll has started it will not be possible to create, edit or delete questions, answers or any content associated with the poll." answers: new: title: New answer diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 8e997a466..acae33b58 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -1122,6 +1122,8 @@ es: poll_not_assigned: "Votación no asignada" edit: title: "Editar pregunta ciudadana" + form: + poll_help: "Solo se pueden seleccionar votaciones que no hayan empezado" new: title: "Crear pregunta ciudadana para la votación %{poll}" title_proposal: "Crear pregunta ciudadana" @@ -1150,6 +1152,7 @@ es: documents_list: Lista de documentos document_title: Título document_actions: Acciones + no_edit: "Una vez comenzada la votación no será posible crear, editar o eliminar preguntas, respuestas o cualquier contenido asociado a la votación." answers: new: title: Nueva respuesta diff --git a/spec/components/admin/poll/questions/form_component_spec.rb b/spec/components/admin/poll/questions/form_component_spec.rb new file mode 100644 index 000000000..13f38085f --- /dev/null +++ b/spec/components/admin/poll/questions/form_component_spec.rb @@ -0,0 +1,37 @@ +require "rails_helper" + +describe Admin::Poll::Questions::FormComponent do + before { sign_in(create(:administrator).user) } + + context "question with a poll" do + let(:poll) { create(:poll) } + let(:question) { Poll::Question.new(poll: poll) } + + it "does not display the poll selector" do + render_inline Admin::Poll::Questions::FormComponent.new(question, url: "/") + + expect(page).not_to have_select "Poll" + expect(page).to have_field "poll_question[poll_id]", type: :hidden, with: poll.id + end + end + + context "question without a poll" do + let(:question) { Poll::Question.new } + + it "allows selecting polls which have not already started" do + create(:poll, :future, name: "Future poll") + + render_inline Admin::Poll::Questions::FormComponent.new(question, url: "/") + + expect(page).to have_select "Poll", options: ["Select Poll", "Future poll"] + end + + it "does not allow selecting polls which have already started" do + create(:poll, name: "Already started poll") + + render_inline Admin::Poll::Questions::FormComponent.new(question, url: "/") + + expect(page).to have_select "Poll", options: ["Select Poll"] + end + end +end diff --git a/spec/components/admin/poll/questions/table_actions_component_spec.rb b/spec/components/admin/poll/questions/table_actions_component_spec.rb new file mode 100644 index 000000000..b1b84513b --- /dev/null +++ b/spec/components/admin/poll/questions/table_actions_component_spec.rb @@ -0,0 +1,25 @@ +require "rails_helper" + +describe Admin::Poll::Questions::TableActionsComponent, controller: Admin::BaseController do + before { sign_in(create(:administrator).user) } + + it "displays the edit and destroy actions when the poll has not started" do + question = create(:poll_question, poll: create(:poll, :future)) + + render_inline Admin::Poll::Questions::TableActionsComponent.new(question) + + expect(page).to have_link "Edit answers" + expect(page).to have_link "Edit" + expect(page).to have_button "Delete" + end + + it "does not display the edit and destroy actions when the poll has started" do + question = create(:poll_question, poll: create(:poll)) + + render_inline Admin::Poll::Questions::TableActionsComponent.new(question) + + expect(page).to have_link "Edit answers" + expect(page).not_to have_link "Edit" + expect(page).not_to have_button "Delete" + end +end diff --git a/spec/controllers/admin/poll/questions_controller_spec.rb b/spec/controllers/admin/poll/questions_controller_spec.rb new file mode 100644 index 000000000..8358e7a3c --- /dev/null +++ b/spec/controllers/admin/poll/questions_controller_spec.rb @@ -0,0 +1,105 @@ +require "rails_helper" + +describe Admin::Poll::QuestionsController, :admin do + let(:current_poll) { create(:poll) } + let(:future_poll) { create(:poll, :future) } + + describe "POST create" do + it "is not possible for an already started poll" do + post :create, params: { + poll_question: { + translations_attributes: { + "0" => { + locale: "en", + title: "Question from started poll" + } + }, + poll_id: current_poll + } + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'create' on Question." + expect(Poll::Question.count).to eq 0 + end + + it "is possible for a not started poll" do + post :create, params: { + poll_question: { + translations_attributes: { + "0" => { + locale: "en", + title: "Question from future poll" + } + }, + poll_id: future_poll + } + } + + expect(response).to redirect_to admin_question_path(Poll::Question.last) + expect(Poll::Question.last.title).to eq "Question from future poll" + expect(Poll::Question.count).to eq 1 + end + end + + describe "PATCH update" do + it "is not possible for an already started poll" do + current_question = create(:poll_question, poll: current_poll, title: "Sample title") + + patch :update, params: { + poll_question: { + translations_attributes: { + "0" => { + locale: "en", + title: "New title", + id: current_question.translations.first.id + } + } + }, + id: current_question + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'update' on Question." + expect(current_question.reload.title).to eq "Sample title" + end + + it "is possible for a not started poll" do + future_question = create(:poll_question, poll: future_poll) + + patch :update, params: { + poll_question: { + translations_attributes: { + "0" => { + locale: "en", + title: "New title", + id: future_question.translations.first.id + } + } + }, + id: future_question + } + + expect(response).to redirect_to admin_question_path(future_question) + expect(flash[:notice]).to eq "Changes saved" + expect(future_question.reload.title).to eq "New title" + end + end + + describe "DELETE destroy" do + it "is not possible for an already started poll" do + current_question = create(:poll_question, poll: current_poll) + delete :destroy, params: { id: current_question } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'destroy' on Question." + expect(Poll::Question.count).to eq 1 + end + + it "is possible for a not started poll" do + future_question = create(:poll_question, poll: future_poll) + delete :destroy, params: { id: future_question } + + expect(response).to redirect_to admin_poll_path(future_poll) + expect(flash[:notice]).to eq "Question deleted successfully" + expect(Poll::Question.count).to eq 0 + end + end +end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 72e1722ee..f9a6da74f 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -16,8 +16,10 @@ describe Abilities::Administrator do let(:budget_investment) { create(:budget_investment) } let(:finished_investment) { create(:budget_investment, budget: create(:budget, :finished)) } let(:legislation_question) { create(:legislation_question) } - let(:poll) { create(:poll) } - let(:poll_question) { create(:poll_question) } + let(:current_poll) { create(:poll) } + let(:future_poll) { create(:poll, :future) } + let(:current_poll_question) { create(:poll_question) } + let(:future_poll_question) { create(:poll_question, poll: future_poll) } let(:poll_question_answer) { create(:poll_question_answer) } let(:answer_image) { build(:image, imageable: poll_question_answer) } @@ -27,7 +29,7 @@ describe Abilities::Administrator do let(:proposal_document) { build(:document, documentable: proposal, user: proposal.author) } let(:budget_investment_document) { build(:document, documentable: budget_investment) } - let(:poll_question_document) { build(:document, documentable: poll_question) } + let(:poll_question_document) { build(:document, documentable: current_poll_question) } let(:proposal_image) { build(:image, imageable: proposal, user: proposal.author) } let(:budget_investment_image) { build(:image, imageable: budget_investment) } @@ -74,8 +76,8 @@ describe Abilities::Administrator do it { should be_able_to(:comment_as_administrator, legislation_question) } it { should_not be_able_to(:comment_as_moderator, legislation_question) } - it { should be_able_to(:comment_as_administrator, poll) } - it { should_not be_able_to(:comment_as_moderator, poll) } + it { should be_able_to(:comment_as_administrator, current_poll) } + it { should_not be_able_to(:comment_as_moderator, current_poll) } it { should be_able_to(:summary, past_process) } it { should_not be_able_to(:summary, past_draft_process) } @@ -113,8 +115,12 @@ describe Abilities::Administrator do it { should be_able_to(:manage, Dashboard::Action) } it { should be_able_to(:read, Poll::Question) } - it { should be_able_to(:create, Poll::Question) } - it { should be_able_to(:update, Poll::Question) } + it { should be_able_to(:create, future_poll_question) } + it { should be_able_to(:update, future_poll_question) } + it { should be_able_to(:destroy, future_poll_question) } + it { should_not be_able_to(:create, current_poll_question) } + it { should_not be_able_to(:update, current_poll_question) } + it { should_not be_able_to(:destroy, current_poll_question) } it { should be_able_to(:manage, Poll::Question::Answer) } diff --git a/spec/system/admin/poll/questions_spec.rb b/spec/system/admin/poll/questions_spec.rb index 14a622e37..2297678b8 100644 --- a/spec/system/admin/poll/questions_spec.rb +++ b/spec/system/admin/poll/questions_spec.rb @@ -2,9 +2,9 @@ require "rails_helper" describe "Admin poll questions", :admin do scenario "Index" do - poll1 = create(:poll) - poll2 = create(:poll) - poll3 = create(:poll) + poll1 = create(:poll, :future) + poll2 = create(:poll, :future) + poll3 = create(:poll, :future) proposal = create(:proposal) question1 = create(:poll_question, poll: poll1) question2 = create(:poll_question, poll: poll2) @@ -55,25 +55,32 @@ describe "Admin poll questions", :admin do expect(page).to have_content question.author.name end - scenario "Create" do - poll = create(:poll, name: "Movies") - title = "Star Wars: Episode IV - A New Hope" + describe "Create" do + scenario "Is possible for a not started poll" do + poll = create(:poll, :future, name: "Movies") - visit admin_poll_path(poll) - click_link "Create question" + visit admin_poll_path(poll) + click_link "Create question" - expect(page).to have_content("Create question to poll Movies") - expect(page).to have_selector("input[id='poll_question_poll_id'][value='#{poll.id}']", - visible: :hidden) - fill_in "Question", with: title + expect(page).to have_content("Create question to poll Movies") + expect(page).to have_selector("input[id='poll_question_poll_id'][value='#{poll.id}']", + visible: :hidden) - click_button "Save" + fill_in "Question", with: "Star Wars: Episode IV - A New Hope" + click_button "Save" - expect(page).to have_content(title) + expect(page).to have_content "Star Wars: Episode IV - A New Hope" + end + + scenario "Is not possible for an already started poll" do + visit admin_poll_path(create(:poll)) + + expect(page).not_to have_link "Create question" + end end scenario "Create from proposal" do - create(:poll, name: "Proposals") + create(:poll, :future, name: "Proposals") proposal = create(:proposal) visit admin_proposal_path(proposal) @@ -84,7 +91,7 @@ describe "Admin poll questions", :admin do expect(page).to have_current_path(new_admin_question_path, ignore_query: true) expect(page).to have_field("Question", with: proposal.title) - select "Proposals", from: "poll_question_poll_id" + select "Proposals", from: "Poll" click_button "Save" @@ -92,7 +99,7 @@ describe "Admin poll questions", :admin do end scenario "Create from successful proposal" do - create(:poll, name: "Proposals") + create(:poll, :future, name: "Proposals") proposal = create(:proposal, :successful) visit admin_proposal_path(proposal) @@ -103,7 +110,7 @@ describe "Admin poll questions", :admin do expect(page).to have_current_path(new_admin_question_path, ignore_query: true) expect(page).to have_field("Question", with: proposal.title) - select "Proposals", from: "poll_question_poll_id" + select "Proposals", from: "Poll" click_button "Save" @@ -115,29 +122,29 @@ describe "Admin poll questions", :admin do end scenario "Update" do - poll = create(:poll) - question1 = create(:poll_question, poll: poll) + poll = create(:poll, :future) + question = create(:poll_question, poll: poll) + old_title = question.title + new_title = "Vegetables are great and everyone should have one" visit admin_poll_path(poll) - within("#poll_question_#{question1.id}") do + within("#poll_question_#{question.id}") do click_link "Edit" end expect(page).to have_link "Go back", href: admin_poll_path(poll) - old_title = question1.title - new_title = "Potatoes are great and everyone should have one" fill_in "Question", with: new_title click_button "Save" expect(page).to have_content "Changes saved" expect(page).to have_content new_title - expect(page).not_to have_content(old_title) + expect(page).not_to have_content old_title end scenario "Destroy" do - poll = create(:poll) + poll = create(:poll, :future) question1 = create(:poll_question, poll: poll) question2 = create(:poll_question, poll: poll) @@ -156,7 +163,7 @@ describe "Admin poll questions", :admin do context "Poll select box" do scenario "translates the poll name in options" do - poll = create(:poll, name_en: "Name in English", name_es: "Nombre en Español") + poll = create(:poll, :future, name_en: "Name in English", name_es: "Nombre en Español") proposal = create(:proposal) visit admin_proposal_path(proposal) @@ -172,7 +179,7 @@ describe "Admin poll questions", :admin do scenario "uses fallback if name is not translated to current locale", if: Globalize.fallbacks(:fr).reject { |locale| locale.match(/fr/) }.first == :es do - poll = create(:poll, name_en: "Name in English", name_es: "Nombre en Español") + poll = create(:poll, :future, name_en: "Name in English", name_es: "Nombre en Español") proposal = create(:proposal) visit admin_proposal_path(proposal) diff --git a/spec/system/admin/translatable_spec.rb b/spec/system/admin/translatable_spec.rb index 1dab5cbbd..42cbefafd 100644 --- a/spec/system/admin/translatable_spec.rb +++ b/spec/system/admin/translatable_spec.rb @@ -383,7 +383,7 @@ describe "Admin edit translatable records", :admin do end context "Remove a translation with invalid data" do - let(:translatable) { create(:poll_question) } + let(:translatable) { create(:poll_question, poll: create(:poll, :future)) } let(:path) { edit_admin_question_path(translatable) } scenario "Doesn't remove the translation" do