diff --git a/app/components/document_list_component.html.erb b/app/components/document_list_component.html.erb index 32b2d1562..f2587f10f 100644 --- a/app/components/document_list_component.html.erb +++ b/app/components/document_list_component.html.erb @@ -1,4 +1,6 @@ -

<%= label %>

+<% unless label.blank? %> +

<%= label %>

+<% end %>
<% items.each do |item| %>
diff --git a/app/controllers/collection_resources_controller.rb b/app/controllers/collection_resources_controller.rb index 5203ceea1..06988c88c 100644 --- a/app/controllers/collection_resources_controller.rb +++ b/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 diff --git a/app/controllers/start_controller.rb b/app/controllers/start_controller.rb index b65da4d44..d6df81c39 100644 --- a/app/controllers/start_controller.rb +++ b/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" diff --git a/app/frontend/styles/_document-list.scss b/app/frontend/styles/_document-list.scss index e8cef0625..a1f812551 100644 --- a/app/frontend/styles/_document-list.scss +++ b/app/frontend/styles/_document-list.scss @@ -1,5 +1,4 @@ .app-document-list { - margin-top: govuk-spacing(3); margin-bottom: govuk-spacing(6); } diff --git a/app/models/collection_resource.rb b/app/models/collection_resource.rb index cc217b8cd..fc5808cec 100644 --- a/app/models/collection_resource.rb +++ b/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 diff --git a/app/services/collection_resources_service.rb b/app/services/collection_resources_service.rb index 82247f7a0..f6762116a 100644 --- a/app/services/collection_resources_service.rb +++ b/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 diff --git a/app/services/storage/local_disk_service.rb b/app/services/storage/local_disk_service.rb index ad3cc9608..228f0339e 100644 --- a/app/services/storage/local_disk_service.rb +++ b/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 diff --git a/app/services/storage/s3_service.rb b/app/services/storage/s3_service.rb index 2e8daa719..a6eef7d49 100644 --- a/app/services/storage/s3_service.rb +++ b/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 diff --git a/app/services/storage/storage_service.rb b/app/services/storage/storage_service.rb index afb3d4a58..37d4cc0fd 100644 --- a/app/services/storage/storage_service.rb +++ b/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 diff --git a/app/views/collection_resources/_collection_resource_summary_list.erb b/app/views/collection_resources/_collection_resource_summary_list.erb index 0e630f0aa..581bb6303 100644 --- a/app/views/collection_resources/_collection_resource_summary_list.erb +++ b/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 %> -
+
<%= 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) %>
-
+
diff --git a/app/views/collection_resources/delete_confirmation.html.erb b/app/views/collection_resources/delete_confirmation.html.erb new file mode 100644 index 000000000..9f861bc19 --- /dev/null +++ b/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 %> + +
+
+ <%= "#{@collection_resource.log_type.humanize} #{text_year_range_format(@collection_resource.year)}" %> +

+ <%= content_for(:title) %> +

+

+ This file will no longer be available for users to download. +

+ + <%= govuk_warning_text(text: "You will not be able to undo this action.") %> + +
+ <%= govuk_button_to( + "Delete resource", + collection_resource_delete_path(@collection_resource), + method: :delete, + ) %> + <%= govuk_button_link_to( + "Cancel", + collection_resources_path, + secondary: true, + ) %> +
+
+
diff --git a/app/views/collection_resources/index.html.erb b/app/views/collection_resources/index.html.erb index 207a63991..ee5574f80 100644 --- a/app/views/collection_resources/index.html.erb +++ b/app/views/collection_resources/index.html.erb @@ -14,16 +14,13 @@ <% end %>

<%= title %>

-<% @mandatory_lettings_collection_resources_per_year.each do |year, mandatory_resources| %> +<% editable_collection_resource_years.each do |year| %>

Lettings <%= text_year_range_format(year) %>

- <%= 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] } %>

Sales <%= text_year_range_format(year) %>

- <%= 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 %> diff --git a/config/routes.rb b/config/routes.rb index cbac4894a..6ac7b3f34 100644 --- a/config/routes.rb +++ b/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" diff --git a/db/migrate/20241011112158_add_discarded_at.rb b/db/migrate/20241011112158_add_discarded_at.rb new file mode 100644 index 000000000..d910b8797 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 1f5cca5ca..84bf048af 100644 --- a/db/schema.rb +++ b/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| diff --git a/spec/requests/collection_resources_controller_spec.rb b/spec/requests/collection_resources_controller_spec.rb index a6d88f911..eb6f56aed 100644 --- a/spec/requests/collection_resources_controller_spec.rb +++ b/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