CLDC-3649 Delete custom resource (#2693)

* Stub some collection resource requests

* Rebase changes

* Add delete confirmation page

* Allow deleting resources

* lint

* Return unauthorised status

* Fix delete path from delete confirmation page

* Change the ordering for collection resources

* Do not create header for empty label

* Add margin between sections

app/components/document_list_component.html.erb

@@ -1,4 +1,6 @@
-<h3 class="govuk-heading-m"><%= label %></h3>
+<% unless label.blank? %>
+  <h3 class="govuk-heading-m"><%= label %></h3>
+<% end %>
 <dl class="app-document-list">
   <% items.each do |item| %>
     <div class="app-document-list__item">

app/controllers/collection_resources_controller.rb

@@ -8,8 +8,8 @@ class CollectionResourcesController < ApplicationController
 
     @mandatory_lettings_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("lettings", editable_collection_resource_years)
     @mandatory_sales_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("sales", editable_collection_resource_years)
-    @additional_lettings_collection_resources_per_year = CollectionResource.where(log_type: "lettings", mandatory: false).group_by(&:year)
-    @additional_sales_collection_resources_per_year = CollectionResource.where(log_type: "sales", mandatory: false).group_by(&:year)
+    @additional_lettings_collection_resources_per_year = CollectionResource.visible.where(log_type: "lettings", mandatory: false).group_by(&:year)
+    @additional_sales_collection_resources_per_year = CollectionResource.visible.where(log_type: "sales", mandatory: false).group_by(&:year)
   end
 
   def download_mandatory_collection_resource
@@ -35,7 +35,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def edit_mandatory_collection_resource
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     year = params[:year].to_i
     resource_type = params[:resource_type]
@@ -51,7 +51,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def edit_additional_collection_resource
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id])
 
@@ -62,7 +62,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def update_mandatory_collection_resource
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     year = resource_params[:year].to_i
     resource_type = resource_params[:resource_type]
@@ -92,7 +92,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def update_additional_collection_resource
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id])
 
@@ -122,7 +122,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def confirm_mandatory_collection_resources_release
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     @year = params[:year].to_i
 
@@ -132,7 +132,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def release_mandatory_collection_resources
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     year = params[:year].to_i
 
@@ -145,7 +145,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def new
-    return render_not_found unless current_user.support?
+    return render_not_authorized unless current_user.support?
 
     year = params[:year].to_i
     log_type = params[:log_type]
@@ -156,7 +156,7 @@ class CollectionResourcesController < ApplicationController
   end
 
   def create
-    return render_not_found unless current_user.support? && editable_collection_resource_years.include?(resource_params[:year].to_i)
+    return render_not_authorized unless current_user.support? && editable_collection_resource_years.include?(resource_params[:year].to_i)
 
     @collection_resource = CollectionResource.new(resource_params)
     @collection_resource.download_filename ||= @collection_resource.file&.original_filename
@@ -184,6 +184,29 @@ class CollectionResourcesController < ApplicationController
     end
   end
 
+  def delete_confirmation
+    return render_not_authorized unless current_user.support?
+
+    @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id])
+
+    return render_not_found unless @collection_resource
+
+    render "collection_resources/delete_confirmation"
+  end
+
+  def delete
+    return render_not_authorized unless current_user.support?
+
+    @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id])
+
+    return render_not_found unless @collection_resource
+
+    @collection_resource.discard!
+
+    flash[:notice] = "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.short_display_name.downcase} has been deleted."
+    redirect_to collection_resources_path
+  end
+
 private
 
   def resource_params

app/controllers/start_controller.rb

@@ -4,8 +4,8 @@ class StartController < ApplicationController
   def index
     @mandatory_lettings_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("lettings", displayed_collection_resource_years)
     @mandatory_sales_collection_resources_per_year = MandatoryCollectionResourcesService.generate_resources("sales", displayed_collection_resource_years)
-    @additional_lettings_collection_resources_per_year = CollectionResource.where(log_type: "lettings", mandatory: false, year: displayed_collection_resource_years).group_by(&:year)
-    @additional_sales_collection_resources_per_year = CollectionResource.where(log_type: "sales", mandatory: false, year: displayed_collection_resource_years).group_by(&:year)
+    @additional_lettings_collection_resources_per_year = CollectionResource.visible.where(log_type: "lettings", mandatory: false, year: displayed_collection_resource_years).group_by(&:year)
+    @additional_sales_collection_resources_per_year = CollectionResource.visible.where(log_type: "sales", mandatory: false, year: displayed_collection_resource_years).group_by(&:year)
     if current_user
       @homepage_presenter = HomepagePresenter.new(current_user)
       render "home/index"

app/frontend/styles/_document-list.scss

@@ -1,5 +1,4 @@
 .app-document-list {
-  margin-top: govuk-spacing(3);
   margin-bottom: govuk-spacing(6);
 }
 

app/models/collection_resource.rb

@@ -1,8 +1,10 @@
 class CollectionResource < ApplicationRecord
   include Rails.application.routes.url_helpers
+  has_paper_trail
 
   attr_accessor :file
 
+  scope :visible, -> { where(discarded_at: nil) }
   validates :short_display_name, presence: true
 
   def download_path
@@ -35,4 +37,9 @@ class CollectionResource < ApplicationRecord
   def validate_short_display_name
     errors.add(:short_display_name, :blank) if short_display_name.blank?
   end
+
+  def discard!
+    CollectionResourcesService.new.delete_collection_resource(download_filename)
+    update!(discarded_at: Time.zone.now)
+  end
 end

app/services/collection_resources_service.rb

@@ -27,4 +27,8 @@ class CollectionResourcesService
     content_type = MiniMime.lookup_by_filename(filename)&.content_type
     @storage_service.write_file(filename, file, content_type:)
   end
+
+  def delete_collection_resource(filename)
+    @storage_service.delete_file(filename)
+  end
 end

app/services/storage/local_disk_service.rb

@@ -43,5 +43,11 @@ module Storage
 
       File.exist?(path)
     end
+
+    def delete_file(filename)
+      path = Rails.root.join("tmp/storage", filename)
+
+      File.delete(path)
+    end
   end
 end

app/services/storage/s3_service.rb

@@ -64,6 +64,10 @@ module Storage
       false
     end
 
+    def delete_file(file_name)
+      @client.delete_object(bucket: @configuration.bucket_name, key: file_name)
+    end
+
   private
 
     def create_configuration

app/services/storage/storage_service.rb

@@ -15,5 +15,13 @@ module Storage
     def write_file(_file_name, _data)
       raise NotImplementedError
     end
+
+    def get_file(_file_name, _data)
+      raise NotImplementedError
+    end
+
+    def delete_file(_file_name, _data)
+      raise NotImplementedError
+    end
   end
 end

app/views/collection_resources/_collection_resource_summary_list.erb

@@ -35,15 +35,15 @@
           ) %>
           <% row.with_action(
             text: "Delete",
-            href: "/",
+            href: collection_resource_delete_confirmation_path(resource),
             classes: "app-!-colour-red"
           ) %>
         <% end %>
       <% end %>
     <% end %>
-    <div class="govuk-!-margin-bottom-6">
+    <div class="govuk-!-margin-bottom-8">
       <%= govuk_link_to "Add new #{mandatory_resources.first.log_type} #{text_year_range_format(mandatory_resources.first.year)} resource", href: new_collection_resource_path(year: mandatory_resources.first.year, log_type: mandatory_resources.first.log_type) %>
     </div>
-    <hr class="govuk-section-break govuk-section-break--visible govuk-section-break--m">
+    <hr class="govuk-section-break govuk-section-break--visible govuk-section-break--m govuk-!-margin-bottom-8">
   </div>
 </div>

app/views/collection_resources/delete_confirmation.html.erb

@@ -0,0 +1,31 @@
+<% content_for :before_content do %>
+  <% content_for :title, "Are you sure you want to delete the #{@collection_resource.short_display_name.downcase}?" %>
+  <%= govuk_back_link href: collection_resources_path %>
+<% end %>
+
+<div class="govuk-grid-row">
+  <div class="govuk-grid-column-two-thirds-from-desktop">
+    <span class="govuk-caption-l"><%= "#{@collection_resource.log_type.humanize} #{text_year_range_format(@collection_resource.year)}" %></span>
+    <h1 class="govuk-heading-xl">
+      <%= content_for(:title) %>
+    </h1>
+    <p class="govuk-body app-!-colour-muted">
+      This file will no longer be available for users to download.
+    </p>
+
+    <%= govuk_warning_text(text: "You will not be able to undo this action.") %>
+
+    <div class="govuk-button-group">
+      <%= govuk_button_to(
+        "Delete resource",
+        collection_resource_delete_path(@collection_resource),
+        method: :delete,
+      ) %>
+      <%= govuk_button_link_to(
+        "Cancel",
+        collection_resources_path,
+        secondary: true,
+      ) %>
+    </div>
+  </div>
+</div>

app/views/collection_resources/index.html.erb

@@ -14,16 +14,13 @@
 <% end %>
 <h1 class="govuk-heading-l"><%= title %></h1>
 
-<% @mandatory_lettings_collection_resources_per_year.each do |year, mandatory_resources| %>
+<% editable_collection_resource_years.each do |year| %>
   <h2 class="govuk-heading-m">
     Lettings <%= text_year_range_format(year) %>
   </h2>
-  <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_lettings_collection_resources_per_year[year] } %>
-<% end %>
-
-<% @mandatory_sales_collection_resources_per_year.each do |year, mandatory_resources| %>
+  <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: @mandatory_lettings_collection_resources_per_year[year], additional_resources: @additional_lettings_collection_resources_per_year[year] } %>
   <h2 class="govuk-heading-m">
     Sales <%= text_year_range_format(year) %>
   </h2>
-  <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_sales_collection_resources_per_year[year] } %>
+  <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: @mandatory_sales_collection_resources_per_year[year], additional_resources: @additional_sales_collection_resources_per_year[year] } %>
 <% end %>

config/routes.rb

@@ -51,6 +51,8 @@ Rails.application.routes.draw do
     get "/download", to: "collection_resources#download_additional_collection_resource"
     get "/edit", to: "collection_resources#edit_additional_collection_resource"
     patch "/update", to: "collection_resources#update_additional_collection_resource"
+    get "/delete-confirmation", to: "collection_resources#delete_confirmation"
+    delete "/delete", to: "collection_resources#delete"
   end
 
   get "clear-filters", to: "sessions#clear_filters"

db/migrate/20241011112158_add_discarded_at.rb

@@ -0,0 +1,5 @@
+class AddDiscardedAt < ActiveRecord::Migration[7.0]
+  def change
+    add_column :collection_resources, :discarded_at, :datetime
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_10_08_100119) do
+ActiveRecord::Schema[7.0].define(version: 2024_10_11_112158) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -61,6 +61,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_10_08_100119) do
     t.boolean "released_to_user"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.datetime "discarded_at"
   end
 
   create_table "csv_variable_definitions", force: :cascade do |t|

spec/requests/collection_resources_controller_spec.rb

@@ -2,7 +2,7 @@ require "rails_helper"
 
 RSpec.describe CollectionResourcesController, type: :request do
   let(:page) { Capybara::Node::Simple.new(response.body) }
-  let(:storage_service) { instance_double(Storage::S3Service, get_file_metadata: nil) }
+  let(:storage_service) { instance_double(Storage::S3Service, get_file_metadata: nil, delete_file: nil) }
 
   before do
     allow(Storage::S3Service).to receive(:new).and_return(storage_service)
@@ -183,7 +183,7 @@ RSpec.describe CollectionResourcesController, type: :request do
           expect(page).to have_content("additional resource")
           expect(page).not_to have_content("additional resource 2")
           expect(page).to have_link("additional.pdf", href: collection_resource_download_path(collection_resource))
-          expect(page).to have_link("Delete")
+          expect(page).to have_link("Delete", href: collection_resource_delete_confirmation_path(collection_resource))
         end
       end
     end
@@ -277,9 +277,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get edit_mandatory_collection_resource_path(year: 2024, log_type: "sales", resource_type: "bulk_upload_template")
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -290,9 +290,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get edit_mandatory_collection_resource_path(year: 2024, log_type: "sales", resource_type: "bulk_upload_template")
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -366,9 +366,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         patch update_mandatory_collection_resource_path, params: params
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -379,9 +379,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         patch update_mandatory_collection_resource_path, params: params
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
   end
@@ -401,9 +401,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get confirm_mandatory_collection_resources_release_path(year: 2025)
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -414,9 +414,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get confirm_mandatory_collection_resources_release_path(year: 2025)
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -466,9 +466,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         patch release_mandatory_collection_resources_path(year: 2024)
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -479,9 +479,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         patch release_mandatory_collection_resources_path(year: 2024)
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -528,9 +528,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get new_collection_resource_path(year: 2025, log_type: "sales")
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -541,9 +541,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get new_collection_resource_path(year: 2025, log_type: "sales")
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -589,9 +589,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         post collection_resources_path, params: params
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -602,9 +602,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         post collection_resources_path, params: params
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
   end
@@ -722,9 +722,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get collection_resource_edit_path(collection_resource)
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -735,9 +735,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         get collection_resource_edit_path(collection_resource)
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -794,9 +794,9 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         patch collection_resource_update_path(collection_resource), params: params
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
       end
     end
 
@@ -807,9 +807,132 @@ RSpec.describe CollectionResourcesController, type: :request do
         sign_in user
       end
 
-      it "returns page not found" do
+      it "returns page not authorised" do
         patch collection_resource_update_path(collection_resource), params: params
-        expect(response).to have_http_status(:not_found)
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+  end
+
+  describe "GET #collection_resource_delete_confirmation" do
+    let(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") }
+
+    context "when user is not signed in" do
+      it "redirects to the sign in page" do
+        get collection_resource_delete_confirmation_path(collection_resource)
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+
+    context "when user is signed in as a data coordinator" do
+      let(:user) { create(:user, :data_coordinator) }
+
+      before do
+        sign_in user
+      end
+
+      it "returns page not authorised" do
+        get collection_resource_delete_confirmation_path(collection_resource)
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    context "when user is signed in as a data provider" do
+      let(:user) { create(:user, :data_provider) }
+
+      before do
+        sign_in user
+      end
+
+      it "returns page not authorised" do
+        get collection_resource_delete_confirmation_path(collection_resource)
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    context "when user is signed in as a support user" do
+      let(:user) { create(:user, :support) }
+
+      before do
+        allow(Time.zone).to receive(:today).and_return(Time.zone.local(2025, 1, 8))
+        allow(user).to receive(:need_two_factor_authentication?).and_return(false)
+        sign_in user
+      end
+
+      context "and the file exists on S3" do
+        it "displays delete confirmation page content" do
+          get collection_resource_delete_confirmation_path(collection_resource)
+
+          expect(page).to have_content("Sales 2025 to 2026")
+          expect(page).to have_content("Are you sure you want to delete the additional resource?")
+          expect(page).to have_content("This file will no longer be available for users to download.")
+          expect(page).to have_content("You will not be able to undo this action.")
+          expect(page).to have_button("Delete resource")
+          expect(page).to have_link("Back", href: collection_resources_path)
+          expect(page).to have_link("Cancel", href: collection_resources_path)
+        end
+      end
+    end
+  end
+
+  describe "DELETE #collection_resource_delete" do
+    let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") }
+
+    context "when user is not signed in" do
+      it "redirects to the sign in page" do
+        delete collection_resource_delete_path(collection_resource)
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+
+    context "when user is signed in as a data coordinator" do
+      let(:user) { create(:user, :data_coordinator) }
+
+      before do
+        sign_in user
+      end
+
+      it "returns page not authorised" do
+        delete collection_resource_delete_path(collection_resource)
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    context "when user is signed in as a data provider" do
+      let(:user) { create(:user, :data_provider) }
+
+      before do
+        sign_in user
+      end
+
+      it "returns page not authorised" do
+        delete collection_resource_delete_path(collection_resource)
+        expect(response).to have_http_status(:unauthorized)
+      end
+    end
+
+    context "when user is signed in as a support user" do
+      let(:user) { create(:user, :support) }
+
+      before do
+        allow(storage_service).to receive(:file_exists?).and_return(true)
+        allow(Time.zone).to receive(:today).and_return(Time.zone.local(2025, 1, 8))
+        allow(user).to receive(:need_two_factor_authentication?).and_return(false)
+        sign_in user
+      end
+
+      context "and the file exists on S3" do
+        it "displays delete confirmation page content" do
+          expect(CollectionResource.visible.count).to eq(1)
+          delete collection_resource_delete_path(collection_resource)
+
+          expect(CollectionResource.count).to eq(1)
+          expect(CollectionResource.visible.count).to eq(0)
+          expect(response).to redirect_to(collection_resources_path)
+          expect(storage_service).to have_received(:delete_file).with(collection_resource.download_filename)
+          follow_redirect!
+          expect(page).to have_content("The sales 2025 to 2026 additional resource has been deleted.")
+        end
       end
     end
   end