Browse Source
* 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 <nat.dean-lewis@softwire.com> Co-authored-by: James Rose <james@jbpr.net>pull/867/head
Sam
2 years ago
committed by
GitHub
27 changed files with 552 additions and 146 deletions
@ -1,23 +1,21 @@ |
|||||||
module Modules::LettingsLogsFilter |
module Modules::LettingsLogsFilter |
||||||
def filtered_lettings_logs(logs) |
def filtered_lettings_logs(logs, search_term, filters) |
||||||
if session[:lettings_logs_filters].present? |
all_orgs = params["organisation_select"] == "all" |
||||||
filters = JSON.parse(session[:lettings_logs_filters]) |
FilterService.filter_lettings_logs(logs, search_term, filters, all_orgs, current_user) |
||||||
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 |
|
||||||
end |
end |
||||||
|
|
||||||
def set_session_filters(specific_org: false) |
def load_session_filters(specific_org: false) |
||||||
new_filters = session[:lettings_logs_filters].present? ? JSON.parse(session[:lettings_logs_filters]) : {} |
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? } |
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 |
||||||
end |
end |
||||||
|
|||||||
@ -1,13 +1,9 @@ |
|||||||
module Modules::SearchFilter |
module Modules::SearchFilter |
||||||
def filtered_collection(base_collection, search_term = nil) |
def filtered_collection(base_collection, search_term = nil) |
||||||
if search_term.present? |
FilterService.filter_by_search(base_collection, search_term) |
||||||
base_collection.search_by(search_term) |
|
||||||
else |
|
||||||
base_collection |
|
||||||
end |
|
||||||
end |
end |
||||||
|
|
||||||
def filtered_users(base_collection, search_term = nil) |
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 |
||||||
end |
end |
||||||
|
|||||||
@ -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 |
||||||
@ -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 |
||||||
@ -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 |
||||||
@ -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 |
||||||
@ -0,0 +1,15 @@ |
|||||||
|
<% content_for :title, "We've sending you an email" %> |
||||||
|
<div class="govuk-grid-row"> |
||||||
|
<div class="govuk-grid-column-two-thirds"> |
||||||
|
<%= govuk_panel(title_text: "We're sending you an email") %> |
||||||
|
|
||||||
|
<p class="govuk-body">It should arrive in a few minutes, but it could take longer.</p> |
||||||
|
|
||||||
|
<h2 class="govuk-heading-m">What happens next</h2> |
||||||
|
<p class="govuk-body">Open your email inbox and click the link to download your CSV file.</p> |
||||||
|
|
||||||
|
<p class="govuk-body"> |
||||||
|
<%= govuk_link_to "Return to logs", lettings_logs_path %> |
||||||
|
</p> |
||||||
|
</div> |
||||||
|
</div> |
||||||
@ -0,0 +1,16 @@ |
|||||||
|
<% content_for :title, "Download CSV" %> |
||||||
|
|
||||||
|
<% content_for :before_content do %> |
||||||
|
<%= govuk_back_link(href: :back) %> |
||||||
|
<% end %> |
||||||
|
|
||||||
|
<div class="govuk-grid-row"> |
||||||
|
<div class="govuk-grid-column-two-thirds"> |
||||||
|
<h1 class="govuk-heading-l">Download CSV</h2> |
||||||
|
|
||||||
|
<p class="govuk-body">We'll send a secure download link to your email address <strong><%= @current_user.email %></strong>.</p> |
||||||
|
<p class="govuk-body">You've selected <%= count %> logs.</p> |
||||||
|
|
||||||
|
<%= govuk_button_to "Send email", post_path, method: :post, params: { search: search_term } %> |
||||||
|
</div> |
||||||
|
</div> |
||||||
@ -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 |
||||||
@ -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 |
||||||
@ -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 |
||||||
Loading…
Reference in new issue