From 1681c8c029ebb601205a114978ca3d985405e054 Mon Sep 17 00:00:00 2001 From: Kat Date: Thu, 10 Oct 2024 16:28:42 +0100 Subject: [PATCH] Allow updating additional resources --- .../collection_resources_controller.rb | 44 ++++++- .../_collection_resource_summary_list.erb | 22 +--- app/views/collection_resources/edit.html.erb | 13 +- app/views/collection_resources/new.html.erb | 2 +- config/routes.rb | 8 +- spec/factories/collection_resource.rb | 4 +- spec/features/collection_resources_spec.rb | 34 ++++- .../collection_resources_controller_spec.rb | 119 ++++++++++++++++++ 8 files changed, 219 insertions(+), 27 deletions(-) diff --git a/app/controllers/collection_resources_controller.rb b/app/controllers/collection_resources_controller.rb index 25b5a18d0..54d9ccad7 100644 --- a/app/controllers/collection_resources_controller.rb +++ b/app/controllers/collection_resources_controller.rb @@ -34,7 +34,7 @@ class CollectionResourcesController < ApplicationController download_resource(resource.download_filename) end - def edit + def edit_mandatory_collection_resource return render_not_found unless current_user.support? year = params[:year].to_i @@ -50,7 +50,18 @@ class CollectionResourcesController < ApplicationController render "collection_resources/edit" end - def update + def edit_additional_collection_resource + return render_not_found unless current_user.support? + + @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless @collection_resource + return render_not_found unless resource_for_year_can_be_updated?(@collection_resource.year) + + render "collection_resources/edit" + end + + def update_mandatory_collection_resource return render_not_found unless current_user.support? year = resource_params[:year].to_i @@ -80,6 +91,35 @@ class CollectionResourcesController < ApplicationController redirect_to collection_resources_path end + def update_additional_collection_resource + return render_not_found unless current_user.support? + + @collection_resource = CollectionResource.find_by(id: params[:collection_resource_id]) + + return render_not_found unless @collection_resource + return render_not_found unless resource_for_year_can_be_updated?(@collection_resource.year) + + @collection_resource.file = resource_params[:file] + @collection_resource.short_display_name = resource_params[:short_display_name] + @collection_resource.download_filename = @collection_resource.file&.original_filename + @collection_resource.display_name = "#{@collection_resource.log_type} #{@collection_resource.short_display_name} (#{text_year_range_format(@collection_resource.year)})" + @collection_resource.validate_attached_file + return render "collection_resources/edit" if @collection_resource.errors.any? + + if @collection_resource.save + begin + CollectionResourcesService.new.upload_collection_resource(@collection_resource.download_filename, @collection_resource.file) + flash[:notice] = "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.short_display_name.downcase} has been updated." + redirect_to collection_resources_path + rescue StandardError + @collection_resource.errors.add(:file, :error_uploading) + render "collection_resources/edit" + end + else + render "collection_resources/edit" + end + end + def confirm_mandatory_collection_resources_release return render_not_found unless current_user.support? diff --git a/app/views/collection_resources/_collection_resource_summary_list.erb b/app/views/collection_resources/_collection_resource_summary_list.erb index a08759fe1..1d82cc65b 100644 --- a/app/views/collection_resources/_collection_resource_summary_list.erb +++ b/app/views/collection_resources/_collection_resource_summary_list.erb @@ -26,23 +26,13 @@ <% additional_resources&.each do |resource| %> <% summary_list.with_row do |row| %> <% row.with_key { resource.short_display_name } %> - <% if file_exists_on_s3?(resource.download_filename) %> - <% row.with_value do %> - <%= render DocumentListComponent.new(items: document_list_edit_component_items([resource]), label: "") %> - <% end %> - <% row.with_action( - text: "Change", - href: "/", - ) %> - <% else %> - <% row.with_value do %> -

No file uploaded

- <% end %> - <% row.with_action( - text: "Upload", - href: "/", - ) %> + <% row.with_value do %> + <%= render DocumentListComponent.new(items: document_list_edit_component_items([resource]), label: "") %> <% end %> + <% row.with_action( + text: "Change", + href: collection_resource_edit_path(resource), + ) %> <% end %> <% end %> <% end %> diff --git a/app/views/collection_resources/edit.html.erb b/app/views/collection_resources/edit.html.erb index e5b2a6d33..7152cae82 100644 --- a/app/views/collection_resources/edit.html.erb +++ b/app/views/collection_resources/edit.html.erb @@ -5,7 +5,7 @@
<% resource_exists = file_exists_on_s3?(@collection_resource.download_filename) %> - <%= form_with model: @collection_resource, url: update_mandatory_collection_resource_path, method: :patch do |f| %> + <%= form_with model: @collection_resource, url: @collection_resource.mandatory ? update_mandatory_collection_resource_path : collection_resource_update_path(@collection_resource), method: :patch do |f| %> <%= f.hidden_field :year %> <%= f.hidden_field :log_type %> <%= f.hidden_field :resource_type %> @@ -13,7 +13,7 @@ <%= f.govuk_error_summary %> <%= "#{@collection_resource.log_type.humanize} #{text_year_range_format(@collection_resource.year)}" %> -

<%= resource_exists ? "Change" : "Upload" %> the <%= @collection_resource.resource_type.humanize.downcase %>

+

<%= resource_exists ? "Change" : "Upload" %> the <%= @collection_resource.short_display_name.downcase %>

This file will be available for all users to download. @@ -21,6 +21,15 @@ <%= f.govuk_file_field :file, label: { text: "Upload file", size: "m" } %> + <% if resource_exists %> +

Current file: <%= govuk_link_to @collection_resource.download_filename, href: @collection_resource.download_path %>

+ <% end %> + + <% unless @collection_resource.mandatory %> + <%= f.govuk_text_field :short_display_name, + label: nil, + hint: { text: "This will be used in the download link on the homepage. Do not include the log type or collection year.
For example, if you enter “bulk upload change log”, the download link will say “Download the #{@collection_resource.log_type} bulk upload change log (#{text_year_range_format(@collection_resource.year)})”.".html_safe } %> + <% end %> <%= f.govuk_submit resource_exists ? "Save changes" : "Upload" %> <%= govuk_button_link_to "Cancel", collection_resources_path, secondary: true %> diff --git a/app/views/collection_resources/new.html.erb b/app/views/collection_resources/new.html.erb index 984f9a3a8..4628a3869 100644 --- a/app/views/collection_resources/new.html.erb +++ b/app/views/collection_resources/new.html.erb @@ -23,7 +23,7 @@ <%= f.govuk_text_field :short_display_name, label: nil, - hint: { text: "This will be used in the download link on the homepage. Do not include the log type or collection year. For example, if you enter “bulk upload change log”, the download link will say “Download the lettings bulk upload change log (2024 to 2025)”." } %> + hint: { text: "This will be used in the download link on the homepage. Do not include the log type or collection year.
For example, if you enter “bulk upload change log”, the download link will say “Download the #{@collection_resource.log_type} bulk upload change log (#{text_year_range_format(@collection_resource.year)})”.".html_safe } %> <%= f.govuk_submit "Add resource" %> <%= govuk_button_link_to "Cancel", collection_resources_path, secondary: true %> diff --git a/config/routes.rb b/config/routes.rb index db4b2a8a2..cbac4894a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -42,13 +42,15 @@ Rails.application.routes.draw do get "collection-resources", to: "collection_resources#index" get "/collection-resources/:log_type/:year/:resource_type/download", to: "collection_resources#download_mandatory_collection_resource", as: :download_mandatory_collection_resource - get "/collection-resources/:log_type/:year/:resource_type/edit", to: "collection_resources#edit", as: :edit_mandatory_collection_resource - patch "/collection-resources", to: "collection_resources#update", as: :update_mandatory_collection_resource + get "/collection-resources/:log_type/:year/:resource_type/edit", to: "collection_resources#edit_mandatory_collection_resource", as: :edit_mandatory_collection_resource + patch "/collection-resources", to: "collection_resources#update_mandatory_collection_resource", as: :update_mandatory_collection_resource get "/collection-resources/:year/release", to: "collection_resources#confirm_mandatory_collection_resources_release", as: :confirm_mandatory_collection_resources_release patch "/collection-resources/:year/release", to: "collection_resources#release_mandatory_collection_resources", as: :release_mandatory_collection_resources resources :collection_resources, path: "/collection-resources" do - get "/download", to: "collection_resources#download_additional_collection_resource" # when we get to adding them + 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" end get "clear-filters", to: "sessions#clear_filters" diff --git a/spec/factories/collection_resource.rb b/spec/factories/collection_resource.rb index ca2715015..e65e4c496 100644 --- a/spec/factories/collection_resource.rb +++ b/spec/factories/collection_resource.rb @@ -8,8 +8,8 @@ FactoryBot.define do download_filename { "24_25_lettings_paper_form.pdf" } trait(:additional) do resource_type { nil } - display_name { "additional resource" } - short_display_name { nil } + display_name { "lettings additional resource (2024 to 2025)" } + short_display_name { "additional resource" } year { 2024 } log_type { "lettings" } download_filename { "additional.pdf" } diff --git a/spec/features/collection_resources_spec.rb b/spec/features/collection_resources_spec.rb index ac66b80b8..c6094d843 100644 --- a/spec/features/collection_resources_spec.rb +++ b/spec/features/collection_resources_spec.rb @@ -170,7 +170,7 @@ RSpec.describe "Collection resources" do end context "when uploading an additional resource" do - it "only allows excel files for lettings" do + it "allows valid files" do expect(CollectionResource.count).to eq(0) visit(new_collection_resource_path(year: 2025, log_type: "sales")) @@ -206,4 +206,36 @@ RSpec.describe "Collection resources" do expect(page).to have_content("You must answer resource type.") end end + + context "when updating an additional resource" do + let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales") } + + it "only allows valid files" do + expect(CollectionResource.count).to eq(1) + + visit(collection_resource_edit_path(collection_resource)) + fill_in("collection_resource[short_display_name]", with: "some updated file") + attach_file "file", file_fixture("pdf_file.pdf") + + click_button("Save changes") + expect(collection_resources_service).to have_received(:upload_collection_resource).with("pdf_file.pdf", anything) + expect(CollectionResource.count).to eq(1) + expect(CollectionResource.first.year).to eq(2025) + expect(CollectionResource.first.log_type).to eq("sales") + expect(CollectionResource.first.resource_type).to be_nil + expect(CollectionResource.first.mandatory).to be_falsey + expect(CollectionResource.first.released_to_user).to be_nil + expect(CollectionResource.first.display_name).to eq("sales some updated file (2025 to 2026)") + expect(CollectionResource.first.short_display_name).to eq("some updated file") + expect(page).to have_content("The sales 2025 to 2026 some updated file has been updated.") + end + + it "validates file is attached" do + visit(collection_resource_edit_path(collection_resource)) + + fill_in("collection_resource[short_display_name]", with: "some file") + click_button("Save changes") + expect(page).to have_content("Select which file to upload.") + end + end end diff --git a/spec/requests/collection_resources_controller_spec.rb b/spec/requests/collection_resources_controller_spec.rb index 62ff81cfb..41025a35c 100644 --- a/spec/requests/collection_resources_controller_spec.rb +++ b/spec/requests/collection_resources_controller_spec.rb @@ -122,6 +122,16 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("The 2025 to 2026 collection resources are not yet available to users.") expect(page).to have_link("Release the 2025 to 2026 collection resources to users", href: confirm_mandatory_collection_resources_release_path(year: 2025)) end + + context "when there are additional resources" do + let!(:collection_resource) { create(:collection_resource, :additional, year: 2025, short_display_name: "additional resource", download_filename: "additional.pdf") } + + it "displays change links for additional resources" do + get collection_resources_path + + expect(page).to have_link("Change", href: collection_resource_edit_path(collection_resource)) + end + end end context "when files are not on S3" do @@ -676,4 +686,113 @@ RSpec.describe CollectionResourcesController, type: :request do end end end + + describe "GET #edit_additional_collection_resource" 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_edit_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 found" do + get collection_resource_edit_path(collection_resource) + expect(response).to have_http_status(:not_found) + 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 found" do + get collection_resource_edit_path(collection_resource) + expect(response).to have_http_status(:not_found) + 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 + before do + allow(storage_service).to receive(:file_exists?).and_return(true) + end + + it "displays update collection resources page content" do + get collection_resource_edit_path(collection_resource) + + expect(page).to have_content("Sales 2025 to 2026") + expect(page).to have_content("Change the additional resource") + expect(page).to have_content("This file will be available for all users to download.") + expect(page).to have_content("Upload file") + expect(page).to have_button("Save changes") + 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 "PATCH #update_additional_collection_resource" do + let(:some_file) { File.open(file_fixture("blank_bulk_upload_sales.csv")) } + let(:params) { { collection_resource: { short_display_name: "short name", file: some_file } } } + let(:collection_resource_service) { instance_double(CollectionResourcesService) } + let(:collection_resource) { create(:collection_resource, :additional, year: 2025, log_type: "sales", short_display_name: "additional resource", download_filename: "additional.pdf") } + + before do + allow(CollectionResourcesService).to receive(:new).and_return(collection_resource_service) + end + + context "when user is not signed in" do + it "redirects to the sign in page" do + patch collection_resource_update_path(collection_resource), params: params + 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 found" do + patch collection_resource_update_path(collection_resource), params: params + expect(response).to have_http_status(:not_found) + 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 found" do + patch collection_resource_update_path(collection_resource), params: params + expect(response).to have_http_status(:not_found) + end + end + end end