From 855fedc0255aac83538b4e45436d23ba7afa076d Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Wed, 14 Sep 2022 16:16:49 +0200 Subject: [PATCH 01/18] Allow only creation of polls with dates that are not in the past --- app/models/poll.rb | 14 ++++++++++++++ config/locales/en/general.yml | 1 + config/locales/es/general.yml | 1 + db/dev_seeds/polls.rb | 18 +++++++++++++----- spec/factories/polls.rb | 5 +++++ spec/models/poll/poll_spec.rb | 32 ++++++++++++++++++++++++++++++-- 6 files changed, 64 insertions(+), 7 deletions(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index 2acd9cfb0..58ad1b2eb 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -37,6 +37,8 @@ class Poll < ApplicationRecord validates_translation :name, presence: true validate :date_range + validate :start_date_is_not_past_date, on: :create + validate :start_date_change, on: :update validate :only_one_active, unless: :public? accepts_nested_attributes_for :questions, reject_if: :all_blank, allow_destroy: true @@ -143,6 +145,18 @@ class Poll < ApplicationRecord end end + def start_date_is_not_past_date + if starts_at.present? && starts_at < Time.current + errors.add(:starts_at, I18n.t("errors.messages.past_date")) + end + end + + def start_date_change + if will_save_change_to_starts_at? && starts_at < Time.current + errors.add(:starts_at, I18n.t("errors.messages.past_date")) + end + end + def generate_slug? slug.nil? end diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 96b0655c9..ea032e72f 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -137,6 +137,7 @@ en: allowed_file_content_types: "content type must be one of %{types}" user_not_found: User not found invalid_date_range: "Invalid date range" + past_date: "Must not be a past date" form: accept_terms: I agree to the %{policy} and the %{conditions} accept_terms_title: I agree to the Privacy Policy and the Terms and conditions of use diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index ebfbd6749..e1c01bc8d 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -137,6 +137,7 @@ es: allowed_file_content_types: "el tipo de contenido debe ser uno de los siguientes: %{types}" user_not_found: Usuario no encontrado invalid_date_range: "El rango de fechas no es válido" + past_date: "No puede ser una fecha pasada" form: accept_terms: Acepto la %{policy} y las %{conditions} accept_terms_title: Acepto la Política de privacidad y las Condiciones de uso diff --git a/db/dev_seeds/polls.rb b/db/dev_seeds/polls.rb index 6ed23e601..a4bd79fba 100644 --- a/db/dev_seeds/polls.rb +++ b/db/dev_seeds/polls.rb @@ -2,30 +2,38 @@ require_dependency "poll/answer" require_dependency "poll/question/answer" section "Creating polls" do - Poll.create!(name: I18n.t("seeds.polls.current_poll"), + def create_poll!(attributes) + poll = Poll.create!(attributes.merge(starts_at: 1.day.from_now, ends_at: 2.days.from_now)) + poll.update_columns( + starts_at: attributes[:starts_at].beginning_of_minute, + ends_at: attributes[:ends_at].beginning_of_minute + ) + end + + create_poll!(name: I18n.t("seeds.polls.current_poll"), slug: I18n.t("seeds.polls.current_poll").parameterize, starts_at: 7.days.ago, ends_at: 7.days.from_now, geozone_restricted: false) - Poll.create!(name: I18n.t("seeds.polls.current_poll_geozone_restricted"), + create_poll!(name: I18n.t("seeds.polls.current_poll_geozone_restricted"), slug: I18n.t("seeds.polls.current_poll_geozone_restricted").parameterize, starts_at: 5.days.ago, ends_at: 5.days.from_now, geozone_restricted: true, geozones: Geozone.sample(3)) - Poll.create!(name: I18n.t("seeds.polls.recounting_poll"), + create_poll!(name: I18n.t("seeds.polls.recounting_poll"), slug: I18n.t("seeds.polls.recounting_poll").parameterize, starts_at: 15.days.ago, ends_at: 2.days.ago) - Poll.create!(name: I18n.t("seeds.polls.expired_poll_without_stats"), + create_poll!(name: I18n.t("seeds.polls.expired_poll_without_stats"), slug: I18n.t("seeds.polls.expired_poll_without_stats").parameterize, starts_at: 2.months.ago, ends_at: 1.month.ago) - Poll.create!(name: I18n.t("seeds.polls.expired_poll_with_stats"), + create_poll!(name: I18n.t("seeds.polls.expired_poll_with_stats"), slug: I18n.t("seeds.polls.expired_poll_with_stats").parameterize, starts_at: 2.months.ago, ends_at: 1.month.ago, diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 7820532f7..707a1af26 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -6,6 +6,7 @@ FactoryBot.define do starts_at { 1.month.ago } ends_at { 1.month.from_now } + to_create { |poll| poll.save(validate: false) } trait :expired do starts_at { 1.month.ago } @@ -17,6 +18,10 @@ FactoryBot.define do ends_at { 2.months.ago } end + trait :future do + starts_at { 1.day.from_now } + end + trait :published do published { true } end diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index 7d48c502f..8b2224364 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" describe Poll do - let(:poll) { build(:poll) } + let(:poll) { build(:poll, :future) } describe "Concerns" do it_behaves_like "notifiable" @@ -22,7 +22,9 @@ describe Poll do it "is not valid without a start date" do poll.starts_at = nil + expect(poll).not_to be_valid + expect(poll.errors[:starts_at]).to eq ["Invalid date range"] end it "is not valid without an end date" do @@ -35,11 +37,37 @@ describe Poll do poll.ends_at = 2.months.ago expect(poll).not_to be_valid end + + it "is valid if start date is greater than current time" do + poll.starts_at = 1.minute.from_now + expect(poll).to be_valid + end + + it "is not valid if start date is a past date" do + poll.starts_at = 1.minute.ago + + expect(poll).not_to be_valid + expect(poll.errors[:starts_at]).to eq ["Must not be a past date"] + end + + context "persisted poll" do + let(:poll) { create(:poll, :future) } + + it "is valid if the start date changes to a future date" do + poll.starts_at = 1.minute.from_now + expect(poll).to be_valid + end + + it "is not valid if the start date changes to a past date" do + poll.starts_at = 1.minute.ago + expect(poll).not_to be_valid + end + end end describe "proposal polls specific validations" do let(:proposal) { create(:proposal) } - let(:poll) { build(:poll, related: proposal) } + let(:poll) { build(:poll, :future, related: proposal) } it "is valid when overlapping but different proposals" do other_proposal = create(:proposal) From b41fbfa52d5b4e71e49e749d551a04807b161032 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 14 Sep 2022 16:17:56 +0200 Subject: [PATCH 02/18] Avoid updating ddbb after visiting a page in tests The extra check to see the voter count has increased was redundant; we already check the request has finished inside the `vote_for_poll_via_web` method and we check all three voters are created in the results table. Updating the poll so it's in the past after starting the browser might result in database inconsistencies while running the tests, so we're using `travel_to` instead. --- spec/system/polls/results_spec.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/spec/system/polls/results_spec.rb b/spec/system/polls/results_spec.rb index 057519922..2fbfe82a1 100644 --- a/spec/system/polls/results_spec.rb +++ b/spec/system/polls/results_spec.rb @@ -19,22 +19,19 @@ describe "Poll Results" do login_as user1 vote_for_poll_via_web(poll, question1, "Yes") vote_for_poll_via_web(poll, question2, "Blue") - expect(Poll::Voter.count).to eq(1) logout login_as user2 vote_for_poll_via_web(poll, question1, "Yes") vote_for_poll_via_web(poll, question2, "Green") - expect(Poll::Voter.count).to eq(2) logout login_as user3 vote_for_poll_via_web(poll, question1, "No") vote_for_poll_via_web(poll, question2, "Yellow") - expect(Poll::Voter.count).to eq(3) logout - poll.update!(ends_at: 1.day.ago) + travel_to(poll.ends_at + 1.day) visit results_poll_path(poll) From a774456b51d1c6a7360e339d5d365ae7ece8c9bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 16 Sep 2022 16:50:15 +0200 Subject: [PATCH 03/18] Simplify test to edit a poll Instead of having to add `beginning_of_minute` to deal with an issue with Capybara filling datetime fields as mentioned in commit 5a0fde4048, we can travel to the beginning of the minute so we don't have to take the seconds into account. --- spec/system/admin/poll/polls_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/poll/polls_spec.rb b/spec/system/admin/poll/polls_spec.rb index 23f023e1f..b9f6df575 100644 --- a/spec/system/admin/poll/polls_spec.rb +++ b/spec/system/admin/poll/polls_spec.rb @@ -75,8 +75,8 @@ describe "Admin polls", :admin do end scenario "Edit" do - travel_to(Time.zone.local(2015, 7, 15, 13, 32, 13)) - poll = create(:poll, :with_image, ends_at: 1.month.from_now.beginning_of_minute) + travel_to(Time.zone.local(2015, 7, 15, 13, 32, 00)) + poll = create(:poll, :with_image, ends_at: 1.month.from_now) visit admin_poll_path(poll) click_link "Edit poll" From 471096c698668a6e39538af9e006d2e3bc46dc6a Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Wed, 14 Sep 2022 16:40:27 +0200 Subject: [PATCH 04/18] Disallow to modify the start date for an already started poll We need to update a couple of tests because a poll is created in the tests with a timestamp that includes nanoseconds and in the form to edit the time of the poll the nanoseconds are not sent, meaning it was detected as a change. --- app/models/poll.rb | 8 ++++++-- config/locales/en/general.yml | 2 ++ config/locales/es/general.yml | 2 ++ spec/models/poll/poll_spec.rb | 7 +++++++ spec/system/admin/poll/polls_spec.rb | 2 +- spec/system/admin/translatable_spec.rb | 2 +- 6 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index 58ad1b2eb..9087b64d0 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -152,8 +152,12 @@ class Poll < ApplicationRecord end def start_date_change - if will_save_change_to_starts_at? && starts_at < Time.current - errors.add(:starts_at, I18n.t("errors.messages.past_date")) + if will_save_change_to_starts_at? + if starts_at_in_database < Time.current + errors.add(:starts_at, I18n.t("errors.messages.cannot_change_date.poll_started")) + elsif starts_at < Time.current + errors.add(:starts_at, I18n.t("errors.messages.past_date")) + end end end diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index ea032e72f..122175846 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -138,6 +138,8 @@ en: user_not_found: User not found invalid_date_range: "Invalid date range" past_date: "Must not be a past date" + cannot_change_date: + poll_started: "Cannot be changed if voting has already started" form: accept_terms: I agree to the %{policy} and the %{conditions} accept_terms_title: I agree to the Privacy Policy and the Terms and conditions of use diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index e1c01bc8d..b20e7a22b 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -138,6 +138,8 @@ es: user_not_found: Usuario no encontrado invalid_date_range: "El rango de fechas no es válido" past_date: "No puede ser una fecha pasada" + cannot_change_date: + poll_started: "No puede ser cambiada si la votación ha comenzado" form: accept_terms: Acepto la %{policy} y las %{conditions} accept_terms_title: Acepto la Política de privacidad y las Condiciones de uso diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index 8b2224364..daa30aca1 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -62,6 +62,13 @@ describe Poll do poll.starts_at = 1.minute.ago expect(poll).not_to be_valid end + + it "is not valid if changing the start date for an already started poll" do + poll = create(:poll, starts_at: 10.days.ago) + + poll.starts_at = 10.days.from_now + expect(poll).not_to be_valid + end end end diff --git a/spec/system/admin/poll/polls_spec.rb b/spec/system/admin/poll/polls_spec.rb index b9f6df575..202c6b394 100644 --- a/spec/system/admin/poll/polls_spec.rb +++ b/spec/system/admin/poll/polls_spec.rb @@ -553,7 +553,7 @@ describe "Admin polls", :admin do end scenario "edit poll with sdg related list" do - poll = create(:poll, name: "Upcoming poll with SDG related content") + poll = create(:poll, :future, name: "Upcoming poll with SDG related content") poll.sdg_goals = [SDG::Goal[1], SDG::Goal[17]] visit edit_admin_poll_path(poll) diff --git a/spec/system/admin/translatable_spec.rb b/spec/system/admin/translatable_spec.rb index 09602ae16..1dab5cbbd 100644 --- a/spec/system/admin/translatable_spec.rb +++ b/spec/system/admin/translatable_spec.rb @@ -242,7 +242,7 @@ describe "Admin edit translatable records", :admin do end context "Change value of a translated field to blank" do - let(:translatable) { create(:poll) } + let(:translatable) { create(:poll, :future) } let(:path) { edit_admin_poll_path(translatable) } scenario "Updates the field to a blank value" do From 8b5c315eb0b389de1d736a3c6c486cbc109f15e3 Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Wed, 14 Sep 2022 17:54:49 +0200 Subject: [PATCH 05/18] Allow to modify the end date as long as it is not to the past --- app/models/poll.rb | 7 +++++++ spec/models/poll/poll_spec.rb | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/models/poll.rb b/app/models/poll.rb index 9087b64d0..abaaf5d2d 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -39,6 +39,7 @@ class Poll < ApplicationRecord validate :date_range validate :start_date_is_not_past_date, on: :create validate :start_date_change, on: :update + validate :end_date_is_not_past_date, on: :update validate :only_one_active, unless: :public? accepts_nested_attributes_for :questions, reject_if: :all_blank, allow_destroy: true @@ -161,6 +162,12 @@ class Poll < ApplicationRecord end end + def end_date_is_not_past_date + if will_save_change_to_ends_at? && ends_at < Time.current + errors.add(:ends_at, I18n.t("errors.messages.past_date")) + end + end + def generate_slug? slug.nil? end diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index daa30aca1..ca79d47ff 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -69,6 +69,18 @@ describe Poll do poll.starts_at = 10.days.from_now expect(poll).not_to be_valid end + + it "is valid if changing the end date to a future date" do + poll.ends_at = 1.day.from_now + expect(poll).to be_valid + end + + it "is not valid if changing the end date to a past date" do + poll = create(:poll, starts_at: 10.days.ago, ends_at: 10.days.from_now) + + poll.ends_at = 1.day.ago + expect(poll).not_to be_valid + end end end From ecd22131f18f752edc4d9969bc0c29fbdcdbdac5 Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Wed, 14 Sep 2022 18:06:34 +0200 Subject: [PATCH 06/18] Disallow to modify the end date for an ended poll --- app/models/poll.rb | 7 +++++++ config/locales/en/general.yml | 1 + config/locales/es/general.yml | 1 + spec/models/poll/poll_spec.rb | 16 +++++++++++++++- 4 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index abaaf5d2d..eafa7d14c 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -40,6 +40,7 @@ class Poll < ApplicationRecord validate :start_date_is_not_past_date, on: :create validate :start_date_change, on: :update validate :end_date_is_not_past_date, on: :update + validate :end_date_change, on: :update validate :only_one_active, unless: :public? accepts_nested_attributes_for :questions, reject_if: :all_blank, allow_destroy: true @@ -168,6 +169,12 @@ class Poll < ApplicationRecord end end + def end_date_change + if will_save_change_to_ends_at? && ends_at_in_database < Time.current + errors.add(:ends_at, I18n.t("errors.messages.cannot_change_date.poll_ended")) + end + end + def generate_slug? slug.nil? end diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 122175846..4a70c1a3e 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -140,6 +140,7 @@ en: past_date: "Must not be a past date" cannot_change_date: poll_started: "Cannot be changed if voting has already started" + poll_ended: "Cannot be changed if voting has already ended" form: accept_terms: I agree to the %{policy} and the %{conditions} accept_terms_title: I agree to the Privacy Policy and the Terms and conditions of use diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index b20e7a22b..d731f4bd4 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -140,6 +140,7 @@ es: past_date: "No puede ser una fecha pasada" cannot_change_date: poll_started: "No puede ser cambiada si la votación ha comenzado" + poll_ended: "No puede ser cambiada si la votación ha acabado" form: accept_terms: Acepto la %{policy} y las %{conditions} accept_terms_title: Acepto la Política de privacidad y las Condiciones de uso diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index ca79d47ff..00f97602e 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -70,7 +70,7 @@ describe Poll do expect(poll).not_to be_valid end - it "is valid if changing the end date to a future date" do + it "is valid if changing the end date for a non-expired poll to a future date" do poll.ends_at = 1.day.from_now expect(poll).to be_valid end @@ -81,6 +81,20 @@ describe Poll do poll.ends_at = 1.day.ago expect(poll).not_to be_valid end + + it "is valid if the past end date is the same as it was" do + poll = create(:poll, starts_at: 3.days.ago, ends_at: 2.days.ago) + poll.ends_at = poll.ends_at + + expect(poll).to be_valid + end + + it "is not valid if changing the end date for an expired poll" do + poll = create(:poll, :expired) + + poll.ends_at = 1.day.from_now + expect(poll).not_to be_valid + end end end From d561a295e74063b035c1e40bf0b23b91ff0acd23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 15 Sep 2022 15:24:21 +0200 Subject: [PATCH 07/18] Move question form view to component --- .../poll/questions/form_component.html.erb | 40 ++++++++++++++++++ .../admin/poll/questions/form_component.rb | 11 +++++ app/views/admin/poll/questions/_form.html.erb | 41 +------------------ 3 files changed, 52 insertions(+), 40 deletions(-) create mode 100644 app/components/admin/poll/questions/form_component.html.erb create mode 100644 app/components/admin/poll/questions/form_component.rb diff --git a/app/components/admin/poll/questions/form_component.html.erb b/app/components/admin/poll/questions/form_component.html.erb new file mode 100644 index 000000000..09c9ba9ad --- /dev/null +++ b/app/components/admin/poll/questions/form_component.html.erb @@ -0,0 +1,40 @@ +<%= render "shared/globalize_locales", resource: question %> + +<%= translatable_form_for(question, url: url) do |f| %> + + <%= render "shared/errors", resource: question %> + + <%= f.hidden_field :proposal_id %> + +
+
+ <% if poll.present? %> + <%= f.hidden_field :poll_id, value: poll.id %> + <% elsif question.poll.present? %> + <%= 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") %> +
+ <% end %> +
+
+ +
+ <%= f.translatable_fields do |translations_form| %> +
+ <%= translations_form.text_field :title %> +
+ <% end %> +
+ +
+
+ <%= f.submit(class: "button success expanded", value: t("shared.save")) %> +
+
+ +<% end %> diff --git a/app/components/admin/poll/questions/form_component.rb b/app/components/admin/poll/questions/form_component.rb new file mode 100644 index 000000000..25507c284 --- /dev/null +++ b/app/components/admin/poll/questions/form_component.rb @@ -0,0 +1,11 @@ +class Admin::Poll::Questions::FormComponent < ApplicationComponent + include TranslatableFormHelper + include GlobalizeHelper + attr_reader :poll, :question, :url + + def initialize(poll, question, url:) + @poll = poll + @question = question + @url = url + end +end diff --git a/app/views/admin/poll/questions/_form.html.erb b/app/views/admin/poll/questions/_form.html.erb index 5679830e4..fb792c3de 100644 --- a/app/views/admin/poll/questions/_form.html.erb +++ b/app/views/admin/poll/questions/_form.html.erb @@ -1,40 +1 @@ -<%= render "shared/globalize_locales", resource: @question %> - -<%= translatable_form_for(@question, url: form_url) do |f| %> - - <%= render "shared/errors", resource: @question %> - - <%= f.hidden_field :proposal_id %> - -
-
- <% if @poll.present? %> - <%= f.hidden_field :poll_id, value: @poll.id %> - <% elsif @question.poll.present? %> - <%= 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") %> -
- <% end %> -
-
- -
- <%= f.translatable_fields do |translations_form| %> -
- <%= translations_form.text_field :title %> -
- <% end %> -
- -
-
- <%= f.submit(class: "button success expanded", value: t("shared.save")) %> -
-
- -<% end %> +<%= render Admin::Poll::Questions::FormComponent.new(@poll, @question, url: form_url) %> From 4c8be42ea1122edc674de9fd1b8fa9758fbb8d69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 15 Sep 2022 16:39:29 +0200 Subject: [PATCH 08/18] Simplify new question form In this form, the only case where `poll` might be present without `question.poll` being present to is going to be the `new` action. We can assign the poll in the `new` action and get rid of the `poll` variable in the form. --- app/components/admin/poll/questions/form_component.html.erb | 4 +--- app/components/admin/poll/questions/form_component.rb | 5 ++--- app/controllers/admin/poll/questions_controller.rb | 1 + app/views/admin/poll/questions/_form.html.erb | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/components/admin/poll/questions/form_component.html.erb b/app/components/admin/poll/questions/form_component.html.erb index 09c9ba9ad..34193725f 100644 --- a/app/components/admin/poll/questions/form_component.html.erb +++ b/app/components/admin/poll/questions/form_component.html.erb @@ -8,9 +8,7 @@
- <% if poll.present? %> - <%= f.hidden_field :poll_id, value: poll.id %> - <% elsif question.poll.present? %> + <% if question.poll.present? %> <%= f.hidden_field :poll_id, value: question.poll.id %> <% else %>
diff --git a/app/components/admin/poll/questions/form_component.rb b/app/components/admin/poll/questions/form_component.rb index 25507c284..4930c984e 100644 --- a/app/components/admin/poll/questions/form_component.rb +++ b/app/components/admin/poll/questions/form_component.rb @@ -1,10 +1,9 @@ class Admin::Poll::Questions::FormComponent < ApplicationComponent include TranslatableFormHelper include GlobalizeHelper - attr_reader :poll, :question, :url + attr_reader :question, :url - def initialize(poll, question, url:) - @poll = poll + def initialize(question, url:) @question = question @url = url end diff --git a/app/controllers/admin/poll/questions_controller.rb b/app/controllers/admin/poll/questions_controller.rb index 598e664e7..ea133c65f 100644 --- a/app/controllers/admin/poll/questions_controller.rb +++ b/app/controllers/admin/poll/questions_controller.rb @@ -16,6 +16,7 @@ class Admin::Poll::QuestionsController < Admin::Poll::BaseController @polls = Poll.all proposal = Proposal.find(params[:proposal_id]) if params[:proposal_id].present? @question.copy_attributes_from_proposal(proposal) + @question.poll = @poll end def create diff --git a/app/views/admin/poll/questions/_form.html.erb b/app/views/admin/poll/questions/_form.html.erb index fb792c3de..c1254b5ca 100644 --- a/app/views/admin/poll/questions/_form.html.erb +++ b/app/views/admin/poll/questions/_form.html.erb @@ -1 +1 @@ -<%= render Admin::Poll::Questions::FormComponent.new(@poll, @question, url: form_url) %> +<%= render Admin::Poll::Questions::FormComponent.new(@question, url: form_url) %> From d499a6944e7f0d886b51edc4ed2aec445d2dbec0 Mon Sep 17 00:00:00 2001 From: taitus Date: Mon, 19 Sep 2022 14:45:15 +0200 Subject: [PATCH 09/18] Fix typos in success messages --- config/locales/en/admin.yml | 10 +++++----- config/locales/en/responders.yml | 4 ++-- spec/system/admin/budget_investments_spec.rb | 18 +++++++++--------- .../answers/documents/documents_spec.rb | 2 +- spec/system/admin/settings_spec.rb | 4 ++-- .../admin/site_customization/documents_spec.rb | 4 ++-- spec/system/admin/system_emails_spec.rb | 2 +- spec/system/budgets/investments_spec.rb | 4 ++-- 8 files changed, 24 insertions(+), 24 deletions(-) diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index cfc5bf653..2663a3436 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -528,10 +528,10 @@ en: size: "Size" url: "URL" create: - success_notice: "Document uploaded succesfully" + success_notice: "Document uploaded successfully" unable_notice: "Invalid document" destroy: - success_notice: "Document deleted succesfully" + success_notice: "Document deleted successfully" hidden_users: index: filter: Filter @@ -894,7 +894,7 @@ en: pending_to_be_sent: This is the content pending to be sent moderate_pending: Moderate notification send send_pending: Send pending - send_pending_notification: Pending notifications sent succesfully + send_pending_notification: Pending notifications sent successfully proposal_notification_digest: title: "Proposal notification digest" description: "Gathers all proposal notifications for an user in a single message, to avoid too much emails." @@ -1127,7 +1127,7 @@ en: title: "Create question to poll %{poll}" title_proposal: "Create question" destroy: - notice: "Question deleted succesfully" + notice: "Question deleted successfully" answers: images: add_image: "Add image" @@ -1308,7 +1308,7 @@ en: title: Map configuration help: Here you can customize the way the map is displayed to users. Drag map marker or click anywhere over the map, set desired zoom and click button "Update". flash: - update: Map configuration updated succesfully. + update: Map configuration updated successfully. form: submit: Update how_to_enable: 'To show the map to users you must enable "Proposals and budget investments geolocation" on "Features" tab.' diff --git a/config/locales/en/responders.yml b/config/locales/en/responders.yml index 371cbaf71..be118de2a 100644 --- a/config/locales/en/responders.yml +++ b/config/locales/en/responders.yml @@ -26,12 +26,12 @@ en: poll_booth: "Booth updated successfully." active_poll: "Polls description updated successfully." proposal: "Proposal updated successfully." - budget_investment: "Investment project updated succesfully." + budget_investment: "Investment project updated successfully." topic: "Topic updated successfully." valuator_group: "Valuator group updated successfully" translation: "Translation updated successfully" destroy: - budget_investment: "Investment project deleted succesfully." + budget_investment: "Investment project deleted successfully." topic: "Topic deleted successfully." poll_question_answer_video: "Answer video deleted successfully." valuator_group: "Valuator group deleted successfully" diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index 82af1feb0..c0de69ff7 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1069,7 +1069,7 @@ describe "Admin budget investments", :admin do select "Marta desc (marta@admins.org)", from: "budget_investment[administrator_id]" click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." expect(page).to have_content "Assigned administrator: Marta" end @@ -1102,7 +1102,7 @@ describe "Admin budget investments", :admin do click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." within("#assigned_valuators") do expect(page).to have_content("Valentina (v1@valuators.org)") @@ -1127,7 +1127,7 @@ describe "Admin budget investments", :admin do click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." within("#assigned_valuator_groups") do expect(page).to have_content("Health") @@ -1151,7 +1151,7 @@ describe "Admin budget investments", :admin do click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." within("#assigned_valuator_groups") { expect(page).to have_content("Health") } within("#assigned_valuators") do @@ -1173,7 +1173,7 @@ describe "Admin budget investments", :admin do click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." within "#tags" do expect(page).to have_content "Education" @@ -1190,7 +1190,7 @@ describe "Admin budget investments", :admin do fill_in "budget_investment_valuation_tag_list", with: "Refugees, Solidarity" click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." within "#tags" do expect(page).to have_content "Refugees" @@ -1216,7 +1216,7 @@ describe "Admin budget investments", :admin do fill_in "budget_investment_valuation_tag_list", with: "Education, Environment" click_button "Update" - expect(page).to have_content "Investment project updated succesfully" + expect(page).to have_content "Investment project updated successfully" visit admin_budget_budget_investment_path(budget_investment.budget, budget_investment) @@ -1244,7 +1244,7 @@ describe "Admin budget investments", :admin do fill_in "budget_investment_valuation_tag_list", with: "Refugees, Solidarity" click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." visit budget_investment_path(budget_investment.budget, budget_investment) expect(page).to have_content "Park" @@ -1314,7 +1314,7 @@ describe "Admin budget investments", :admin do click_button "Update" - expect(page).to have_content "Investment project updated succesfully." + expect(page).to have_content "Investment project updated successfully." expect(page).to have_content("Milestone Tags: tag1, tag2") end end diff --git a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb index 1ab90af53..8a3b3e356 100644 --- a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb +++ b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb @@ -30,7 +30,7 @@ describe "Documents", :admin do documentable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.pdf")) click_button "Save" - expect(page).to have_content "Document uploaded succesfully" + expect(page).to have_content "Document uploaded successfully" expect(page).to have_link "clippy.pdf" end diff --git a/spec/system/admin/settings_spec.rb b/spec/system/admin/settings_spec.rb index d4af85d41..f5da37d3e 100644 --- a/spec/system/admin/settings_spec.rb +++ b/spec/system/admin/settings_spec.rb @@ -81,7 +81,7 @@ describe "Admin settings", :admin do click_on "Update" end - expect(page).to have_content "Map configuration updated succesfully" + expect(page).to have_content "Map configuration updated successfully" end scenario "Should display marker by default" do @@ -104,7 +104,7 @@ describe "Admin settings", :admin do end expect(find("#latitude", visible: :hidden).value).not_to eq "51.48" - expect(page).to have_content "Map configuration updated succesfully" + expect(page).to have_content "Map configuration updated successfully" end end diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index 5e86779d6..5c284c525 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -57,7 +57,7 @@ describe "Documents", :admin do attach_file("document_attachment", file_fixture("logo.pdf")) click_button "Upload" - expect(page).to have_content "Document uploaded succesfully" + expect(page).to have_content "Document uploaded successfully" expect(page).to have_link "logo.pdf" end @@ -80,7 +80,7 @@ describe "Documents", :admin do end end - expect(page).to have_content "Document deleted succesfully" + expect(page).to have_content "Document deleted successfully" expect(page).not_to have_content document.title end end diff --git a/spec/system/admin/system_emails_spec.rb b/spec/system/admin/system_emails_spec.rb index 425c5b4d0..cce079532 100644 --- a/spec/system/admin/system_emails_spec.rb +++ b/spec/system/admin/system_emails_spec.rb @@ -356,7 +356,7 @@ describe "System Emails" do expect(email).to deliver_to(voter) expect(email).to have_body_text(proposal_notification.body) - expect(page).to have_content("Pending notifications sent succesfully") + expect(page).to have_content("Pending notifications sent successfully") end end end diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb index 2395be61c..ba3a20e22 100644 --- a/spec/system/budgets/investments_spec.rb +++ b/spec/system/budgets/investments_spec.rb @@ -702,7 +702,7 @@ describe "Budget Investments" do click_button "Update Investment" - expect(page).to have_content "Investment project updated succesfully" + expect(page).to have_content "Investment project updated successfully" expect(page).to have_content "Park improvements" end @@ -1157,7 +1157,7 @@ describe "Budget Investments" do accept_confirm { click_link("Delete") } end - expect(page).to have_content "Investment project deleted succesfully" + expect(page).to have_content "Investment project deleted successfully" visit user_path(user, tab: :budget_investments) From 8a26954bc5f870faf724779123a4bf53e86c424f Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Thu, 15 Sep 2022 16:36:50 +0200 Subject: [PATCH 10/18] 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 From 7a812a82e27caf73ad415820616f83dff040d94f Mon Sep 17 00:00:00 2001 From: taitus Date: Thu, 15 Sep 2022 16:36:50 +0200 Subject: [PATCH 11/18] Remove redundant expectations in questions test --- spec/system/admin/poll/questions_spec.rb | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/spec/system/admin/poll/questions_spec.rb b/spec/system/admin/poll/questions_spec.rb index 2297678b8..99c81a34c 100644 --- a/spec/system/admin/poll/questions_spec.rb +++ b/spec/system/admin/poll/questions_spec.rb @@ -4,11 +4,9 @@ describe "Admin poll questions", :admin do scenario "Index" do 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) - question3 = create(:poll_question, poll: poll3, proposal: proposal) + question2 = create(:poll_question, poll: poll2, proposal: proposal) visit admin_poll_path(poll1) expect(page).to have_content(poll1.name) @@ -25,17 +23,7 @@ describe "Admin poll questions", :admin do within("#poll_question_#{question2.id}") do expect(page).to have_content question2.title - expect(page).to have_link "Edit answers" - expect(page).to have_link "Edit" - expect(page).to have_button "Delete" - end - - visit admin_poll_path(poll3) - expect(page).to have_content(poll3.name) - - within("#poll_question_#{question3.id}") do - expect(page).to have_content question3.title - expect(page).to have_link "(See proposal)", href: proposal_path(question3.proposal) + expect(page).to have_link "(See proposal)", href: proposal_path(question2.proposal) expect(page).to have_link "Edit answers" expect(page).to have_link "Edit" expect(page).to have_button "Delete" From 3a6e99cb8c2d2b97c991b22b894264d183da1660 Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Sat, 26 Feb 2022 17:41:37 +0700 Subject: [PATCH 12/18] Don't allow changing answers if the poll has started Just like we did with questions. --- .../answers/table_actions_component.html.erb | 1 + .../answers/table_actions_component.rb | 14 +++ app/models/abilities/administrator.rb | 5 +- app/views/admin/poll/questions/show.html.erb | 12 ++- .../answers/table_actions_component_spec.rb | 21 +++++ .../poll/questions/answers_controller_spec.rb | 88 +++++++++++++++++++ spec/models/abilities/administrator_spec.rb | 12 ++- .../poll/questions/answers/answers_spec.rb | 57 +++++++----- spec/system/admin/translatable_spec.rb | 2 +- 9 files changed, 183 insertions(+), 29 deletions(-) create mode 100644 app/components/admin/poll/questions/answers/table_actions_component.html.erb create mode 100644 app/components/admin/poll/questions/answers/table_actions_component.rb create mode 100644 spec/components/admin/poll/questions/answers/table_actions_component_spec.rb create mode 100644 spec/controllers/admin/poll/questions/answers_controller_spec.rb diff --git a/app/components/admin/poll/questions/answers/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/table_actions_component.html.erb new file mode 100644 index 000000000..a9fc5cd8a --- /dev/null +++ b/app/components/admin/poll/questions/answers/table_actions_component.html.erb @@ -0,0 +1 @@ +<%= render Admin::TableActionsComponent.new(answer, actions: actions) %> diff --git a/app/components/admin/poll/questions/answers/table_actions_component.rb b/app/components/admin/poll/questions/answers/table_actions_component.rb new file mode 100644 index 000000000..fcec11fce --- /dev/null +++ b/app/components/admin/poll/questions/answers/table_actions_component.rb @@ -0,0 +1,14 @@ +class Admin::Poll::Questions::Answers::TableActionsComponent < ApplicationComponent + attr_reader :answer + delegate :can?, to: :helpers + + def initialize(answer) + @answer = answer + end + + private + + def actions + [:edit].select { |action| can?(action, answer) } + end +end diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 437b678a1..96f4b581a 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -95,7 +95,10 @@ module Abilities can [:update, :destroy], Poll::Question do |question| !question.poll.started? end - can :manage, Poll::Question::Answer + can [:read, :order_answers], Poll::Question::Answer + can [:create, :update], Poll::Question::Answer do |answer| + can?(:update, answer.question) + end can :manage, Poll::Question::Answer::Video can [:create, :destroy], Image do |image| image.imageable_type == "Poll::Question::Answer" diff --git a/app/views/admin/poll/questions/show.html.erb b/app/views/admin/poll/questions/show.html.erb index bfeff9f6d..ec4e9cecc 100644 --- a/app/views/admin/poll/questions/show.html.erb +++ b/app/views/admin/poll/questions/show.html.erb @@ -32,8 +32,14 @@
- <%= link_to t("admin.questions.show.add_answer"), new_admin_question_answer_path(@question), - class: "button float-right" %> + <% if can?(:create, Poll::Question::Answer.new(question: @question)) %> + <%= link_to t("admin.questions.show.add_answer"), new_admin_question_answer_path(@question), + class: "button float-right" %> + <% else %> +
+ <%= t("admin.questions.no_edit") %> +
+ <% end %>
@@ -73,7 +79,7 @@ admin_answer_videos_path(answer) %> <% end %> diff --git a/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb b/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb new file mode 100644 index 000000000..3ed96b8da --- /dev/null +++ b/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb @@ -0,0 +1,21 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::TableActionsComponent, controller: Admin::BaseController do + before { sign_in(create(:administrator).user) } + + it "displays the edit action when the poll has not started" do + answer = create(:poll_question_answer, poll: create(:poll, :future)) + + render_inline Admin::Poll::Questions::Answers::TableActionsComponent.new(answer) + + expect(page).to have_link "Edit" + end + + it "does not display the edit action when the poll has started" do + answer = create(:poll_question_answer, poll: create(:poll)) + + render_inline Admin::Poll::Questions::Answers::TableActionsComponent.new(answer) + + expect(page).not_to have_link "Edit" + end +end diff --git a/spec/controllers/admin/poll/questions/answers_controller_spec.rb b/spec/controllers/admin/poll/questions/answers_controller_spec.rb new file mode 100644 index 000000000..6ecccf37b --- /dev/null +++ b/spec/controllers/admin/poll/questions/answers_controller_spec.rb @@ -0,0 +1,88 @@ +require "rails_helper" + +describe Admin::Poll::Questions::AnswersController, :admin do + let(:current_question) { create(:poll_question, poll: create(:poll)) } + let(:future_question) { create(:poll_question, poll: create(:poll, :future)) } + + describe "POST create" do + it "is not possible for an already started poll" do + post :create, params: { + poll_question_answer: { + translations_attributes: { + "0" => { + locale: "en", + title: "Answer from started poll" + } + } + }, + question_id: current_question + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'create' on Answer." + expect(Poll::Question::Answer.count).to eq 0 + end + + it "is possible for a not started poll" do + post :create, params: { + poll_question_answer: { + translations_attributes: { + "0" => { + locale: "en", + title: "Answer from future poll" + } + } + }, + question_id: future_question + } + + expect(response).to redirect_to admin_question_path(future_question) + expect(Poll::Question::Answer.last.title).to eq "Answer from future poll" + expect(Poll::Question::Answer.count).to eq 1 + end + end + + describe "PATCH update" do + it "is not possible for an already started poll" do + current_answer = create(:poll_question_answer, question: current_question, title: "Sample title") + + patch :update, params: { + poll_question_answer: { + translations_attributes: { + "0" => { + locale: "en", + title: "New title", + id: current_answer.translations.first.id + } + } + }, + question_id: current_question, + id: current_answer + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'update' on Answer." + expect(current_answer.reload.title).to eq "Sample title" + end + + it "is possible for a not started poll" do + future_answer = create(:poll_question_answer, question: future_question) + + patch :update, params: { + poll_question_answer: { + translations_attributes: { + "0" => { + locale: "en", + title: "New title", + id: future_answer.translations.first.id + } + } + }, + question_id: future_question, + id: future_answer + } + + expect(response).to redirect_to admin_question_path(future_question) + expect(flash[:notice]).to eq "Changes saved" + expect(future_answer.reload.title).to eq "New title" + end + end +end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index f9a6da74f..dfd1a76f0 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -20,8 +20,9 @@ describe Abilities::Administrator do 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) } + let(:current_poll_question_answer) { create(:poll_question_answer) } + let(:future_poll_question_answer) { create(:poll_question_answer, poll: future_poll) } + let(:answer_image) { build(:image, imageable: current_poll_question_answer) } let(:past_process) { create(:legislation_process, :past) } let(:past_draft_process) { create(:legislation_process, :past, :not_published) } @@ -122,7 +123,12 @@ describe Abilities::Administrator do 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) } + it { should be_able_to(:read, Poll::Question::Answer) } + it { should be_able_to(:order_answers, Poll::Question::Answer) } + it { should be_able_to(:create, future_poll_question_answer) } + it { should be_able_to(:update, future_poll_question_answer) } + it { should_not be_able_to(:create, current_poll_question_answer) } + it { should_not be_able_to(:update, current_poll_question_answer) } it { should be_able_to(:manage, Poll::Question::Answer::Video) } diff --git a/spec/system/admin/poll/questions/answers/answers_spec.rb b/spec/system/admin/poll/questions/answers/answers_spec.rb index 3b12ce770..9e3ac1a4f 100644 --- a/spec/system/admin/poll/questions/answers/answers_spec.rb +++ b/spec/system/admin/poll/questions/answers/answers_spec.rb @@ -1,40 +1,55 @@ require "rails_helper" describe "Answers", :admin do - scenario "Create" do - question = create(:poll_question) + let(:future_poll) { create(:poll, :future) } + let(:current_poll) { create(:poll) } - visit admin_question_path(question) - click_link "Add answer" + describe "Create" do + scenario "Is possible for a not started poll" do + question = create(:poll_question, poll: future_poll) - expect(page).to have_link "Go back", href: admin_question_path(question) + visit admin_question_path(question) + click_link "Add answer" - fill_in "Answer", with: "The answer is always 42" - fill_in_ckeditor "Description", with: "The Hitchhiker's Guide To The Universe" + expect(page).to have_link "Go back", href: admin_question_path(question) - click_button "Save" + fill_in "Answer", with: "The answer is always 42" + fill_in_ckeditor "Description", with: "The Hitchhiker's Guide To The Universe" - expect(page).to have_content "The answer is always 42" - expect(page).to have_content "The Hitchhiker's Guide To The Universe" - end + click_button "Save" - scenario "Create second answer and place after the first one" do - question = create(:poll_question) - create(:poll_question_answer, title: "First", question: question, given_order: 1) + expect(page).to have_content "Answer created successfully" + expect(page).to have_content "The answer is always 42" + expect(page).to have_content "The Hitchhiker's Guide To The Universe" + end - visit admin_question_path(question) - click_link "Add answer" + scenario "Is not possible for an already started poll" do + question = create(:poll_question, poll: current_poll) - fill_in "Answer", with: "Second" - fill_in_ckeditor "Description", with: "Description" + visit admin_question_path(question) - click_button "Save" + expect(page).not_to have_link "Add answer" + expect(page).to have_content "Once the poll has started it will not be possible to create, edit or" + end - expect("First").to appear_before("Second") + scenario "Create second answer and place after the first one" do + question = create(:poll_question, poll: future_poll) + create(:poll_question_answer, title: "First", question: question, given_order: 1) + + visit admin_question_path(question) + click_link "Add answer" + + fill_in "Answer", with: "Second" + fill_in_ckeditor "Description", with: "Description" + + click_button "Save" + + expect("First").to appear_before("Second") + end end scenario "Update" do - question = create(:poll_question) + question = create(:poll_question, poll: future_poll) create(:poll_question_answer, question: question, title: "Answer title", given_order: 2) create(:poll_question_answer, question: question, title: "Another title", given_order: 1) diff --git a/spec/system/admin/translatable_spec.rb b/spec/system/admin/translatable_spec.rb index 42cbefafd..ca81e8e39 100644 --- a/spec/system/admin/translatable_spec.rb +++ b/spec/system/admin/translatable_spec.rb @@ -215,7 +215,7 @@ describe "Admin edit translatable records", :admin do end context "CKEditor fields" do - let(:translatable) { create(:poll_question_answer) } + let(:translatable) { create(:poll_question_answer, poll: create(:poll, :future)) } let(:path) { edit_admin_question_answer_path(translatable.question, translatable) } scenario "Changes the existing translation" do From 14542df0defa5464279c1a63dbbc5f5407d522d1 Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Sat, 26 Feb 2022 17:41:37 +0700 Subject: [PATCH 13/18] Allow to delete answers if the poll has not started yet Deleting answers was not even possible. But it was possible to delete questions. So we implemented the same behavior. --- .../answers/table_actions_component.rb | 2 +- .../poll/questions/answers_controller.rb | 5 +++++ app/models/abilities/administrator.rb | 2 +- config/locales/en/admin.yml | 2 ++ config/locales/es/admin.yml | 2 ++ config/routes/admin.rb | 2 +- .../answers/table_actions_component_spec.rb | 6 ++++-- .../poll/questions/answers_controller_spec.rb | 19 +++++++++++++++++++ spec/models/abilities/administrator_spec.rb | 2 ++ .../poll/questions/answers/answers_spec.rb | 15 +++++++++++++++ 10 files changed, 52 insertions(+), 5 deletions(-) diff --git a/app/components/admin/poll/questions/answers/table_actions_component.rb b/app/components/admin/poll/questions/answers/table_actions_component.rb index fcec11fce..577e099b1 100644 --- a/app/components/admin/poll/questions/answers/table_actions_component.rb +++ b/app/components/admin/poll/questions/answers/table_actions_component.rb @@ -9,6 +9,6 @@ class Admin::Poll::Questions::Answers::TableActionsComponent < ApplicationCompon private def actions - [:edit].select { |action| can?(action, answer) } + [:edit, :destroy].select { |action| can?(action, answer) } end end diff --git a/app/controllers/admin/poll/questions/answers_controller.rb b/app/controllers/admin/poll/questions/answers_controller.rb index c015f1ebe..844f3d494 100644 --- a/app/controllers/admin/poll/questions/answers_controller.rb +++ b/app/controllers/admin/poll/questions/answers_controller.rb @@ -30,6 +30,11 @@ class Admin::Poll::Questions::AnswersController < Admin::Poll::BaseController end end + def destroy + @answer.destroy! + redirect_to admin_question_path(@question), notice: t("admin.answers.destroy.success_notice") + end + def order_answers ::Poll::Question::Answer.order_answers(params[:ordered_list]) head :ok diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 96f4b581a..07f26a649 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -96,7 +96,7 @@ module Abilities !question.poll.started? end can [:read, :order_answers], Poll::Question::Answer - can [:create, :update], Poll::Question::Answer do |answer| + can [:create, :update, :destroy], Poll::Question::Answer do |answer| can?(:update, answer.question) end can :manage, Poll::Question::Answer::Video diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index e1a1bd5cf..3084abb6f 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -1159,6 +1159,8 @@ en: title: New answer edit: title: Edit answer + destroy: + success_notice: "Answer deleted successfully" videos: index: title: Videos diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index acae33b58..3ea4905cc 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -1158,6 +1158,8 @@ es: title: Nueva respuesta edit: title: Editar respuesta + destroy: + success_notice: "Respuesta eliminada correctamente" videos: index: title: Vídeos diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 84d3e6b68..56a2cf77e 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -169,7 +169,7 @@ namespace :admin do end resources :questions, shallow: true do - resources :answers, except: [:index, :show, :destroy], controller: "questions/answers", shallow: false + resources :answers, except: [:index, :show], controller: "questions/answers", shallow: false resources :answers, only: [], controller: "questions/answers" do resources :images, controller: "questions/answers/images" resources :videos, controller: "questions/answers/videos", shallow: false diff --git a/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb b/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb index 3ed96b8da..4e6080a11 100644 --- a/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb +++ b/spec/components/admin/poll/questions/answers/table_actions_component_spec.rb @@ -3,19 +3,21 @@ require "rails_helper" describe Admin::Poll::Questions::Answers::TableActionsComponent, controller: Admin::BaseController do before { sign_in(create(:administrator).user) } - it "displays the edit action when the poll has not started" do + it "displays the edit and destroy actions when the poll has not started" do answer = create(:poll_question_answer, poll: create(:poll, :future)) render_inline Admin::Poll::Questions::Answers::TableActionsComponent.new(answer) expect(page).to have_link "Edit" + expect(page).to have_button "Delete" end - it "does not display the edit action when the poll has started" do + it "does not display the edit and destroy actions when the poll has started" do answer = create(:poll_question_answer, poll: create(:poll)) render_inline Admin::Poll::Questions::Answers::TableActionsComponent.new(answer) expect(page).not_to have_link "Edit" + expect(page).not_to have_button "Delete" end end diff --git a/spec/controllers/admin/poll/questions/answers_controller_spec.rb b/spec/controllers/admin/poll/questions/answers_controller_spec.rb index 6ecccf37b..bdee4c59a 100644 --- a/spec/controllers/admin/poll/questions/answers_controller_spec.rb +++ b/spec/controllers/admin/poll/questions/answers_controller_spec.rb @@ -85,4 +85,23 @@ describe Admin::Poll::Questions::AnswersController, :admin do expect(future_answer.reload.title).to eq "New title" end end + + describe "DELETE destroy" do + it "is not possible for an already started poll" do + current_answer = create(:poll_question_answer, question: current_question) + delete :destroy, params: { question_id: current_question, id: current_answer } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'destroy' on Answer." + expect(Poll::Question::Answer.count).to eq 1 + end + + it "is possible for a not started poll" do + future_answer = create(:poll_question_answer, question: future_question) + delete :destroy, params: { question_id: future_question, id: future_answer } + + expect(response).to redirect_to admin_question_path(future_question) + expect(flash[:notice]).to eq "Answer deleted successfully" + expect(Poll::Question::Answer.count).to eq 0 + end + end end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index dfd1a76f0..853fcee82 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -127,8 +127,10 @@ describe Abilities::Administrator do it { should be_able_to(:order_answers, Poll::Question::Answer) } it { should be_able_to(:create, future_poll_question_answer) } it { should be_able_to(:update, future_poll_question_answer) } + it { should be_able_to(:destroy, future_poll_question_answer) } it { should_not be_able_to(:create, current_poll_question_answer) } it { should_not be_able_to(:update, current_poll_question_answer) } + it { should_not be_able_to(:destroy, current_poll_question_answer) } it { should be_able_to(:manage, Poll::Question::Answer::Video) } diff --git a/spec/system/admin/poll/questions/answers/answers_spec.rb b/spec/system/admin/poll/questions/answers/answers_spec.rb index 9e3ac1a4f..21e53e2b1 100644 --- a/spec/system/admin/poll/questions/answers/answers_spec.rb +++ b/spec/system/admin/poll/questions/answers/answers_spec.rb @@ -71,6 +71,21 @@ describe "Answers", :admin do expect("Another title").to appear_before("New title") end + scenario "Destroy" do + answer = create(:poll_question_answer, poll: future_poll, title: "I'm not useful") + + visit admin_question_path(answer.question) + + within("tr", text: "I'm not useful") do + accept_confirm("Are you sure? This action will delete \"I'm not useful\" and can't be undone.") do + click_button "Delete" + end + end + + expect(page).to have_content "Answer deleted successfully" + expect(page).not_to have_content "I'm not useful" + end + scenario "Reorder" do question = create(:poll_question) create(:poll_question_answer, question: question, title: "First", given_order: 1) From 5fe86264ca36b1d7cd79427a3e083e9846d80f1f Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Fri, 2 Sep 2022 14:29:04 +0200 Subject: [PATCH 14/18] Don't allow to modify answer's videos for started polls Same rules that will apply for the answer itself should apply for the attachments like videos, images, and/or documents. --- .../videos/table_actions_component.html.erb | 1 + .../answers/videos/table_actions_component.rb | 14 +++ app/models/abilities/administrator.rb | 5 +- .../questions/answers/videos/index.html.erb | 14 ++- .../videos/table_actions_component_spec.rb | 23 +++++ .../answers/videos_controller_spec.rb | 87 +++++++++++++++++++ spec/models/abilities/administrator_spec.rb | 9 +- .../questions/answers/videos/videos_spec.rb | 53 +++++++---- 8 files changed, 182 insertions(+), 24 deletions(-) create mode 100644 app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb create mode 100644 app/components/admin/poll/questions/answers/videos/table_actions_component.rb create mode 100644 spec/components/admin/poll/questions/answers/videos/table_actions_component_spec.rb create mode 100644 spec/controllers/admin/poll/questions/answers/videos_controller_spec.rb diff --git a/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb new file mode 100644 index 000000000..f799eae21 --- /dev/null +++ b/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb @@ -0,0 +1 @@ +<%= render Admin::TableActionsComponent.new(video, actions: actions) %> diff --git a/app/components/admin/poll/questions/answers/videos/table_actions_component.rb b/app/components/admin/poll/questions/answers/videos/table_actions_component.rb new file mode 100644 index 000000000..935cd5c80 --- /dev/null +++ b/app/components/admin/poll/questions/answers/videos/table_actions_component.rb @@ -0,0 +1,14 @@ +class Admin::Poll::Questions::Answers::Videos::TableActionsComponent < ApplicationComponent + attr_reader :video + delegate :can?, to: :helpers + + def initialize(video) + @video = video + end + + private + + def actions + [:edit, :destroy].select { |action| can?(action, video) } + end +end diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 07f26a649..ee323ef80 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -99,7 +99,10 @@ module Abilities can [:create, :update, :destroy], Poll::Question::Answer do |answer| can?(:update, answer.question) end - can :manage, Poll::Question::Answer::Video + can :read, Poll::Question::Answer::Video + can [:create, :update, :destroy], Poll::Question::Answer::Video do |video| + can?(:update, video.answer) + end can [:create, :destroy], Image do |image| image.imageable_type == "Poll::Question::Answer" end diff --git a/app/views/admin/poll/questions/answers/videos/index.html.erb b/app/views/admin/poll/questions/answers/videos/index.html.erb index bd436d88a..c9e82498a 100644 --- a/app/views/admin/poll/questions/answers/videos/index.html.erb +++ b/app/views/admin/poll/questions/answers/videos/index.html.erb @@ -6,9 +6,15 @@ <%= t("admin.answers.videos.index.title") %> -<%= link_to t("admin.answers.videos.index.add_video"), - new_admin_answer_video_path(@answer), - class: "button success float-right" %> +<% if can?(:create, Poll::Question::Answer.new(question: @answer.question)) %> + <%= link_to t("admin.answers.videos.index.add_video"), + new_admin_answer_video_path(@answer), + class: "button success float-right" %> +<% else %> +
+ <%= t("admin.questions.no_edit") %> +
+<% end %>
@@ -27,7 +33,7 @@
<% end %> diff --git a/spec/components/admin/poll/questions/answers/videos/table_actions_component_spec.rb b/spec/components/admin/poll/questions/answers/videos/table_actions_component_spec.rb new file mode 100644 index 000000000..e8449103d --- /dev/null +++ b/spec/components/admin/poll/questions/answers/videos/table_actions_component_spec.rb @@ -0,0 +1,23 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::Videos::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 + video = create(:poll_answer_video, poll: create(:poll, :future)) + + render_inline Admin::Poll::Questions::Answers::Videos::TableActionsComponent.new(video) + + 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 + video = create(:poll_answer_video, poll: create(:poll)) + + render_inline Admin::Poll::Questions::Answers::Videos::TableActionsComponent.new(video) + + expect(page).not_to have_link "Edit" + expect(page).not_to have_button "Delete" + end +end diff --git a/spec/controllers/admin/poll/questions/answers/videos_controller_spec.rb b/spec/controllers/admin/poll/questions/answers/videos_controller_spec.rb new file mode 100644 index 000000000..65c5dc1f8 --- /dev/null +++ b/spec/controllers/admin/poll/questions/answers/videos_controller_spec.rb @@ -0,0 +1,87 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::VideosController, :admin do + let(:current_answer) { create(:poll_question_answer, poll: create(:poll)) } + let(:future_answer) { create(:poll_question_answer, poll: create(:poll, :future)) } + + describe "POST create" do + it "is not possible for an already started poll" do + post :create, params: { + poll_question_answer_video: { + title: "Video from started poll", + url: "https://www.youtube.com/watch?v=-JMf43st-1A" + }, + answer_id: current_answer + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'create' on Video." + expect(Poll::Question::Answer::Video.count).to eq 0 + end + + it "is possible for a not started poll" do + post :create, params: { + poll_question_answer_video: { + title: "Video from not started poll", + url: "https://www.youtube.com/watch?v=-JMf43st-1A" + }, + answer_id: future_answer + } + + expect(response).to redirect_to admin_answer_videos_path(future_answer) + expect(flash[:notice]).to eq "Video created successfully" + expect(Poll::Question::Answer::Video.count).to eq 1 + end + end + + describe "PATCH update" do + it "is not possible for an already started poll" do + current_video = create(:poll_answer_video, answer: current_answer, title: "Sample title") + + patch :update, params: { + poll_question_answer_video: { + title: "New title" + }, + id: current_video, + answer_id: current_answer + } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'update' on Video." + expect(current_video.reload.title).to eq "Sample title" + end + + it "is possible for a not started poll" do + future_video = create(:poll_answer_video, answer: future_answer) + + patch :update, params: { + poll_question_answer_video: { + title: "New title" + }, + id: future_video, + answer_id: future_answer + } + + expect(response).to redirect_to admin_answer_videos_path(future_answer) + expect(flash[:notice]).to eq "Changes saved" + expect(future_video.reload.title).to eq "New title" + end + end + + describe "DELETE destroy" do + it "is not possible for an already started poll" do + current_video = create(:poll_answer_video, answer: current_answer) + delete :destroy, params: { answer_id: current_answer, id: current_video } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'destroy' on Video." + expect(Poll::Question::Answer::Video.count).to eq 1 + end + + it "is possible for a not started poll" do + future_video = create(:poll_answer_video, answer: future_answer) + delete :destroy, params: { answer_id: future_answer, id: future_video } + + expect(response).to redirect_to admin_answer_videos_path(future_answer) + expect(flash[:notice]).to eq "Answer video deleted successfully." + expect(Poll::Question::Answer::Video.count).to eq 0 + end + end +end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 853fcee82..289407fd4 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -22,6 +22,8 @@ describe Abilities::Administrator do let(:future_poll_question) { create(:poll_question, poll: future_poll) } let(:current_poll_question_answer) { create(:poll_question_answer) } let(:future_poll_question_answer) { create(:poll_question_answer, poll: future_poll) } + let(:current_poll_answer_video) { create(:poll_answer_video, answer: current_poll_question_answer) } + let(:future_poll_answer_video) { create(:poll_answer_video, answer: future_poll_question_answer) } let(:answer_image) { build(:image, imageable: current_poll_question_answer) } let(:past_process) { create(:legislation_process, :past) } @@ -132,7 +134,12 @@ describe Abilities::Administrator do it { should_not be_able_to(:update, current_poll_question_answer) } it { should_not be_able_to(:destroy, current_poll_question_answer) } - it { should be_able_to(:manage, Poll::Question::Answer::Video) } + it { should be_able_to(:create, future_poll_answer_video) } + it { should be_able_to(:update, future_poll_answer_video) } + it { should be_able_to(:destroy, future_poll_answer_video) } + it { should_not be_able_to(:create, current_poll_answer_video) } + it { should_not be_able_to(:update, current_poll_answer_video) } + it { should_not be_able_to(:destroy, current_poll_answer_video) } it { should be_able_to(:create, answer_image) } it { should be_able_to(:destroy, answer_image) } diff --git a/spec/system/admin/poll/questions/answers/videos/videos_spec.rb b/spec/system/admin/poll/questions/answers/videos/videos_spec.rb index b4d2cdfab..67278fbdd 100644 --- a/spec/system/admin/poll/questions/answers/videos/videos_spec.rb +++ b/spec/system/admin/poll/questions/answers/videos/videos_spec.rb @@ -1,55 +1,72 @@ require "rails_helper" describe "Videos", :admin do - let!(:question) { create(:poll_question) } - let!(:answer) { create(:poll_question_answer, question: question) } + let(:future_poll) { create(:poll, :future) } + let(:current_poll) { create(:poll) } let(:title) { "'Magical' by Junko Ohashi" } let(:url) { "https://www.youtube.com/watch?v=-JMf43st-1A" } - scenario "Create" do - visit admin_question_path(question) + describe "Create" do + scenario "Is possible for a not started poll" do + question = create(:poll_question, poll: future_poll) + answer = create(:poll_question_answer, question: question) - within("#poll_question_answer_#{answer.id}") do - click_link "Video list" + visit admin_question_path(question) + + within("#poll_question_answer_#{answer.id}") do + click_link "Video list" + end + click_link "Add video" + + fill_in "Title", with: title + fill_in "External video", with: url + + click_button "Save" + + expect(page).to have_content "Video created successfully" + expect(page).to have_content title + expect(page).to have_content url end - click_link "Add video" - fill_in "Title", with: title - fill_in "External video", with: url + scenario "Is not possible for an already started poll" do + answer = create(:poll_question_answer, poll: current_poll) - click_button "Save" + visit admin_answer_videos_path(answer) - expect(page).to have_content title - expect(page).to have_content url + expect(page).not_to have_link "Add video" + expect(page).to have_content "Once the poll has started it will not be possible to create, edit or" + end end scenario "Update" do - video = create(:poll_answer_video, answer: answer) + video = create(:poll_answer_video, poll: future_poll) - visit edit_admin_answer_video_path(answer, video) + visit edit_admin_answer_video_path(video.answer, video) - expect(page).to have_link "Go back", href: admin_answer_videos_path(answer) + expect(page).to have_link "Go back", href: admin_answer_videos_path(video.answer) fill_in "Title", with: title fill_in "External video", with: url click_button "Save" + expect(page).to have_content "Changes saved" expect(page).to have_content title expect(page).to have_content url end scenario "Destroy" do - video = create(:poll_answer_video, answer: answer) + video = create(:poll_answer_video, poll: future_poll) - visit admin_answer_videos_path(answer) + visit admin_answer_videos_path(video.answer) - within("#poll_question_answer_video_#{video.id}") do + within("tr", text: video.title) do accept_confirm("Are you sure? This action will delete \"#{video.title}\" and can't be undone.") do click_button "Delete" end end expect(page).to have_content "Answer video deleted successfully." + expect(page).not_to have_content video.title end end From 245594f32b8f4ca8408a6ff61d099df5dcfbe84e Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Fri, 2 Sep 2022 18:46:17 +0200 Subject: [PATCH 15/18] Don't allow to modify answer's images for started polls Note that the `create` action doesn't create an image but updates an answer instead. We're removing the references to `:create` in the abilities since it isn't used. In the future we might change the form to add an image to an answer because it's been broken for ages since it shows all the attached images. --- .../questions/answers/images_controller.rb | 3 +- app/models/abilities/administrator.rb | 4 +- app/models/abilities/common.rb | 4 +- .../questions/answers/images/index.html.erb | 32 +++++--- config/locales/en/admin.yml | 3 + config/locales/es/admin.yml | 3 + .../answers/images_controller_spec.rb | 52 +++++++++++++ spec/factories/polls.rb | 4 + spec/models/abilities/administrator_spec.rb | 7 +- .../questions/answers/images/images_spec.rb | 78 +++++++++++++------ 10 files changed, 151 insertions(+), 39 deletions(-) create mode 100644 spec/controllers/admin/poll/questions/answers/images_controller_spec.rb diff --git a/app/controllers/admin/poll/questions/answers/images_controller.rb b/app/controllers/admin/poll/questions/answers/images_controller.rb index a209b058f..9b60f3f4c 100644 --- a/app/controllers/admin/poll/questions/answers/images_controller.rb +++ b/app/controllers/admin/poll/questions/answers/images_controller.rb @@ -2,6 +2,7 @@ class Admin::Poll::Questions::Answers::ImagesController < Admin::Poll::BaseContr include ImageAttributes load_and_authorize_resource :answer, class: "::Poll::Question::Answer" + load_and_authorize_resource only: [:destroy] def index end @@ -11,6 +12,7 @@ class Admin::Poll::Questions::Answers::ImagesController < Admin::Poll::BaseContr def create @answer.attributes = images_params + authorize! :update, @answer if @answer.save redirect_to admin_answer_images_path(@answer), @@ -21,7 +23,6 @@ class Admin::Poll::Questions::Answers::ImagesController < Admin::Poll::BaseContr end def destroy - @image = ::Image.find(params[:id]) @image.destroy! respond_to do |format| diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index ee323ef80..3c94e802e 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -103,8 +103,8 @@ module Abilities can [:create, :update, :destroy], Poll::Question::Answer::Video do |video| can?(:update, video.answer) end - can [:create, :destroy], Image do |image| - image.imageable_type == "Poll::Question::Answer" + can [:destroy], Image do |image| + image.imageable_type == "Poll::Question::Answer" && can?(:update, image.imageable) end can :manage, SiteCustomization::Page diff --git a/app/models/abilities/common.rb b/app/models/abilities/common.rb index 049a2c8b5..264b98521 100644 --- a/app/models/abilities/common.rb +++ b/app/models/abilities/common.rb @@ -74,7 +74,9 @@ module Abilities document.documentable&.author_id == user.id end - can [:destroy], Image, imageable: { author_id: user.id } + can [:destroy], Image do |image| + image.imageable_type != "Poll::Question::Answer" && image.imageable&.author_id == user.id + end can [:create, :destroy], DirectUpload diff --git a/app/views/admin/poll/questions/answers/images/index.html.erb b/app/views/admin/poll/questions/answers/images/index.html.erb index f031f2b1f..e3f2b3e92 100644 --- a/app/views/admin/poll/questions/answers/images/index.html.erb +++ b/app/views/admin/poll/questions/answers/images/index.html.erb @@ -1,8 +1,20 @@ <%= back_link_to admin_question_path(@answer.question) %> -<%= link_to t("admin.questions.answers.images.add_image"), - new_admin_answer_image_path(@answer), - class: "button hollow float-right" %> +
+ +

+ <%= t("admin.answers.images.index.title") %> +

+ +<% if can?(:update, @answer) %> + <%= link_to t("admin.questions.answers.images.add_image"), + new_admin_answer_image_path(@answer), + class: "button hollow float-right" %> +<% else %> +
+ <%= t("admin.questions.no_edit") %> +
+<% end %>
  • <%= @answer.question.title %>
    • @@ -13,11 +25,13 @@
    <%= render_image(image, :large, true) if image.present? %> - <%= link_to t("images.remove_image"), - admin_image_path(image), - class: "delete float-right", - method: :delete, - remote: true, - data: { confirm: t("admin.actions.confirm_action", action: t("images.remove_image"), name: image.title) } %> + <% if can?(:destroy, image) %> + <%= link_to t("images.remove_image"), + admin_image_path(image), + class: "delete float-right", + method: :delete, + remote: true, + data: { confirm: t("admin.actions.confirm_action", action: t("images.remove_image"), name: image.title) } %> + <% end %>
    <% end %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 3084abb6f..20635ee3a 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -1161,6 +1161,9 @@ en: title: Edit answer destroy: success_notice: "Answer deleted successfully" + images: + index: + title: Images videos: index: title: Videos diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 3ea4905cc..d40af634b 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -1160,6 +1160,9 @@ es: title: Editar respuesta destroy: success_notice: "Respuesta eliminada correctamente" + images: + index: + title: Imágenes videos: index: title: Vídeos diff --git a/spec/controllers/admin/poll/questions/answers/images_controller_spec.rb b/spec/controllers/admin/poll/questions/answers/images_controller_spec.rb new file mode 100644 index 000000000..6e2c27b82 --- /dev/null +++ b/spec/controllers/admin/poll/questions/answers/images_controller_spec.rb @@ -0,0 +1,52 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::ImagesController, :admin do + let(:current_answer) { create(:poll_question_answer, poll: create(:poll)) } + let(:future_answer) { create(:poll_question_answer, poll: create(:poll, :future)) } + + describe "POST create" do + let(:answer_attributes) do + { + images_attributes: { + "0" => { + attachment: fixture_file_upload("clippy.jpg"), + title: "Title", + user_id: User.last.id + } + } + } + end + + it "is not possible for an already started poll" do + post :create, params: { poll_question_answer: answer_attributes, answer_id: current_answer } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'update' on Answer." + expect(Image.count).to eq 0 + end + + it "is possible for a not started poll" do + post :create, params: { poll_question_answer: answer_attributes, answer_id: future_answer } + + expect(response).to redirect_to admin_answer_images_path(future_answer) + expect(flash[:notice]).to eq "Image uploaded successfully" + expect(Image.count).to eq 1 + end + end + + describe "DELETE destroy" do + it "is not possible for an already started poll" do + current_image = create(:image, imageable: current_answer) + delete :destroy, xhr: true, params: { id: current_image } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'destroy' on Image." + expect(Image.count).to eq 1 + end + + it "is possible for a not started poll" do + future_image = create(:image, imageable: future_answer) + delete :destroy, xhr: true, params: { id: future_image } + + expect(Image.count).to eq 0 + end + end +end diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 707a1af26..03ad24670 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -80,6 +80,10 @@ FactoryBot.define do trait :with_video do after(:create) { |answer| create(:poll_answer_video, answer: answer) } end + + factory :future_poll_question_answer do + poll { association(:poll, :future) } + end end factory :poll_answer_video, class: "Poll::Question::Answer::Video" do diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 289407fd4..2eebf28a1 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -24,7 +24,8 @@ describe Abilities::Administrator do let(:future_poll_question_answer) { create(:poll_question_answer, poll: future_poll) } let(:current_poll_answer_video) { create(:poll_answer_video, answer: current_poll_question_answer) } let(:future_poll_answer_video) { create(:poll_answer_video, answer: future_poll_question_answer) } - let(:answer_image) { build(:image, imageable: current_poll_question_answer) } + let(:current_poll_answer_image) { build(:image, imageable: current_poll_question_answer) } + let(:future_poll_answer_image) { build(:image, imageable: future_poll_question_answer) } let(:past_process) { create(:legislation_process, :past) } let(:past_draft_process) { create(:legislation_process, :past, :not_published) } @@ -141,8 +142,8 @@ describe Abilities::Administrator do it { should_not be_able_to(:update, current_poll_answer_video) } it { should_not be_able_to(:destroy, current_poll_answer_video) } - it { should be_able_to(:create, answer_image) } - it { should be_able_to(:destroy, answer_image) } + it { should be_able_to(:destroy, future_poll_answer_image) } + it { should_not be_able_to(:destroy, current_poll_answer_image) } it { is_expected.to be_able_to :manage, Dashboard::AdministratorTask } it { is_expected.to be_able_to :manage, dashboard_administrator_task } diff --git a/spec/system/admin/poll/questions/answers/images/images_spec.rb b/spec/system/admin/poll/questions/answers/images/images_spec.rb index 154e2e8fd..cfbc12293 100644 --- a/spec/system/admin/poll/questions/answers/images/images_spec.rb +++ b/spec/system/admin/poll/questions/answers/images/images_spec.rb @@ -1,8 +1,11 @@ require "rails_helper" describe "Images", :admin do + let(:future_poll) { create(:poll, :future) } + let(:current_poll) { create(:poll) } + it_behaves_like "nested imageable", - "poll_question_answer", + "future_poll_question_answer", "new_admin_answer_image_path", { answer_id: "id" }, nil, @@ -30,34 +33,63 @@ describe "Images", :admin do end end - scenario "Add image to answer" do - answer = create(:poll_question_answer) + describe "Add image to answer" do + scenario "Is possible for a not started poll" do + answer = create(:poll_question_answer, poll: future_poll) - visit admin_answer_images_path(answer) - expect(page).not_to have_css("img[title='clippy.jpg']") - expect(page).not_to have_content("clippy.jpg") + visit admin_answer_images_path(answer) - visit new_admin_answer_image_path(answer) - imageable_attach_new_file(file_fixture("clippy.jpg")) - click_button "Save image" + expect(page).not_to have_css "img[title='clippy.jpg']" + expect(page).not_to have_content "clippy.jpg" - expect(page).to have_css("img[title='clippy.jpg']") - expect(page).to have_content("clippy.jpg") - end + click_link "Add image" + expect(page).to have_content "Descriptive image" - scenario "Remove image from answer" do - answer = create(:poll_question_answer) - image = create(:image, imageable: answer) + imageable_attach_new_file(file_fixture("clippy.jpg")) + click_button "Save image" - visit admin_answer_images_path(answer) - expect(page).to have_css("img[title='#{image.title}']") - expect(page).to have_content(image.title) - - accept_confirm "Are you sure? Remove image \"#{image.title}\"" do - click_link "Remove image" + expect(page).to have_content "Image uploaded successfully" + expect(page).to have_css "img[title='clippy.jpg']" + expect(page).to have_content "clippy.jpg" end - expect(page).not_to have_css("img[title='#{image.title}']") - expect(page).not_to have_content(image.title) + scenario "Is not possible for an already started poll" do + answer = create(:poll_question_answer, poll: current_poll) + + visit admin_answer_images_path(answer) + + expect(page).not_to have_link "Add image" + expect(page).to have_content "Once the poll has started it will not be possible to create, edit or" + end + end + + describe "Remove image from answer" do + scenario "Is possible for a not started poll" do + answer = create(:poll_question_answer, poll: future_poll) + image = create(:image, imageable: answer) + + visit admin_answer_images_path(answer) + expect(page).to have_css "img[title='#{image.title}']" + expect(page).to have_content image.title + + accept_confirm "Are you sure? Remove image \"#{image.title}\"" do + click_link "Remove image" + end + + expect(page).not_to have_css "img[title='#{image.title}']" + expect(page).not_to have_content image.title + end + + scenario "Is not possible for an already started poll" do + answer = create(:poll_question_answer, poll: current_poll) + image = create(:image, imageable: answer) + + visit admin_answer_images_path(answer) + expect(page).to have_css "img[title='#{image.title}']" + expect(page).to have_content image.title + + expect(page).not_to have_link "Remove image" + expect(page).to have_content "Once the poll has started it will not be possible to create, edit or" + end end end From 4c8f247de713a6e735ea8b98199d0b6c04bfc7e7 Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Mon, 5 Sep 2022 11:33:13 +0200 Subject: [PATCH 16/18] Don't allow to modify answer's documents for started polls --- .../documents/index_component.html.erb | 32 ++++++++--------- .../answers/documents/index_component.rb | 1 + .../table_actions_component.html.erb | 9 +++++ .../documents/table_actions_component.rb | 14 ++++++++ .../questions/answers/documents_controller.rb | 1 + app/models/abilities/administrator.rb | 4 ++- app/models/abilities/common.rb | 2 +- .../answers/documents/index_component_spec.rb | 19 ++++++++++ .../documents/table_actions_component_spec.rb | 27 ++++++++++++++ .../answers/documents_controller_spec.rb | 35 +++++++++++++++++++ spec/controllers/documents_controller_spec.rb | 28 +++++++++++++++ spec/models/abilities/administrator_spec.rb | 5 +++ .../answers/documents/documents_spec.rb | 7 ++-- 13 files changed, 162 insertions(+), 22 deletions(-) create mode 100644 app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb create mode 100644 app/components/admin/poll/questions/answers/documents/table_actions_component.rb create mode 100644 spec/components/admin/poll/questions/answers/documents/index_component_spec.rb create mode 100644 spec/components/admin/poll/questions/answers/documents/table_actions_component_spec.rb create mode 100644 spec/controllers/admin/poll/questions/answers/documents_controller_spec.rb create mode 100644 spec/controllers/documents_controller_spec.rb diff --git a/app/components/admin/poll/questions/answers/documents/index_component.html.erb b/app/components/admin/poll/questions/answers/documents/index_component.html.erb index 9c6b95c20..9f34e68e9 100644 --- a/app/components/admin/poll/questions/answers/documents/index_component.html.erb +++ b/app/components/admin/poll/questions/answers/documents/index_component.html.erb @@ -8,15 +8,21 @@
- <%= form_for(Poll::Question::Answer.new, url: admin_answer_documents_path(answer)) do |f| %> - <%= render "shared/errors", resource: answer %> + <% if can?(:update, @answer) %> + <%= form_for(Poll::Question::Answer.new, url: admin_answer_documents_path(answer)) do |f| %> + <%= render "shared/errors", resource: answer %> -
- <%= render "documents/nested_documents", f: f %> -
+
+ <%= render "documents/nested_documents", f: f %> +
-
- <%= f.submit(class: "button expanded", value: t("shared.save")) %> +
+ <%= f.submit(class: "button expanded", value: t("shared.save")) %> +
+ <% end %> + <% else %> +
+ <%= t("admin.questions.no_edit") %>
<% end %> @@ -33,17 +39,7 @@ <%= link_to document.title, document.attachment %>
<% end %> diff --git a/app/components/admin/poll/questions/answers/documents/index_component.rb b/app/components/admin/poll/questions/answers/documents/index_component.rb index ca72d4f98..5a46f05da 100644 --- a/app/components/admin/poll/questions/answers/documents/index_component.rb +++ b/app/components/admin/poll/questions/answers/documents/index_component.rb @@ -1,5 +1,6 @@ class Admin::Poll::Questions::Answers::Documents::IndexComponent < ApplicationComponent attr_reader :answer + delegate :can?, to: :helpers def initialize(answer) @answer = answer diff --git a/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb new file mode 100644 index 000000000..f2406512e --- /dev/null +++ b/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb @@ -0,0 +1,9 @@ +<%= render Admin::TableActionsComponent.new(document, + actions: actions, + destroy_path: document_path(document)) do |table_actions| %> + <%= table_actions.action(:download, + text: t("documents.buttons.download_document"), + path: document.attachment, + target: "_blank", + rel: "nofollow") %> +<% end %> diff --git a/app/components/admin/poll/questions/answers/documents/table_actions_component.rb b/app/components/admin/poll/questions/answers/documents/table_actions_component.rb new file mode 100644 index 000000000..38b7a7c08 --- /dev/null +++ b/app/components/admin/poll/questions/answers/documents/table_actions_component.rb @@ -0,0 +1,14 @@ +class Admin::Poll::Questions::Answers::Documents::TableActionsComponent < ApplicationComponent + attr_reader :document + delegate :can?, to: :helpers + + def initialize(document) + @document = document + end + + private + + def actions + [:destroy].select { |action| can?(action, document) } + end +end diff --git a/app/controllers/admin/poll/questions/answers/documents_controller.rb b/app/controllers/admin/poll/questions/answers/documents_controller.rb index 3210896d6..eeea65aa6 100644 --- a/app/controllers/admin/poll/questions/answers/documents_controller.rb +++ b/app/controllers/admin/poll/questions/answers/documents_controller.rb @@ -8,6 +8,7 @@ class Admin::Poll::Questions::Answers::DocumentsController < Admin::Poll::BaseCo def create @answer.attributes = documents_params + authorize! :update, @answer if @answer.save redirect_to admin_answer_documents_path(@answer), diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 3c94e802e..f6e2bb038 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -124,7 +124,9 @@ module Abilities cannot :comment_as_moderator, [::Legislation::Question, Legislation::Annotation, ::Legislation::Proposal] can [:create], Document - can [:destroy], Document, documentable_type: "Poll::Question::Answer" + can [:destroy], Document do |document| + document.documentable_type == "Poll::Question::Answer" && can?(:update, document.documentable) + end can [:create, :destroy], DirectUpload can [:deliver], Newsletter, hidden_at: nil diff --git a/app/models/abilities/common.rb b/app/models/abilities/common.rb index 264b98521..2da9a057d 100644 --- a/app/models/abilities/common.rb +++ b/app/models/abilities/common.rb @@ -71,7 +71,7 @@ module Abilities can [:create, :destroy], Follow, user_id: user.id can [:destroy], Document do |document| - document.documentable&.author_id == user.id + document.documentable_type != "Poll::Question::Answer" && document.documentable&.author_id == user.id end can [:destroy], Image do |image| diff --git a/spec/components/admin/poll/questions/answers/documents/index_component_spec.rb b/spec/components/admin/poll/questions/answers/documents/index_component_spec.rb new file mode 100644 index 000000000..053c37ce5 --- /dev/null +++ b/spec/components/admin/poll/questions/answers/documents/index_component_spec.rb @@ -0,0 +1,19 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::Documents::IndexComponent do + before { sign_in(create(:administrator).user) } + let(:future_answer) { create(:poll_question_answer, poll: create(:poll, :future)) } + let(:current_answer) { create(:poll_question_answer, poll: create(:poll)) } + + it "displays the 'Add new document' link when the poll has not started" do + render_inline Admin::Poll::Questions::Answers::Documents::IndexComponent.new(future_answer) + + expect(page).to have_link "Add new document" + end + + it "does not display the 'Add new document' link when the poll has started" do + render_inline Admin::Poll::Questions::Answers::Documents::IndexComponent.new(current_answer) + + expect(page).not_to have_link "Add new document" + end +end diff --git a/spec/components/admin/poll/questions/answers/documents/table_actions_component_spec.rb b/spec/components/admin/poll/questions/answers/documents/table_actions_component_spec.rb new file mode 100644 index 000000000..2d2261aea --- /dev/null +++ b/spec/components/admin/poll/questions/answers/documents/table_actions_component_spec.rb @@ -0,0 +1,27 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::Documents::TableActionsComponent, controller: Admin::BaseController do + before { sign_in(create(:administrator).user) } + let(:future_answer) { create(:poll_question_answer, poll: create(:poll, :future)) } + let(:current_answer) { create(:poll_question_answer, poll: create(:poll)) } + + it "displays the destroy action when the poll has not started" do + document = create(:document, documentable: future_answer) + + render_inline Admin::Poll::Questions::Answers::Documents::TableActionsComponent.new(document) + + expect(page).to have_link "Download file" + expect(page).to have_button "Delete" + expect(page).not_to have_link "Edit" + end + + it "does not display the destroy action when the poll has started" do + document = create(:document, documentable: current_answer) + + render_inline Admin::Poll::Questions::Answers::Documents::TableActionsComponent.new(document) + + expect(page).to have_link "Download file" + expect(page).not_to have_button "Delete" + expect(page).not_to have_link "Edit" + end +end diff --git a/spec/controllers/admin/poll/questions/answers/documents_controller_spec.rb b/spec/controllers/admin/poll/questions/answers/documents_controller_spec.rb new file mode 100644 index 000000000..84e02f0f6 --- /dev/null +++ b/spec/controllers/admin/poll/questions/answers/documents_controller_spec.rb @@ -0,0 +1,35 @@ +require "rails_helper" + +describe Admin::Poll::Questions::Answers::DocumentsController, :admin do + let(:current_answer) { create(:poll_question_answer, poll: create(:poll)) } + let(:future_answer) { create(:poll_question_answer, poll: create(:poll, :future)) } + + describe "POST create" do + let(:answer_attributes) do + { + documents_attributes: { + "0" => { + attachment: fixture_file_upload("clippy.pdf"), + title: "Title", + user_id: User.last.id + } + } + } + end + + it "is not possible for an already started poll" do + post :create, params: { poll_question_answer: answer_attributes, answer_id: current_answer } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'update' on Answer." + expect(Document.count).to eq 0 + end + + it "is possible for a not started poll" do + post :create, params: { poll_question_answer: answer_attributes, answer_id: future_answer } + + expect(response).to redirect_to admin_answer_documents_path(future_answer) + expect(flash[:notice]).to eq "Document uploaded successfully" + expect(Document.count).to eq 1 + end + end +end diff --git a/spec/controllers/documents_controller_spec.rb b/spec/controllers/documents_controller_spec.rb new file mode 100644 index 000000000..9b0977034 --- /dev/null +++ b/spec/controllers/documents_controller_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +describe DocumentsController do + describe "DELETE destroy" do + context "Poll answers administration", :admin do + let(:current_answer) { create(:poll_question_answer, poll: create(:poll)) } + let(:future_answer) { create(:poll_question_answer, poll: create(:poll, :future)) } + + it "is not possible for an already started poll" do + document = create(:document, documentable: current_answer) + delete :destroy, params: { id: document } + + expect(flash[:alert]).to eq "You do not have permission to carry out the action 'destroy' on Document." + expect(Document.count).to eq 1 + end + + it "is possible for a not started poll" do + document = create(:document, documentable: future_answer) + request.env["HTTP_REFERER"] = admin_answer_documents_path(future_answer) + delete :destroy, params: { id: document } + + expect(response).to redirect_to admin_answer_documents_path(future_answer) + expect(flash[:notice]).to eq "Document was deleted successfully." + expect(Document.count).to eq 0 + end + end + end +end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 2eebf28a1..4579aa4c8 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -26,6 +26,8 @@ describe Abilities::Administrator do let(:future_poll_answer_video) { create(:poll_answer_video, answer: future_poll_question_answer) } let(:current_poll_answer_image) { build(:image, imageable: current_poll_question_answer) } let(:future_poll_answer_image) { build(:image, imageable: future_poll_question_answer) } + let(:current_poll_answer_document) { build(:document, documentable: current_poll_question_answer) } + let(:future_poll_answer_document) { build(:document, documentable: future_poll_question_answer) } let(:past_process) { create(:legislation_process, :past) } let(:past_draft_process) { create(:legislation_process, :past, :not_published) } @@ -145,6 +147,9 @@ describe Abilities::Administrator do it { should be_able_to(:destroy, future_poll_answer_image) } it { should_not be_able_to(:destroy, current_poll_answer_image) } + it { should be_able_to(:destroy, future_poll_answer_document) } + it { should_not be_able_to(:destroy, current_poll_answer_document) } + it { is_expected.to be_able_to :manage, Dashboard::AdministratorTask } it { is_expected.to be_able_to :manage, dashboard_administrator_task } diff --git a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb index 8a3b3e356..d4439e5bb 100644 --- a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb +++ b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" describe "Documents", :admin do + let(:future_poll) { create(:poll, :future) } + context "Index" do scenario "Answer with no documents" do answer = create(:poll_question_answer) @@ -23,7 +25,7 @@ describe "Documents", :admin do end scenario "Create document for answer" do - answer = create(:poll_question_answer) + answer = create(:poll_question_answer, poll: future_poll) visit admin_answer_documents_path(answer) @@ -35,7 +37,7 @@ describe "Documents", :admin do end scenario "Remove document from answer" do - answer = create(:poll_question_answer) + answer = create(:poll_question_answer, poll: future_poll) document = create(:document, documentable: answer) visit admin_answer_documents_path(answer) @@ -45,6 +47,7 @@ describe "Documents", :admin do click_button "Delete" end + expect(page).to have_content "Document was deleted successfully." expect(page).not_to have_content(document.title) end end From 518af3eb97cc9eee948929ec70f1d9b1dcb8fb6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 18 Sep 2022 17:51:18 +0200 Subject: [PATCH 17/18] Remove duplication in poll actions tables We were using the same logic in four different places, so we're creating a new class to handle that logic. Note that I didn't find a way to delegate the `content` method to a `Admin::TableActionsComponent`, so we're delegating the `action` method instead. That means we need to create a method returning an `Admin::TableActionsComponent`. We also need to cache this object; otherwise we were getting an error when calling `actions.action` from the `Admin::Poll::Questions::TableActionsComponent`. --- .../allowed_table_actions_component.html.erb | 3 + .../admin/allowed_table_actions_component.rb | 20 +++++++ .../table_actions_component.html.erb | 15 +++-- .../documents/table_actions_component.rb | 7 --- .../answers/table_actions_component.html.erb | 2 +- .../answers/table_actions_component.rb | 7 --- .../videos/table_actions_component.html.erb | 2 +- .../answers/videos/table_actions_component.rb | 7 --- .../table_actions_component.html.erb | 4 +- .../poll/questions/table_actions_component.rb | 7 --- .../allowed_table_actions_component_spec.rb | 57 +++++++++++++++++++ 11 files changed, 91 insertions(+), 40 deletions(-) create mode 100644 app/components/admin/allowed_table_actions_component.html.erb create mode 100644 app/components/admin/allowed_table_actions_component.rb create mode 100644 spec/components/admin/allowed_table_actions_component_spec.rb diff --git a/app/components/admin/allowed_table_actions_component.html.erb b/app/components/admin/allowed_table_actions_component.html.erb new file mode 100644 index 000000000..df89fe8f7 --- /dev/null +++ b/app/components/admin/allowed_table_actions_component.html.erb @@ -0,0 +1,3 @@ +<%= render table_actions_component do %> + <%= content %> +<% end %> diff --git a/app/components/admin/allowed_table_actions_component.rb b/app/components/admin/allowed_table_actions_component.rb new file mode 100644 index 000000000..dc5f2e1d5 --- /dev/null +++ b/app/components/admin/allowed_table_actions_component.rb @@ -0,0 +1,20 @@ +class Admin::AllowedTableActionsComponent < ApplicationComponent + attr_reader :record, :options + delegate :can?, to: :helpers + delegate :action, to: :table_actions_component + + def initialize(record, **options) + @record = record + @options = options + end + + private + + def actions + (options[:actions] || [:edit, :destroy]).select { |action| can?(action, record) } + end + + def table_actions_component + @table_actions_component ||= Admin::TableActionsComponent.new(record, **options.merge(actions: actions)) + end +end diff --git a/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb index f2406512e..19e8e3ac2 100644 --- a/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb +++ b/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb @@ -1,9 +1,8 @@ -<%= render Admin::TableActionsComponent.new(document, - actions: actions, - destroy_path: document_path(document)) do |table_actions| %> - <%= table_actions.action(:download, - text: t("documents.buttons.download_document"), - path: document.attachment, - target: "_blank", - rel: "nofollow") %> +<%= render Admin::AllowedTableActionsComponent.new(document, + destroy_path: document_path(document)) do |actions| %> + <%= actions.action(:download, + text: t("documents.buttons.download_document"), + path: document.attachment, + target: "_blank", + rel: "nofollow") %> <% end %> diff --git a/app/components/admin/poll/questions/answers/documents/table_actions_component.rb b/app/components/admin/poll/questions/answers/documents/table_actions_component.rb index 38b7a7c08..da895ef1a 100644 --- a/app/components/admin/poll/questions/answers/documents/table_actions_component.rb +++ b/app/components/admin/poll/questions/answers/documents/table_actions_component.rb @@ -1,14 +1,7 @@ class Admin::Poll::Questions::Answers::Documents::TableActionsComponent < ApplicationComponent attr_reader :document - delegate :can?, to: :helpers def initialize(document) @document = document end - - private - - def actions - [:destroy].select { |action| can?(action, document) } - end end diff --git a/app/components/admin/poll/questions/answers/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/table_actions_component.html.erb index a9fc5cd8a..d401f1a36 100644 --- a/app/components/admin/poll/questions/answers/table_actions_component.html.erb +++ b/app/components/admin/poll/questions/answers/table_actions_component.html.erb @@ -1 +1 @@ -<%= render Admin::TableActionsComponent.new(answer, actions: actions) %> +<%= render Admin::AllowedTableActionsComponent.new(answer) %> diff --git a/app/components/admin/poll/questions/answers/table_actions_component.rb b/app/components/admin/poll/questions/answers/table_actions_component.rb index 577e099b1..94b60b92e 100644 --- a/app/components/admin/poll/questions/answers/table_actions_component.rb +++ b/app/components/admin/poll/questions/answers/table_actions_component.rb @@ -1,14 +1,7 @@ class Admin::Poll::Questions::Answers::TableActionsComponent < ApplicationComponent attr_reader :answer - delegate :can?, to: :helpers def initialize(answer) @answer = answer end - - private - - def actions - [:edit, :destroy].select { |action| can?(action, answer) } - end end diff --git a/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb index f799eae21..e4ebef5f4 100644 --- a/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb +++ b/app/components/admin/poll/questions/answers/videos/table_actions_component.html.erb @@ -1 +1 @@ -<%= render Admin::TableActionsComponent.new(video, actions: actions) %> +<%= render Admin::AllowedTableActionsComponent.new(video) %> diff --git a/app/components/admin/poll/questions/answers/videos/table_actions_component.rb b/app/components/admin/poll/questions/answers/videos/table_actions_component.rb index 935cd5c80..06e772b6b 100644 --- a/app/components/admin/poll/questions/answers/videos/table_actions_component.rb +++ b/app/components/admin/poll/questions/answers/videos/table_actions_component.rb @@ -1,14 +1,7 @@ class Admin::Poll::Questions::Answers::Videos::TableActionsComponent < ApplicationComponent attr_reader :video - delegate :can?, to: :helpers def initialize(video) @video = video end - - private - - def actions - [:edit, :destroy].select { |action| can?(action, video) } - 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 index 5858ad82c..736eda7af 100644 --- a/app/components/admin/poll/questions/table_actions_component.html.erb +++ b/app/components/admin/poll/questions/table_actions_component.html.erb @@ -1,3 +1,3 @@ -<%= render Admin::TableActionsComponent.new(question, actions: actions) do |table_actions| %> - <%= table_actions.action(:answers, text: t("admin.polls.show.edit_answers")) %> +<%= render Admin::AllowedTableActionsComponent.new(question) do |actions| %> + <%= 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 index 85c7bb279..ba4eb66f4 100644 --- a/app/components/admin/poll/questions/table_actions_component.rb +++ b/app/components/admin/poll/questions/table_actions_component.rb @@ -1,14 +1,7 @@ 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/spec/components/admin/allowed_table_actions_component_spec.rb b/spec/components/admin/allowed_table_actions_component_spec.rb new file mode 100644 index 000000000..6396be6b8 --- /dev/null +++ b/spec/components/admin/allowed_table_actions_component_spec.rb @@ -0,0 +1,57 @@ +require "rails_helper" + +describe Admin::AllowedTableActionsComponent, controller: Admin::BaseController do + before do + sign_in(create(:administrator).user) + allow_any_instance_of(Admin::AllowedTableActionsComponent).to receive(:can?).and_return true + end + let(:record) { create(:banner, title: "Important!") } + + it "renders edit and destroy actions by default if they're allowed" do + component = Admin::AllowedTableActionsComponent.new(record) + + render_inline component + + expect(page).to have_link count: 1 + expect(page).to have_link "Edit" + expect(page).to have_button count: 1 + expect(page).to have_button "Delete" + end + + it "accepts an actions parameter" do + render_inline Admin::AllowedTableActionsComponent.new(record, actions: [:edit]) + + expect(page).to have_link "Edit" + expect(page).not_to have_button "Delete" + end + + it "accepts custom options" do + render_inline Admin::AllowedTableActionsComponent.new(record, edit_text: "change", edit_path: "/myedit") + + expect(page).to have_link "change", href: "/myedit" + end + + it "accepts custom content" do + render_inline Admin::AllowedTableActionsComponent.new(record) do + "Main".html_safe + end + + expect(page).to have_link count: 2 + expect(page).to have_link "Main", href: "/" + expect(page).to have_link "Edit" + + expect(page).to have_button count: 1 + expect(page).to have_button "Delete" + end + + it "only renders the allowed actions" do + component = Admin::AllowedTableActionsComponent.new(record) + allow(component).to receive(:can?).with(:edit, record).and_return true + allow(component).to receive(:can?).with(:destroy, record).and_return false + + render_inline component + + expect(page).to have_link "Edit" + expect(page).not_to have_button "Delete" + end +end From 24099e880bee971cad0757939d78fc6939c0f0dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 20 Sep 2022 15:24:33 +0200 Subject: [PATCH 18/18] Fix crash when adding invalid documents to answers We were rendering the `new` action, but that action doesn't exist. Before commit ec861ca8e, we were rendering the `edit` action of an answer, which was confusing as well. Note that, when adding an invalid document, `@answer.documents` contains that invalid document (which is not present in the database). Since we're rendering the index, this new document would appear in the list of the documents that can be deleted; to avoid that, we're kind of "reloading" the answer object in the component by finding the record in the database. We aren't using `@answer.reload` because doing so would remove the validation errors. --- .../documents/index_component.html.erb | 4 +-- .../answers/documents/index_component.rb | 6 ++++ .../questions/answers/documents_controller.rb | 2 +- .../answers/documents/documents_spec.rb | 29 ++++++++++++++----- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/app/components/admin/poll/questions/answers/documents/index_component.html.erb b/app/components/admin/poll/questions/answers/documents/index_component.html.erb index 9f34e68e9..6db626293 100644 --- a/app/components/admin/poll/questions/answers/documents/index_component.html.erb +++ b/app/components/admin/poll/questions/answers/documents/index_component.html.erb @@ -26,14 +26,14 @@ <% end %> - <% if answer.documents.present? %> + <% if documents.present? %>
- <%= render Admin::TableActionsComponent.new(answer, actions: [:edit]) %> + <%= render Admin::Poll::Questions::Answers::TableActionsComponent.new(answer) %>
<%= video.title %> <%= link_to video.url, video.url %> - <%= render Admin::TableActionsComponent.new(video) %> + <%= render Admin::Poll::Questions::Answers::Videos::TableActionsComponent.new(video) %>
- <%= render Admin::TableActionsComponent.new(document, - actions: [:destroy], - destroy_path: document_path(document) - ) do |actions| %> - <%= actions.action(:download, - text: t("documents.buttons.download_document"), - path: document.attachment, - target: "_blank", - rel: "nofollow") %> - - <% end %> + <%= render Admin::Poll::Questions::Answers::Documents::TableActionsComponent.new(document) %>
- <% answer.documents.each do |document| %> + <% documents.each do |document| %>
<%= t("admin.questions.show.answers.document_title") %> <%= t("admin.questions.show.answers.document_actions") %>
<%= link_to document.title, document.attachment %> diff --git a/app/components/admin/poll/questions/answers/documents/index_component.rb b/app/components/admin/poll/questions/answers/documents/index_component.rb index 5a46f05da..b45c9856e 100644 --- a/app/components/admin/poll/questions/answers/documents/index_component.rb +++ b/app/components/admin/poll/questions/answers/documents/index_component.rb @@ -5,4 +5,10 @@ class Admin::Poll::Questions::Answers::Documents::IndexComponent < ApplicationCo def initialize(answer) @answer = answer end + + private + + def documents + @documents ||= @answer.class.find(@answer.id).documents + end end diff --git a/app/controllers/admin/poll/questions/answers/documents_controller.rb b/app/controllers/admin/poll/questions/answers/documents_controller.rb index eeea65aa6..4fce04d05 100644 --- a/app/controllers/admin/poll/questions/answers/documents_controller.rb +++ b/app/controllers/admin/poll/questions/answers/documents_controller.rb @@ -14,7 +14,7 @@ class Admin::Poll::Questions::Answers::DocumentsController < Admin::Poll::BaseCo redirect_to admin_answer_documents_path(@answer), notice: t("admin.documents.create.success_notice") else - render :new + render :index end end diff --git a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb index d4439e5bb..cc6f55bc9 100644 --- a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb +++ b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb @@ -24,16 +24,31 @@ describe "Documents", :admin do end end - scenario "Create document for answer" do - answer = create(:poll_question_answer, poll: future_poll) + describe "Create document for answer" do + scenario "with valid data" do + answer = create(:poll_question_answer, poll: future_poll) - visit admin_answer_documents_path(answer) + visit admin_answer_documents_path(answer) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.pdf")) - click_button "Save" + documentable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.pdf")) + click_button "Save" - expect(page).to have_content "Document uploaded successfully" - expect(page).to have_link "clippy.pdf" + expect(page).to have_content "Document uploaded successfully" + expect(page).to have_link "clippy.pdf" + end + + scenario "with invalid data" do + answer = create(:poll_question_answer, poll: future_poll) + + visit admin_answer_documents_path(answer) + + documentable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.pdf")) + fill_in "Title", with: "" + click_button "Save" + + expect(page).to have_content "1 error prevented this Answer from being saved" + expect(page).to have_content "Documents list" + end end scenario "Remove document from answer" do