From 245594f32b8f4ca8408a6ff61d099df5dcfbe84e Mon Sep 17 00:00:00 2001
From: Julian Herrero <microweb10@gmail.com>
Date: Fri, 2 Sep 2022 18:46:17 +0200
Subject: [PATCH] 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" %>
+<div class="clear"></div>
+
+<h2 class="inline-block">
+  <%= t("admin.answers.images.index.title") %>
+</h2>
+
+<% 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 %>
+  <div class="callout warning">
+    <strong><%= t("admin.questions.no_edit") %></strong>
+  </div>
+<% end %>
 
 <ul class="breadcrumbs margin-top">
   <li><%= @answer.question.title %></li>
@@ -13,11 +25,13 @@
   <div class="small-12 medium-4 column end">
     <%= 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 %>
   </div>
 <% 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