From f047dd9bb4d38d62841227e741f8d37bf0804d8e Mon Sep 17 00:00:00 2001 From: Kat Date: Thu, 10 Oct 2024 13:17:09 +0100 Subject: [PATCH] Display and validate additional resources --- .../collection_resources_controller.rb | 49 +++++++------------ app/models/collection_resource.rb | 27 +++++++++- .../_collection_resource_summary_list.erb | 22 +++++++++ app/views/collection_resources/index.html.erb | 4 +- app/views/collection_resources/new.html.erb | 2 +- config/locales/en.yml | 3 ++ spec/factories/collection_resource.rb | 9 ++++ spec/features/collection_resources_spec.rb | 26 +++++++--- .../collection_resources_controller_spec.rb | 17 +++++++ 9 files changed, 118 insertions(+), 41 deletions(-) diff --git a/app/controllers/collection_resources_controller.rb b/app/controllers/collection_resources_controller.rb index d8c17ad2a..25791b39d 100644 --- a/app/controllers/collection_resources_controller.rb +++ b/app/controllers/collection_resources_controller.rb @@ -8,6 +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) end def download_mandatory_collection_resource @@ -52,7 +54,8 @@ class CollectionResourcesController < ApplicationController @collection_resource = MandatoryCollectionResourcesService.generate_resource(log_type, year, resource_type) render_not_found unless @collection_resource - validate_file(file) + @collection_resource.file = file + @collection_resource.validate_attached_file return render "collection_resources/edit" if @collection_resource.errors.any? @@ -107,26 +110,29 @@ class CollectionResourcesController < ApplicationController @collection_resource = CollectionResource.new(resource_params) @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)})" - validate_file(@collection_resource.file) + @collection_resource.validate_attached_file return render "collection_resources/new" if @collection_resource.errors.any? - begin - CollectionResourcesService.new.upload_collection_resource(@collection_resource.download_filename, @collection_resource.file) - @collection_resource.save! - rescue StandardError - @collection_resource.errors.add(:file, :error_uploading) - return render "collection_resources/new" + 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} is now available to users." + redirect_to collection_resources_path + rescue StandardError + @collection_resource.errors.add(:file, :error_uploading) + render "collection_resources/new" + end + else + render "collection_resources/new" end - - flash[:notice] = "The #{@collection_resource.log_type} #{text_year_range_format(@collection_resource.year)} #{@collection_resource.display_name} is now available to users." - redirect_to collection_resources_path end private def resource_params - params.require(:collection_resource).permit(:year, :log_type, :resource_type, :file, :mandatory, :display_name) + params.require(:collection_resource).permit(:year, :log_type, :resource_type, :file, :mandatory, :short_display_name) end def download_resource(filename) @@ -145,23 +151,4 @@ private def resource_for_year_can_be_updated?(year) editable_collection_resource_years.include?(year) end - - def validate_file(file) - return @collection_resource.errors.add(:file, :blank) unless file - return @collection_resource.errors.add(:file, :above_100_mb) if file.size > 100.megabytes - - argv = %W[file --brief --mime-type -- #{file.path}] - output = `#{argv.shelljoin}` - - case @collection_resource.resource_type - when "paper_form" - unless output.match?(/application\/pdf/) - @collection_resource.errors.add(:file, :must_be_pdf) - end - when "bulk_upload_template", "bulk_upload_specification" - unless output.match?(/application\/vnd\.ms-excel|application\/vnd\.openxmlformats-officedocument\.spreadsheetml\.sheet/) - @collection_resource.errors.add(:file, :must_be_xlsx, resource: @collection_resource.short_display_name.downcase) - end - end - end end diff --git a/app/models/collection_resource.rb b/app/models/collection_resource.rb index 81665ccf6..f193bd003 100644 --- a/app/models/collection_resource.rb +++ b/app/models/collection_resource.rb @@ -3,7 +3,32 @@ class CollectionResource < ApplicationRecord attr_accessor :file + validates :short_display_name, presence: true + def download_path - download_mandatory_collection_resource_path(log_type:, year:, resource_type:) + if mandatory + download_mandatory_collection_resource_path(log_type:, year:, resource_type:) + else + "/" + end + end + + def validate_attached_file + return errors.add(:file, :blank) unless file + return errors.add(:file, :above_100_mb) if file.size > 100.megabytes + + argv = %W[file --brief --mime-type -- #{file.path}] + output = `#{argv.shelljoin}` + + case resource_type + when "paper_form" + unless output.match?(/application\/pdf/) + errors.add(:file, :must_be_pdf) + end + when "bulk_upload_template", "bulk_upload_specification" + unless output.match?(/application\/vnd\.ms-excel|application\/vnd\.openxmlformats-officedocument\.spreadsheetml\.sheet/) + errors.add(:file, :must_be_xlsx, resource: short_display_name.downcase) + end + 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 d806d043e..a08759fe1 100644 --- a/app/views/collection_resources/_collection_resource_summary_list.erb +++ b/app/views/collection_resources/_collection_resource_summary_list.erb @@ -23,6 +23,28 @@ <% end %> <% end %> <% end %> + <% 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: "/", + ) %> + <% end %> + <% 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/index.html.erb b/app/views/collection_resources/index.html.erb index f6785eb46..207a63991 100644 --- a/app/views/collection_resources/index.html.erb +++ b/app/views/collection_resources/index.html.erb @@ -18,12 +18,12 @@

Lettings <%= text_year_range_format(year) %>

- <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: } %> + <%= 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| %>

Sales <%= text_year_range_format(year) %>

- <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources: } %> + <%= render partial: "collection_resource_summary_list", locals: { mandatory_resources:, additional_resources: @additional_sales_collection_resources_per_year[year] } %> <% end %> diff --git a/app/views/collection_resources/new.html.erb b/app/views/collection_resources/new.html.erb index 006636b26..984f9a3a8 100644 --- a/app/views/collection_resources/new.html.erb +++ b/app/views/collection_resources/new.html.erb @@ -21,7 +21,7 @@ <%= f.govuk_file_field :file, label: { text: "Upload file", size: "m" } %> - <%= f.govuk_text_field :display_name, + <%= 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)”." } %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 7aef84fea..ce3cb518e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -215,6 +215,9 @@ en: above_100_mb: The file is above 100MB. must_be_pdf: The paper form must be a PDF. must_be_xlsx: The %{resource} must be a Microsoft Excel file. + short_display_name: + blank: "You must answer resource type." + notification: logs_deleted: one: "%{count} log has been deleted." diff --git a/spec/factories/collection_resource.rb b/spec/factories/collection_resource.rb index c352b70a9..ca2715015 100644 --- a/spec/factories/collection_resource.rb +++ b/spec/factories/collection_resource.rb @@ -6,5 +6,14 @@ FactoryBot.define do year { 2024 } log_type { "lettings" } download_filename { "24_25_lettings_paper_form.pdf" } + trait(:additional) do + resource_type { nil } + display_name { "additional resource" } + short_display_name { nil } + year { 2024 } + log_type { "lettings" } + download_filename { "additional.pdf" } + mandatory { false } + end end end diff --git a/spec/features/collection_resources_spec.rb b/spec/features/collection_resources_spec.rb index 8435fa914..ac66b80b8 100644 --- a/spec/features/collection_resources_spec.rb +++ b/spec/features/collection_resources_spec.rb @@ -174,12 +174,9 @@ RSpec.describe "Collection resources" do expect(CollectionResource.count).to eq(0) visit(new_collection_resource_path(year: 2025, log_type: "sales")) - - click_button("Add resource") - expect(page).to have_content("Select which file to upload") - + fill_in("collection_resource[short_display_name]", with: "some file") attach_file "file", file_fixture("pdf_file.pdf") - fill_in("collection_resource[display_name]", with: "some file") + click_button("Add resource") expect(collection_resources_service).to have_received(:upload_collection_resource).with("pdf_file.pdf", anything) expect(CollectionResource.count).to eq(1) @@ -188,8 +185,25 @@ RSpec.describe "Collection resources" do 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("some file") + expect(CollectionResource.first.display_name).to eq("sales some file (2025 to 2026)") + expect(CollectionResource.first.short_display_name).to eq("some file") expect(page).to have_content("The sales 2025 to 2026 some file is now available to users.") end + + it "validates file is attached" do + visit(new_collection_resource_path(year: 2025, log_type: "sales")) + + fill_in("collection_resource[short_display_name]", with: "some file") + click_button("Add resource") + expect(page).to have_content("Select which file to upload.") + end + + it "validates resource type is given" do + visit(new_collection_resource_path(year: 2025, log_type: "sales")) + + attach_file "file", file_fixture("pdf_file.pdf") + click_button("Add resource") + expect(page).to have_content("You must answer resource type.") + end end end diff --git a/spec/requests/collection_resources_controller_spec.rb b/spec/requests/collection_resources_controller_spec.rb index 5877c67fd..39d490387 100644 --- a/spec/requests/collection_resources_controller_spec.rb +++ b/spec/requests/collection_resources_controller_spec.rb @@ -156,6 +156,23 @@ RSpec.describe CollectionResourcesController, type: :request do expect(page).to have_content("Once you have uploaded all the required 2025 to 2026 collection resources, you will be able to release them to users.") end end + + context "when there are additional resources" do + before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(CollectionResourcesHelper).to receive(:editable_collection_resource_years).and_return([2025]) + # rubocop:enable RSpec/AnyInstance + create(:collection_resource, :additional, year: 2025, short_display_name: "additional resource") + create(:collection_resource, :additional, year: 2026, short_display_name: "additional resource 2") + end + + it "displays additional resources for editable years" do + get collection_resources_path + + expect(page).to have_content("additional resource") + expect(page).not_to have_content("additional resource 2") + end + end end end