From a92fb1030c2dd449ca8e729fb8f86fccd9a77f6d Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 13 Sep 2022 13:49:16 +0100 Subject: [PATCH] CLDC-1362 Improve CSV download performance (#851) * Replaced log CSV direct download with email * Tidy up authorization of organisations controller - We already have a method to authenticate the scope of the user, so we can reuse that. * Use Rails routes instead of absolute paths for CSV download links * Introduce base NotifyMailer to to abstract away shared Notify mail functionality * Fix mailer spec name * Add worker instance to PaaS manifest * Add CSV download bucket instance name into environment * Update tests for improved search term handling * Fix download mailer tests * Clarifying comments Co-authored-by: natdeanlewissoftwire Co-authored-by: James Rose --- .github/workflows/production_pipeline.yml | 2 + .github/workflows/staging_pipeline.yml | 2 + Gemfile | 2 + Gemfile.lock | 5 + app/controllers/lettings_logs_controller.rb | 29 +++- .../modules/lettings_logs_filter.rb | 30 ++-- app/controllers/modules/search_filter.rb | 8 +- app/controllers/organisations_controller.rb | 43 ++--- app/jobs/email_csv_job.rb | 21 +++ app/mailers/csv_download_mailer.rb | 11 ++ app/mailers/notify_mailer.rb | 37 ++++ app/services/filter_service.rb | 22 +++ app/services/storage/s3_service.rb | 6 + app/views/lettings_logs/_log_list.html.erb | 2 +- .../lettings_logs/csv_confirmation.html.erb | 15 ++ app/views/lettings_logs/download_csv.html.erb | 16 ++ app/views/lettings_logs/index.html.erb | 2 +- app/views/organisations/logs.html.erb | 2 +- config/application.rb | 2 + config/routes.rb | 6 + manifest.yml | 4 + spec/jobs/email_csv_job_spec.rb | 159 ++++++++++++++++++ spec/mailers/csv_download_mailer_spec.rb | 30 ++++ spec/rails_helper.rb | 1 + .../requests/lettings_logs_controller_spec.rb | 150 +++++++++-------- .../requests/organisations_controller_spec.rb | 63 ++++--- spec/services/filter_service_spec.rb | 28 +++ 27 files changed, 552 insertions(+), 146 deletions(-) create mode 100644 app/jobs/email_csv_job.rb create mode 100644 app/mailers/csv_download_mailer.rb create mode 100644 app/mailers/notify_mailer.rb create mode 100644 app/services/filter_service.rb create mode 100644 app/views/lettings_logs/csv_confirmation.html.erb create mode 100644 app/views/lettings_logs/download_csv.html.erb create mode 100644 spec/jobs/email_csv_job_spec.rb create mode 100644 spec/mailers/csv_download_mailer_spec.rb create mode 100644 spec/services/filter_service_spec.rb diff --git a/.github/workflows/production_pipeline.yml b/.github/workflows/production_pipeline.yml index 333e1f87b..4400d9239 100644 --- a/.github/workflows/production_pipeline.yml +++ b/.github/workflows/production_pipeline.yml @@ -238,6 +238,7 @@ jobs: RAILS_MASTER_KEY: ${{ secrets.RAILS_MASTER_KEY }} IMPORT_PAAS_INSTANCE: ${{ secrets.IMPORT_PAAS_INSTANCE }} EXPORT_PAAS_INSTANCE: ${{ secrets.EXPORT_PAAS_INSTANCE }} + CSV_DOWNLOAD_PAAS_INSTANCE: ${{ secrets.CSV_DOWNLOAD_PAAS_INSTANCE }} SENTRY_DSN: ${{ secrets.SENTRY_DSN }} run: | cf api $CF_API_ENDPOINT @@ -248,5 +249,6 @@ jobs: cf set-env $APP_NAME RAILS_MASTER_KEY $RAILS_MASTER_KEY cf set-env $APP_NAME IMPORT_PAAS_INSTANCE $IMPORT_PAAS_INSTANCE cf set-env $APP_NAME EXPORT_PAAS_INSTANCE $EXPORT_PAAS_INSTANCE + cf set-env $APP_NAME CSV_DOWNLOAD_PAAS_INSTANCE $CSV_DOWNLOAD_PAAS_INSTANCE cf set-env $APP_NAME SENTRY_DSN $SENTRY_DSN cf push $APP_NAME --strategy rolling diff --git a/.github/workflows/staging_pipeline.yml b/.github/workflows/staging_pipeline.yml index 4c588e919..72d561652 100644 --- a/.github/workflows/staging_pipeline.yml +++ b/.github/workflows/staging_pipeline.yml @@ -214,6 +214,7 @@ jobs: RAILS_MASTER_KEY: ${{ secrets.RAILS_MASTER_KEY }} IMPORT_PAAS_INSTANCE: ${{ secrets.IMPORT_PAAS_INSTANCE }} EXPORT_PAAS_INSTANCE: ${{ secrets.EXPORT_PAAS_INSTANCE }} + CSV_DOWNLOAD_PAAS_INSTANCE: ${{ secrets.CSV_DOWNLOAD_PAAS_INSTANCE }} SENTRY_DSN: ${{ secrets.SENTRY_DSN }} run: | cf api $CF_API_ENDPOINT @@ -226,5 +227,6 @@ jobs: cf set-env $APP_NAME RAILS_MASTER_KEY $RAILS_MASTER_KEY cf set-env $APP_NAME IMPORT_PAAS_INSTANCE $IMPORT_PAAS_INSTANCE cf set-env $APP_NAME EXPORT_PAAS_INSTANCE $EXPORT_PAAS_INSTANCE + cf set-env $APP_NAME CSV_DOWNLOAD_PAAS_INSTANCE $CSV_DOWNLOAD_PAAS_INSTANCE cf set-env $APP_NAME SENTRY_DSN $SENTRY_DSN cf push $APP_NAME --strategy rolling diff --git a/Gemfile b/Gemfile index ad3e1893d..e6dac1995 100644 --- a/Gemfile +++ b/Gemfile @@ -58,6 +58,8 @@ gem "sentry-ruby" gem "possessive" # Strip whitespace from active record attributes gem "auto_strip_attributes" +# Use sidekiq for background processing +gem "sidekiq" group :development, :test do # Check gems for known vulnerabilities diff --git a/Gemfile.lock b/Gemfile.lock index 4f4b352bb..bd7111af6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -381,6 +381,10 @@ GEM sentry-ruby (~> 5.4.2) sentry-ruby (5.4.2) concurrent-ruby (~> 1.0, >= 1.0.2) + sidekiq (6.5.4) + connection_pool (>= 2.2.2) + rack (~> 2.0) + redis (>= 4.5.0) simplecov (0.21.2) docile (~> 1.1) simplecov-html (~> 0.11) @@ -470,6 +474,7 @@ DEPENDENCIES selenium-webdriver sentry-rails sentry-ruby + sidekiq simplecov stimulus-rails timecop (~> 0.9.4) diff --git a/app/controllers/lettings_logs_controller.rb b/app/controllers/lettings_logs_controller.rb index a19fdbe6d..651dd7225 100644 --- a/app/controllers/lettings_logs_controller.rb +++ b/app/controllers/lettings_logs_controller.rb @@ -7,23 +7,20 @@ class LettingsLogsController < ApplicationController before_action :authenticate, if: :json_api_request? before_action :authenticate_user!, unless: :json_api_request? before_action :find_resource, except: %i[create index edit] + before_action :session_filters, if: :current_user + before_action :set_session_filters, if: :current_user def index - set_session_filters - - all_logs = current_user.lettings_logs - unpaginated_filtered_logs = filtered_lettings_logs(filtered_collection(all_logs, search_term)) - respond_to do |format| format.html do + all_logs = current_user.lettings_logs + unpaginated_filtered_logs = filtered_lettings_logs(all_logs, search_term, @session_filters) + + @search_term = search_term @pagy, @lettings_logs = pagy(unpaginated_filtered_logs) @searched = search_term.presence @total_count = all_logs.size end - - format.csv do - send_data byte_order_mark + unpaginated_filtered_logs.to_csv(current_user), filename: "logs-#{Time.zone.now}.csv" - end end end @@ -91,6 +88,20 @@ class LettingsLogsController < ApplicationController end end + def download_csv + unpaginated_filtered_logs = filtered_lettings_logs(current_user.lettings_logs, search_term, @session_filters) + + render "download_csv", locals: { search_term:, count: unpaginated_filtered_logs.size, post_path: email_csv_lettings_logs_path } + end + + def email_csv + all_orgs = params["organisation_select"] == "all" + EmailCsvJob.perform_later(current_user, search_term, @session_filters, all_orgs) + redirect_to csv_confirmation_lettings_logs_path + end + + def csv_confirmation; end + private API_ACTIONS = %w[create show update destroy].freeze diff --git a/app/controllers/modules/lettings_logs_filter.rb b/app/controllers/modules/lettings_logs_filter.rb index c074e48bb..200a0db41 100644 --- a/app/controllers/modules/lettings_logs_filter.rb +++ b/app/controllers/modules/lettings_logs_filter.rb @@ -1,23 +1,21 @@ module Modules::LettingsLogsFilter - def filtered_lettings_logs(logs) - if session[:lettings_logs_filters].present? - filters = JSON.parse(session[:lettings_logs_filters]) - filters.each do |category, values| - next if Array(values).reject(&:empty?).blank? - next if category == "organisation" && params["organisation_select"] == "all" - - logs = logs.public_send("filter_by_#{category}", values, current_user) - end - end - logs = logs.order(created_at: :desc) - current_user.support? ? logs.all.includes(:owning_organisation, :managing_organisation) : logs + def filtered_lettings_logs(logs, search_term, filters) + all_orgs = params["organisation_select"] == "all" + FilterService.filter_lettings_logs(logs, search_term, filters, all_orgs, current_user) end - def set_session_filters(specific_org: false) - new_filters = session[:lettings_logs_filters].present? ? JSON.parse(session[:lettings_logs_filters]) : {} + def load_session_filters(specific_org: false) + current_filters = session[:lettings_logs_filters] + new_filters = current_filters.present? ? JSON.parse(current_filters) : {} current_user.lettings_logs_filters(specific_org:).each { |filter| new_filters[filter] = params[filter] if params[filter].present? } - new_filters = new_filters.except("organisation") if params["organisation_select"] == "all" + params["organisation_select"] == "all" ? new_filters.except("organisation") : new_filters + end + + def session_filters(specific_org: false) + @session_filters ||= load_session_filters(specific_org:) + end - session[:lettings_logs_filters] = new_filters.to_json + def set_session_filters + session[:lettings_logs_filters] = @session_filters.to_json end end diff --git a/app/controllers/modules/search_filter.rb b/app/controllers/modules/search_filter.rb index c32298987..82bf0d6c0 100644 --- a/app/controllers/modules/search_filter.rb +++ b/app/controllers/modules/search_filter.rb @@ -1,13 +1,9 @@ module Modules::SearchFilter def filtered_collection(base_collection, search_term = nil) - if search_term.present? - base_collection.search_by(search_term) - else - base_collection - end + FilterService.filter_by_search(base_collection, search_term) end def filtered_users(base_collection, search_term = nil) - filtered_collection(base_collection, search_term).includes(:organisation) + FilterService.filter_by_search(base_collection, search_term).includes(:organisation) end end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 1cf181788..3a979b78a 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -6,6 +6,8 @@ class OrganisationsController < ApplicationController before_action :authenticate_user! before_action :find_resource, except: %i[index new create] before_action :authenticate_scope!, except: [:index] + before_action -> { session_filters(specific_org: true) }, if: -> { current_user.support? } + before_action :set_session_filters, if: -> { current_user.support? } def index redirect_to organisation_path(current_user.organisation) unless current_user.support? @@ -88,29 +90,32 @@ class OrganisationsController < ApplicationController end def logs - if current_user.support? - set_session_filters(specific_org: true) - - organisation_logs = LettingsLog.all.where(owning_organisation_id: @organisation.id) - unpaginated_filtered_logs = filtered_lettings_logs(filtered_collection(organisation_logs, search_term)) - - respond_to do |format| - format.html do - @pagy, @lettings_logs = pagy(unpaginated_filtered_logs) - @searched = search_term.presence - @total_count = organisation_logs.size - render "logs", layout: "application" - end + organisation_logs = LettingsLog.all.where(owning_organisation_id: @organisation.id) + unpaginated_filtered_logs = filtered_lettings_logs(organisation_logs, search_term, @session_filters) - format.csv do - send_data byte_order_mark + unpaginated_filtered_logs.to_csv, filename: "logs-#{@organisation.name}-#{Time.zone.now}.csv" - end + respond_to do |format| + format.html do + @search_term = search_term + @pagy, @lettings_logs = pagy(unpaginated_filtered_logs) + @searched = search_term.presence + @total_count = organisation_logs.size + render "logs", layout: "application" end - else - redirect_to(lettings_logs_path) end end + def download_csv + organisation_logs = LettingsLog.all.where(owning_organisation_id: @organisation.id) + unpaginated_filtered_logs = filtered_lettings_logs(organisation_logs, search_term, @session_filters) + + render "lettings_logs/download_csv", locals: { search_term:, count: unpaginated_filtered_logs.size, post_path: logs_email_csv_organisation_path } + end + + def email_csv + EmailCsvJob.perform_later(current_user, search_term, @session_filters, false, @organisation) + redirect_to logs_csv_confirmation_organisation_path + end + private def org_params @@ -122,7 +127,7 @@ private end def authenticate_scope! - if %w[create new].include? action_name + if %w[create new logs download_csv email_csv].include? action_name head :unauthorized and return unless current_user.support? elsif current_user.organisation != @organisation && !current_user.support? render_not_found diff --git a/app/jobs/email_csv_job.rb b/app/jobs/email_csv_job.rb new file mode 100644 index 000000000..033ead7ae --- /dev/null +++ b/app/jobs/email_csv_job.rb @@ -0,0 +1,21 @@ +class EmailCsvJob < ApplicationJob + queue_as :default + + BYTE_ORDER_MARK = "\uFEFF".freeze # Required to ensure Excel always reads CSV as UTF-8 + + EXPIRATION_TIME = 3.hours.to_i + + def perform(user, search_term = nil, filters = {}, all_orgs = false, organisation = nil) # rubocop:disable Style/OptionalBooleanParameter - sidekiq can't serialise named params + unfiltered_logs = organisation.present? && user.support? ? LettingsLog.where(owning_organisation_id: organisation.id) : user.lettings_logs + filtered_logs = FilterService.filter_lettings_logs(unfiltered_logs, search_term, filters, all_orgs, user) + + filename = organisation.present? ? "logs-#{organisation.name}-#{Time.zone.now}.csv" : "logs-#{Time.zone.now}.csv" + + storage_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["CSV_DOWNLOAD_PAAS_INSTANCE"]) + storage_service.write_file(filename, BYTE_ORDER_MARK + filtered_logs.to_csv(user)) + + url = storage_service.get_presigned_url(filename, EXPIRATION_TIME) + + CsvDownloadMailer.new.send_email(user, url, EXPIRATION_TIME) + end +end diff --git a/app/mailers/csv_download_mailer.rb b/app/mailers/csv_download_mailer.rb new file mode 100644 index 000000000..619d5e922 --- /dev/null +++ b/app/mailers/csv_download_mailer.rb @@ -0,0 +1,11 @@ +class CsvDownloadMailer < NotifyMailer + CSV_DOWNLOAD_TEMPLATE_ID = "7890e3b9-8c0d-4d08-bafe-427fd7cd95bf".freeze + + def send_csv_download_mail(user, link, duration) + send_email( + user.email, + CSV_DOWNLOAD_TEMPLATE_ID, + { name: user.name, link:, duration: ActiveSupport::Duration.build(duration).inspect }, + ) + end +end diff --git a/app/mailers/notify_mailer.rb b/app/mailers/notify_mailer.rb new file mode 100644 index 000000000..506df5818 --- /dev/null +++ b/app/mailers/notify_mailer.rb @@ -0,0 +1,37 @@ +class NotifyMailer + require "notifications/client" + + def notify_client + @notify_client ||= ::Notifications::Client.new(ENV["GOVUK_NOTIFY_API_KEY"]) + end + + def send_email(email, template_id, personalisation) + return true if intercept_send?(email) + + notify_client.send_email( + email_address: email, + template_id:, + personalisation:, + ) + end + + def personalisation(record, token, url, username: false) + { + name: record.name || record.email, + email: username || record.email, + organisation: record.respond_to?(:organisation) ? record.organisation.name : "", + link: "#{url}#{token}", + } + end + + def intercept_send?(email) + return false unless email_allowlist + + email_domain = email.split("@").last.downcase + !(Rails.env.production? || Rails.env.test?) && email_allowlist.exclude?(email_domain) + end + + def email_allowlist + Rails.application.credentials[:email_allowlist] + end +end diff --git a/app/services/filter_service.rb b/app/services/filter_service.rb new file mode 100644 index 000000000..0986e7aca --- /dev/null +++ b/app/services/filter_service.rb @@ -0,0 +1,22 @@ +class FilterService + def self.filter_by_search(base_collection, search_term = nil) + if search_term.present? + base_collection.search_by(search_term) + else + base_collection + end + end + + def self.filter_lettings_logs(logs, search_term, filters, all_orgs, user) + logs = filter_by_search(logs, search_term) + + filters.each do |category, values| + next if Array(values).reject(&:empty?).blank? + next if category == "organisation" && all_orgs + + logs = logs.public_send("filter_by_#{category}", values, user) + end + logs = logs.order(created_at: :desc) + user.support? ? logs.all.includes(:owning_organisation, :managing_organisation) : logs + end +end diff --git a/app/services/storage/s3_service.rb b/app/services/storage/s3_service.rb index aee76398d..c972225a1 100644 --- a/app/services/storage/s3_service.rb +++ b/app/services/storage/s3_service.rb @@ -20,6 +20,12 @@ module Storage response.key_count == 1 end + def get_presigned_url(file_name, duration) + Aws::S3::Presigner + .new({ client: @client }) + .presigned_url(:get_object, bucket: @configuration.bucket_name, key: file_name, expires_in: duration) + end + def get_file_io(file_name) @client.get_object(bucket: @configuration.bucket_name, key: file_name) .body diff --git a/app/views/lettings_logs/_log_list.html.erb b/app/views/lettings_logs/_log_list.html.erb index 93d365c40..c6ea5e9c3 100644 --- a/app/views/lettings_logs/_log_list.html.erb +++ b/app/views/lettings_logs/_log_list.html.erb @@ -1,6 +1,6 @@

<%= render(SearchResultCaptionComponent.new(searched:, count: pagy.count, item_label:, total_count:, item: "logs", path: request.path)) %> - <%= govuk_link_to "Download (CSV)", "#{request.path}.csv", type: "text/csv" %> + <%= govuk_link_to "Download (CSV)", csv_download_url, type: "text/csv" %>

<% lettings_logs.map do |log| %> diff --git a/app/views/lettings_logs/csv_confirmation.html.erb b/app/views/lettings_logs/csv_confirmation.html.erb new file mode 100644 index 000000000..a5717cae9 --- /dev/null +++ b/app/views/lettings_logs/csv_confirmation.html.erb @@ -0,0 +1,15 @@ +<% content_for :title, "We've sending you an email" %> +
+
+ <%= govuk_panel(title_text: "We're sending you an email") %> + +

It should arrive in a few minutes, but it could take longer.

+ +

What happens next

+

Open your email inbox and click the link to download your CSV file.

+ +

+ <%= govuk_link_to "Return to logs", lettings_logs_path %> +

+
+
diff --git a/app/views/lettings_logs/download_csv.html.erb b/app/views/lettings_logs/download_csv.html.erb new file mode 100644 index 000000000..4e0da2704 --- /dev/null +++ b/app/views/lettings_logs/download_csv.html.erb @@ -0,0 +1,16 @@ +<% content_for :title, "Download CSV" %> + +<% content_for :before_content do %> + <%= govuk_back_link(href: :back) %> +<% end %> + +
+
+

Download CSV

+ +

We'll send a secure download link to your email address <%= @current_user.email %>.

+

You've selected <%= count %> logs.

+ + <%= govuk_button_to "Send email", post_path, method: :post, params: { search: search_term } %> +
+
diff --git a/app/views/lettings_logs/index.html.erb b/app/views/lettings_logs/index.html.erb index 9378aee09..fe1004e69 100644 --- a/app/views/lettings_logs/index.html.erb +++ b/app/views/lettings_logs/index.html.erb @@ -15,7 +15,7 @@
<%= render SearchComponent.new(current_user:, search_label: "Search by log ID, tenant code, property reference or postcode", value: @searched) %> <%= govuk_section_break(visible: true, size: "m") %> - <%= render partial: "log_list", locals: { lettings_logs: @lettings_logs, title: "Logs", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> + <%= render partial: "log_list", locals: { lettings_logs: @lettings_logs, title: "Logs", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count, csv_download_url: csv_download_lettings_logs_path(search: @search_term) } %> <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %>
diff --git a/app/views/organisations/logs.html.erb b/app/views/organisations/logs.html.erb index 916ac65ae..42c1697bc 100644 --- a/app/views/organisations/logs.html.erb +++ b/app/views/organisations/logs.html.erb @@ -21,7 +21,7 @@
<%= render SearchComponent.new(current_user:, search_label: "Search by log ID, tenant code, property reference or postcode", value: @searched) %> <%= govuk_section_break(visible: true, size: "m") %> - <%= render partial: "lettings_logs/log_list", locals: { lettings_logs: @lettings_logs, title: "Logs", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count } %> + <%= render partial: "lettings_logs/log_list", locals: { lettings_logs: @lettings_logs, title: "Logs", pagy: @pagy, searched: @searched, item_label:, total_count: @total_count, csv_download_url: logs_csv_download_organisation_path(@organisation, search: @search_term) } %> <%== render partial: "pagy/nav", locals: { pagy: @pagy, item_name: "logs" } %>
diff --git a/config/application.rb b/config/application.rb index 7bdbfb93e..153782f54 100644 --- a/config/application.rb +++ b/config/application.rb @@ -33,5 +33,7 @@ module DataCollector # config.eager_load_paths << Rails.root.join("extras") config.exceptions_app = routes + + config.active_job.queue_adapter = :sidekiq end end diff --git a/config/routes.rb b/config/routes.rb index 5a2fda475..82fec6493 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -69,6 +69,9 @@ Rails.application.routes.draw do get "users", to: "organisations#users" get "users/invite", to: "users/account#new" get "logs", to: "organisations#logs" + get "logs/csv-download", to: "organisations#download_csv" + post "logs/email-csv", to: "organisations#email_csv" + get "logs/csv-confirmation", to: "lettings_logs#csv_confirmation" get "schemes", to: "organisations#schemes" end end @@ -77,6 +80,9 @@ Rails.application.routes.draw do collection do post "bulk-upload", to: "bulk_upload#bulk_upload" get "bulk-upload", to: "bulk_upload#show" + get "csv-download", to: "lettings_logs#download_csv" + post "email-csv", to: "lettings_logs#email_csv" + get "csv-confirmation", to: "lettings_logs#csv_confirmation" end member do diff --git a/manifest.yml b/manifest.yml index be4095eff..f84feb810 100644 --- a/manifest.yml +++ b/manifest.yml @@ -7,6 +7,10 @@ defaults: &defaults command: bundle exec rake cf:on_first_instance db:migrate && bin/rails server instances: 2 memory: 1G + - type: worker + command: bundle exec sidekiq + health-check-type: process + instances: 2 health-check-type: http health-check-http-endpoint: /health diff --git a/spec/jobs/email_csv_job_spec.rb b/spec/jobs/email_csv_job_spec.rb new file mode 100644 index 000000000..ae3e14b90 --- /dev/null +++ b/spec/jobs/email_csv_job_spec.rb @@ -0,0 +1,159 @@ +require "rails_helper" + +describe EmailCsvJob do + include Helpers + + test_url = :test_url + + let(:job) { described_class.new } + let(:user) { FactoryBot.create(:user) } + let(:organisation) { user.organisation } + let(:other_organisation) { FactoryBot.create(:organisation) } + + context "when a log exists" do + let!(:lettings_log) do + FactoryBot.create( + :lettings_log, + owning_organisation: organisation, + managing_organisation: organisation, + ecstat1: 1, + ) + end + + let(:storage_service) { instance_double(Storage::S3Service) } + let(:mailer) { instance_double(CsvDownloadMailer) } + + before do + FactoryBot.create(:lettings_log, + :completed, + owning_organisation: organisation, + managing_organisation: organisation, + created_by: user) + + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(storage_service).to receive(:write_file) + allow(storage_service).to receive(:get_presigned_url).and_return(test_url) + + allow(CsvDownloadMailer).to receive(:new).and_return(mailer) + allow(mailer).to receive(:send_email) + end + + it "uses an appropriate filename in S3" do + expect(storage_service).to receive(:write_file).with(/logs-.*\.csv/, anything) + job.perform(user) + end + + it "includes the organisation name in the filename when one is provided" do + expect(storage_service).to receive(:write_file).with(/logs-#{organisation.name}-.*\.csv/, anything) + job.perform(user, nil, {}, nil, organisation) + end + + it "sends an E-mail with the presigned URL and duration" do + expect(mailer).to receive(:send_email).with(user, test_url, instance_of(Integer)) + job.perform(user) + end + + context "when writing to S3" do + before do + FactoryBot.create_list(:lettings_log, 4, owning_organisation: other_organisation) + end + + def expect_csv + expect(storage_service).to receive(:write_file) do |_filename, data| + # Ignore byte order marker + csv = CSV.parse(data[1..]) + yield(csv) + end + end + + it "writes CSV data with headers" do + expect_csv do |csv| + expect(csv.first.first).to eq("id") + expect(csv.second.first).to eq(lettings_log.id.to_s) + end + + job.perform(user) + end + + context "when there is no organisation provided" do + it "only writes logs from the user's organisation" do + expect_csv do |csv| + # Headings + 2 rows + expect(csv.count).to eq(3) + end + + job.perform(user) + end + end + + context "when the user is support and an organisation is provided" do + let(:user) { FactoryBot.create(:user, :support) } + + it "only writes logs from that organisation" do + expect_csv do |csv| + # other organisation => Headings + 4 rows + expect(csv.count).to eq(5) + end + + job.perform(user, nil, {}, nil, other_organisation) + end + end + + it "writes answer labels rather than values" do + expect_csv do |csv| + expect(csv.second[15]).to eq("Full-time – 30 hours or more") + end + + job.perform(user) + end + + it "writes filtered logs" do + expect_csv do |csv| + expect(csv.count).to eq(2) + end + + job.perform(user, nil, { status: "completed" }) + end + + it "writes searched logs" do + expect_csv do |csv| + expect(csv.count).to eq(LettingsLog.search_by(lettings_log.id.to_s).count + 1) + end + + job.perform(user, lettings_log.id.to_s) + end + + context "when both filter and search applied" do + let(:postcode) { "XX1 1TG" } + + before do + FactoryBot.create(:lettings_log, :in_progress, postcode_full: postcode, owning_organisation: organisation, created_by: user) + FactoryBot.create(:lettings_log, :completed, postcode_full: postcode, owning_organisation: organisation, created_by: user) + end + + it "downloads logs matching both csv and filter logs" do + expect_csv do |csv| + expect(csv.count).to eq(2) + end + + job.perform(user, postcode, { status: "completed" }) + end + end + + context "when there are more than 20 logs" do + before do + FactoryBot.create_list(:lettings_log, 26, owning_organisation: organisation) + end + + it "does not paginate, it downloads all the user's logs" do + expect_csv do |csv| + # Heading + 2 + 26 + expect(csv.count).to eq(29) + end + + job.perform(user) + end + end + end + end +end diff --git a/spec/mailers/csv_download_mailer_spec.rb b/spec/mailers/csv_download_mailer_spec.rb new file mode 100644 index 000000000..6c145c508 --- /dev/null +++ b/spec/mailers/csv_download_mailer_spec.rb @@ -0,0 +1,30 @@ +require "rails_helper" + +RSpec.describe CsvDownloadMailer do + describe "#send_csv_download_mail" do + let(:notify_client) { instance_double(Notifications::Client) } + let(:user) { FactoryBot.create(:user, email: "user@example.com") } + + before do + allow(Notifications::Client).to receive(:new).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) + end + + it "sends a CSV download E-mail via notify" do + link = :link + duration = 20.minutes.to_i + + expect(notify_client).to receive(:send_email).with( + email_address: user.email, + template_id: described_class::CSV_DOWNLOAD_TEMPLATE_ID, + personalisation: { + name: user.name, + link:, + duration: "20 minutes", + }, + ) + + described_class.new.send_csv_download_mail(user, link, duration) + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 78867bad9..cdeb71092 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -84,4 +84,5 @@ RSpec.configure do |config| config.include Devise::Test::IntegrationHelpers, type: :request config.include ViewComponent::TestHelpers, type: :component config.include Capybara::RSpecMatchers, type: :component + config.include ActiveJob::TestHelper end diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index e70cace51..85b0c0f36 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -383,6 +383,12 @@ RSpec.describe LettingsLogsController, type: :request do end end + it "includes the search on the CSV link" do + search_term = "foo" + get "/logs?search=#{search_term}", headers: headers, params: {} + expect(page).to have_link("Download (CSV)", href: "/logs/csv-download?search=#{search_term}") + end + context "when more than one results with matching postcode" do let!(:matching_postcode_log) { FactoryBot.create(:lettings_log, :completed, owning_organisation: user.organisation, postcode_full: log_to_search.postcode_full) } @@ -491,8 +497,8 @@ RSpec.describe LettingsLogsController, type: :request do expect(page).to have_title("Logs - Submit social housing lettings and sales data (CORE) - GOV.UK") end - it "shows the download csv link" do - expect(page).to have_link("Download (CSV)", href: "/logs.csv") + it "shows the CSV download link" do + expect(page).to have_link("Download (CSV)", href: "/logs/csv-download") end it "does not show the organisation filter" do @@ -729,91 +735,43 @@ RSpec.describe LettingsLogsController, type: :request do expect(CGI.unescape_html(response.body)).to include("You didn’t answer this question") end end - end - describe "CSV download" do - let(:headers) { { "Accept" => "text/csv" } } - let(:user) { FactoryBot.create(:user) } - let(:organisation) { user.organisation } - let(:other_organisation) { FactoryBot.create(:organisation) } - - context "when a log exists" do - let!(:lettings_log) do - FactoryBot.create( - :lettings_log, - owning_organisation: organisation, - managing_organisation: organisation, - ecstat1: 1, - ) - end + context "when requesting CSV download" do + let(:headers) { { "Accept" => "text/html" } } + let(:search_term) { "foo" } before do sign_in user - FactoryBot.create(:lettings_log) - FactoryBot.create(:lettings_log, - :completed, - owning_organisation: organisation, - managing_organisation: organisation, - created_by: user) - get "/logs", headers:, params: {} + get "/logs/csv-download?search=#{search_term}", headers: end - it "downloads a CSV file with headers" do - csv = CSV.parse(response.body) - expect(csv.first.first).to eq("\uFEFFid") - expect(csv.second.first).to eq(lettings_log.id.to_s) - end - - it "does not download other orgs logs" do - csv = CSV.parse(response.body) - expect(csv.count).to eq(3) - end - - it "downloads answer labels rather than values" do - csv = CSV.parse(response.body) - expect(csv.second[15]).to eq("Full-time – 30 hours or more") + it "returns http success" do + expect(response).to have_http_status(:success) end - it "downloads filtered logs" do - get "/logs?status[]=completed", headers:, params: {} - csv = CSV.parse(response.body) - expect(csv.count).to eq(2) + it "shows a confirmation button" do + expect(page).to have_button("Send email") end - it "dowloads searched logs" do - get "/logs?search=#{lettings_log.id}", headers:, params: {} - csv = CSV.parse(response.body) - expect(csv.count).to eq(LettingsLog.search_by(lettings_log.id.to_s).count + 1) + it "includes the search term" do + expect(page).to have_field("search", type: "hidden", with: search_term) end + end - context "when both filter and search applied" do - let(:postcode) { "XX1 1TG" } + context "when confirming the CSV email" do + let(:headers) { { "Accept" => "text/html" } } + context "when a log exists" do before do - FactoryBot.create(:lettings_log, :in_progress, postcode_full: postcode, owning_organisation: organisation, created_by: user) - FactoryBot.create(:lettings_log, :completed, postcode_full: postcode, owning_organisation: organisation, created_by: user) + sign_in user end - it "downloads logs matching both csv and filter logs" do - get "/logs?status[]=completed&search=#{postcode}", headers:, params: {} - csv = CSV.parse(response.body) - expect(csv.count).to eq(2) + it "confirms that the user will receive an email with the requested CSV" do + get "/logs/csv-confirmation" + expect(CGI.unescape_html(response.body)).to include("We're sending you an email") end end end - - context "when there are more than 20 logs" do - before do - sign_in user - FactoryBot.create_list(:lettings_log, 26, owning_organisation: organisation) - get "/logs", headers:, params: {} - end - - it "does not paginate, it downloads all the user's logs" do - csv = CSV.parse(response.body) - expect(csv.count).to eq(27) - end - end end describe "PATCH" do @@ -966,4 +924,60 @@ RSpec.describe LettingsLogsController, type: :request do end end end + + describe "POST #email-csv" do + let(:other_organisation) { FactoryBot.create(:organisation) } + + context "when a log exists" do + let!(:lettings_log) do + FactoryBot.create( + :lettings_log, + owning_organisation:, + managing_organisation: owning_organisation, + ecstat1: 1, + ) + end + + before do + sign_in user + FactoryBot.create(:lettings_log) + FactoryBot.create(:lettings_log, + :completed, + owning_organisation:, + managing_organisation: owning_organisation, + created_by: user) + end + + it "creates an E-mail job" do + expect { + post "/logs/email-csv", headers:, params: {} + }.to enqueue_job(EmailCsvJob).with(user, nil, {}, false) + end + + it "redirects to the confirmation page" do + post "/logs/email-csv", headers:, params: {} + expect(response).to redirect_to(csv_confirmation_lettings_logs_path) + end + + it "passes the search term" do + expect { + post "/logs/email-csv?search=#{lettings_log.id}", headers:, params: {} + }.to enqueue_job(EmailCsvJob).with(user, lettings_log.id.to_s, {}, false) + end + + it "passes filter parameters" do + expect { + post "/logs/email-csv?status[]=completed", headers:, params: {} + }.to enqueue_job(EmailCsvJob).with(user, nil, { "status" => %w[completed] }, false) + end + + it "passes a combination of search term and filter parameters" do + postcode = "XX1 1TG" + + expect { + post "/logs/email-csv?status[]=completed&search=#{postcode}", headers:, params: {} + }.to enqueue_job(EmailCsvJob).with(user, postcode, { "status" => %w[completed] }, false) + end + end + end end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 20197f468..6e635da40 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -352,26 +352,30 @@ RSpec.describe OrganisationsController, type: :request do end context "when viewing logs for other organisation" do - before do + it "does not display the logs" do get "/organisations/#{unauthorised_organisation.id}/logs", headers:, params: {} + expect(response).to have_http_status(:unauthorized) end - it "returns not found 404 from org details route" do - expect(response).to have_http_status(:not_found) - end - - it "shows the 404 view" do - expect(page).to have_content("Page not found") + it "prevents CSV download" do + expect { + post "/organisations/#{unauthorised_organisation.id}/logs/email-csv", headers:, params: {} + }.not_to enqueue_job(EmailCsvJob) + expect(response).to have_http_status(:unauthorized) end end context "when viewing logs for your organisation" do - before do + it "does not display the logs" do get "/organisations/#{organisation.id}/logs", headers:, params: {} + expect(response).to have_http_status(:unauthorized) end - it "redirects to /logs page" do - expect(response).to redirect_to("/logs") + it "prevents CSV download" do + expect { + post "/organisations/#{organisation.id}/logs/email-csv", headers:, params: {} + }.not_to enqueue_job(EmailCsvJob) + expect(response).to have_http_status(:unauthorized) end end @@ -495,26 +499,30 @@ RSpec.describe OrganisationsController, type: :request do end context "when viewing logs for other organisation" do - before do + it "does not display the logs" do get "/organisations/#{unauthorised_organisation.id}/logs", headers:, params: {} + expect(response).to have_http_status(:unauthorized) end - it "returns not found 404 from org details route" do - expect(response).to have_http_status(:not_found) - end - - it "shows the 404 view" do - expect(page).to have_content("Page not found") + it "prevents CSV download" do + expect { + post "/organisations/#{unauthorised_organisation.id}/logs/email-csv", headers:, params: {} + }.not_to enqueue_job(EmailCsvJob) + expect(response).to have_http_status(:unauthorized) end end context "when viewing logs for your organisation" do - before do + it "does not display the logs" do get "/organisations/#{organisation.id}/logs", headers:, params: {} + expect(response).to have_http_status(:unauthorized) end - it "redirects to /logs page" do - expect(response).to redirect_to("/logs") + it "prevents CSV download" do + expect { + post "/organisations/#{organisation.id}/logs/email-csv", headers:, params: {} + }.not_to enqueue_job(EmailCsvJob) + expect(response).to have_http_status(:unauthorized) end end end @@ -1035,11 +1043,10 @@ RSpec.describe OrganisationsController, type: :request do end it "has a CSV download button with the correct path" do - expect(page).to have_link("Download (CSV)", href: "/organisations/#{organisation.id}/logs.csv") + expect(page).to have_link("Download (CSV)", href: "/organisations/#{organisation.id}/logs/csv-download") end context "when you download the CSV" do - let(:headers) { { "Accept" => "text/csv" } } let(:other_organisation) { FactoryBot.create(:organisation) } before do @@ -1048,9 +1055,15 @@ RSpec.describe OrganisationsController, type: :request do end it "only includes logs from that organisation" do - get "/organisations/#{organisation.id}/logs", headers:, params: {} - csv = CSV.parse(response.body) - expect(csv.count).to eq(4) + get "/organisations/#{organisation.id}/logs/csv-download" + + expect(page).to have_text("You've selected 3 logs.") + end + + it "provides the organisation to the mail job" do + expect { + post "/organisations/#{organisation.id}/logs/email-csv?status[]=completed", headers:, params: {} + }.to enqueue_job(EmailCsvJob).with(user, nil, { "status" => %w[completed] }, false, organisation) end end end diff --git a/spec/services/filter_service_spec.rb b/spec/services/filter_service_spec.rb new file mode 100644 index 000000000..1e892fd56 --- /dev/null +++ b/spec/services/filter_service_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +describe FilterService do + describe "filter_by_search" do + before do + FactoryBot.create_list(:organisation, 5) + FactoryBot.create(:organisation, name: "Acme LTD") + end + + let(:organisation_list) { Organisation.all } + + context "when given a search term" do + let(:search_term) { "Acme" } + + it "filters the collection on search term" do + expect(described_class.filter_by_search(organisation_list, search_term).count).to eq(1) + end + end + + context "when not given a search term" do + let(:search_term) { nil } + + it "does not filter the given collection" do + expect(described_class.filter_by_search(organisation_list, search_term).count).to eq(6) + end + end + end +end