From c3b3c6200af1d9ed1a49845257863bf6d6f26f60 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Wed, 16 Aug 2023 09:41:24 +0100
Subject: [PATCH 1/4] CLDC-2602 Only log errors into the s3 (#1843)
* Only log errors into the s3
* lint
---
config/initializers/multi_logger.rb | 12 +++++----
lib/tasks/full_import.rake | 8 +++---
spec/lib/tasks/full_import_spec.rb | 12 ++++-----
.../lettings_logs_import_service_spec.rb | 26 +++++++++++++++++--
.../organisation_import_service_spec.rb | 4 +--
5 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/config/initializers/multi_logger.rb b/config/initializers/multi_logger.rb
index d39d786c1..6c0b4eb27 100644
--- a/config/initializers/multi_logger.rb
+++ b/config/initializers/multi_logger.rb
@@ -1,17 +1,19 @@
class MultiLogger
- def initialize(*targets)
- @targets = targets
+ def initialize(file_logger)
+ @rails_logger = Rails.logger
+ @file_logger = file_logger
end
def info(data)
- @targets.each { |t| t.info(data) }
+ @rails_logger.info(data)
end
def warn(data)
- @targets.each { |t| t.warn(data) }
+ @rails_logger.warn(data)
end
def error(data)
- @targets.each { |t| t.error(data) }
+ @rails_logger.error(data)
+ @file_logger.error(data)
end
end
diff --git a/lib/tasks/full_import.rake b/lib/tasks/full_import.rake
index c1fe146d8..3b74b458a 100644
--- a/lib/tasks/full_import.rake
+++ b/lib/tasks/full_import.rake
@@ -9,7 +9,7 @@ namespace :import do
s3_service = Storage::S3Service.new(PlatformHelper.is_paas? ? Configuration::PaasConfigurationService.new : Configuration::EnvConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
csv = CSV.parse(s3_service.get_file_io(institutions_csv_name), headers: true)
logs_string = StringIO.new
- logger = MultiLogger.new(Rails.logger, Logger.new(logs_string))
+ logger = MultiLogger.new(Logger.new(logs_string))
org_count = csv.length
initial_import_list = [
@@ -36,7 +36,7 @@ namespace :import do
end
log_file = "#{File.basename(institutions_csv_name, File.extname(institutions_csv_name))}_#{File.basename(archive_path, File.extname(archive_path))}_initial.log"
- s3_service.write_file(log_file, logs_string.string)
+ s3_service.write_file(log_file, logs_string.string) if logs_string.string.present?
logs_string.rewind
logs_string.truncate(0)
end
@@ -53,7 +53,7 @@ namespace :import do
csv = CSV.parse(s3_service.get_file_io(institutions_csv_name), headers: true)
org_count = csv.length
logs_string = StringIO.new
- logger = MultiLogger.new(Rails.logger, Logger.new(logs_string))
+ logger = MultiLogger.new(Logger.new(logs_string))
logs_import_list = [
Import.new(Imports::LettingsLogsImportService, :create_logs, "logs", logger),
@@ -77,7 +77,7 @@ namespace :import do
end
log_file = "#{File.basename(institutions_csv_name, File.extname(institutions_csv_name))}_#{File.basename(archive_path, File.extname(archive_path))}_logs.log"
- s3_service.write_file(log_file, logs_string.string)
+ s3_service.write_file(log_file, logs_string.string) if logs_string.string.present?
logs_string.rewind
logs_string.truncate(0)
end
diff --git a/spec/lib/tasks/full_import_spec.rb b/spec/lib/tasks/full_import_spec.rb
index 0b8d3bd9a..b95b370cd 100644
--- a/spec/lib/tasks/full_import_spec.rb
+++ b/spec/lib/tasks/full_import_spec.rb
@@ -77,10 +77,10 @@ describe "full import", type: :task do
allow(Imports::OrganisationRentPeriodImportService).to receive(:new).and_return(instance_double(Imports::OrganisationRentPeriodImportService))
end
- it "creates a report using given organisation csv" do
+ it "does not write a report if there were no errors" do
expect(Storage::S3Service).to receive(:new).with(env_config_service, instance_name)
- expect(storage_service).to receive(:write_file).with("some_name_1_initial.log", / INFO -- : Performing initial imports for organisation org1/)
- expect(storage_service).to receive(:write_file).with("some_name_2_initial.log", / INFO -- : Performing initial imports for organisation org2/)
+ expect(storage_service).not_to receive(:write_file).with("some_name_1_initial.log", "")
+ expect(storage_service).not_to receive(:write_file).with("some_name_2_initial.log", "")
task.invoke("some_name.csv")
end
@@ -106,10 +106,10 @@ describe "full import", type: :task do
allow(Imports::SalesLogsImportService).to receive(:new).and_return(instance_double(Imports::SalesLogsImportService))
end
- it "creates a report using given organisation csv" do
+ it "does not write a report if there were no errors" do
expect(Storage::S3Service).to receive(:new).with(env_config_service, instance_name)
- expect(storage_service).to receive(:write_file).with("some_name_1_logs.log", / INFO -- : Importing logs for organisation org1, expecting 0 logs/)
- expect(storage_service).to receive(:write_file).with("some_name_2_logs.log", / INFO -- : Importing logs for organisation org2, expecting 0 logs/)
+ expect(storage_service).not_to receive(:write_file).with("some_name_1_logs.log", "")
+ expect(storage_service).not_to receive(:write_file).with("some_name_2_logs.log", "")
task.invoke("some_name.csv")
end
diff --git a/spec/services/imports/lettings_logs_import_service_spec.rb b/spec/services/imports/lettings_logs_import_service_spec.rb
index 738d2ed39..8986710d9 100644
--- a/spec/services/imports/lettings_logs_import_service_spec.rb
+++ b/spec/services/imports/lettings_logs_import_service_spec.rb
@@ -14,7 +14,9 @@ RSpec.describe Imports::LettingsLogsImportService do
end
let(:storage_service) { instance_double(Storage::S3Service) }
- let(:logger) { instance_double(ActiveSupport::Logger) }
+ let(:logs_string) { StringIO.new }
+ let(:file_logger) { Logger.new(logs_string) }
+ let(:logger) { MultiLogger.new(file_logger) }
let(:real_2021_2022_form) { Form.new("config/forms/2021_2022.json") }
let(:real_2022_2023_form) { Form.new("config/forms/2022_2023.json") }
@@ -328,6 +330,24 @@ RSpec.describe Imports::LettingsLogsImportService do
end
end
+ context "and submitted log has errors" do
+ let(:lettings_log_id) { "0ead17cb-1668-442d-898c-0d52879ff592" }
+
+ before do
+ lettings_log_xml.at_xpath("//meta:status").content = "submitted"
+ lettings_log_xml.at_xpath("//xmlns:HHMEMB").content = "32"
+ end
+
+ it "logs the error" do
+ expect { lettings_log_service.send(:create_log, lettings_log_xml) }
+ .to raise_error(ActiveRecord::RecordInvalid)
+
+ expect(logs_string.string).to include("Failed to import")
+ expect(logs_string.string).to include("ERROR -- : Validation error: Field hhmemb")
+ expect(logs_string.string).to include("Error message: outside_the_range")
+ end
+ end
+
context "and this is an internal transfer from a non social housing" do
before do
lettings_log_xml.at_xpath("//xmlns:Q11").content = "9 Residential care home"
@@ -1048,7 +1068,9 @@ RSpec.describe Imports::LettingsLogsImportService do
end
let(:storage_service) { instance_double(Storage::S3Service) }
- let(:logger) { instance_double(ActiveSupport::Logger) }
+ let(:logs_string) { StringIO.new }
+ let(:file_logger) { Logger.new(logs_string) }
+ let(:logger) { MultiLogger.new(file_logger) }
let(:real_2021_2022_form) { Form.new("config/forms/2021_2022.json") }
let(:real_2022_2023_form) { Form.new("config/forms/2022_2023.json") }
diff --git a/spec/services/imports/organisation_import_service_spec.rb b/spec/services/imports/organisation_import_service_spec.rb
index 8496c046f..41e843e3c 100644
--- a/spec/services/imports/organisation_import_service_spec.rb
+++ b/spec/services/imports/organisation_import_service_spec.rb
@@ -3,7 +3,7 @@ require "rails_helper"
RSpec.describe Imports::OrganisationImportService do
let(:storage_service) { instance_double(Storage::S3Service) }
let(:logs_string) { StringIO.new }
- let(:logger) { MultiLogger.new(Rails.logger, Logger.new(logs_string)) }
+ let(:logger) { MultiLogger.new(logs_string) }
let(:folder_name) { "organisations" }
let(:filenames) { %w[my_folder/my_file1.xml my_folder/my_file2.xml] }
let(:fixture_directory) { "spec/fixtures/imports/institution" }
@@ -87,7 +87,7 @@ RSpec.describe Imports::OrganisationImportService do
expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(1)
expect { import_service.create_organisations(folder_name) }.to change(Organisation, :count).by(0)
- expect(logs_string.string).to include("Organisation my_new_organisation is already present with old visible ID 1, skipping.")
+ expect(logs_string.string).not_to include("Organisation my_new_organisation is already present with old visible ID 1, skipping.")
expect(Organisation).to exist(old_visible_id: "1", name: "HA Ltd")
end
From d73116f0981a3cecc7b3315a997d83804aef6f4f Mon Sep 17 00:00:00 2001
From: natdeanlewissoftwire
<94526761+natdeanlewissoftwire@users.noreply.github.com>
Date: Wed, 16 Aug 2023 16:22:17 +0100
Subject: [PATCH 2/4] Memoise lettings forms in form handler (#1845)
* feat: memoise lettings forms
* feat: update tests
---
app/models/form_handler.rb | 5 +++++
spec/models/form_spec.rb | 1 -
spec/models/lettings_log_spec.rb | 2 ++
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/app/models/form_handler.rb b/app/models/form_handler.rb
index 8cbd5a2e5..645efb1b4 100644
--- a/app/models/form_handler.rb
+++ b/app/models/form_handler.rb
@@ -75,6 +75,10 @@ class FormHandler
end
def lettings_forms
+ @lettings_forms ||= get_lettings_forms
+ end
+
+ def get_lettings_forms
forms = {}
directories.each do |directory|
Dir.glob("#{directory}/*.json").each do |form_path|
@@ -154,6 +158,7 @@ class FormHandler
def use_real_forms!
@directories = ["config/forms"]
+ @lettings_forms = get_lettings_forms
@forms = get_all_forms
end
diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb
index c812f8172..5547d64ea 100644
--- a/spec/models/form_spec.rb
+++ b/spec/models/form_spec.rb
@@ -188,7 +188,6 @@ RSpec.describe Form, type: :model do
around do |example|
FormHandler.instance.use_real_forms!
-
example.run
end
diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb
index 489a31b9b..36c793c79 100644
--- a/spec/models/lettings_log_spec.rb
+++ b/spec/models/lettings_log_spec.rb
@@ -1575,6 +1575,7 @@ RSpec.describe LettingsLog do
it "clears underoccupation_benefitcap if log is no longer in 2021/22" do
expect { lettings_log.update!(renewal: 1) }.to change(lettings_log, :underoccupation_benefitcap).to 2
Timecop.return
+ Singleton.__init__(FormHandler)
expect { lettings_log.update!(startdate: Time.zone.local(2023, 1, 1)) }.to change(lettings_log, :underoccupation_benefitcap).from(2).to nil
end
@@ -2040,6 +2041,7 @@ RSpec.describe LettingsLog do
before do
Timecop.freeze(Time.zone.local(2022, 4, 2))
+ Singleton.__init__(FormHandler)
lettings_log.update!(startdate: Time.zone.local(2022, 4, 2), scheme:)
Timecop.unfreeze
end
From abbed0be747b32295f7d55f41d2145a5be508782 Mon Sep 17 00:00:00 2001
From: Jack <113976590+bibblobcode@users.noreply.github.com>
Date: Thu, 17 Aug 2023 09:22:42 +0100
Subject: [PATCH 3/4] Update page copy (#1844)
---
app/views/devise/confirmations/invalid.html.erb | 1 -
app/views/devise/confirmations/new.html.erb | 6 +++---
spec/requests/auth/confirmations_controller_spec.rb | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/app/views/devise/confirmations/invalid.html.erb b/app/views/devise/confirmations/invalid.html.erb
index 8d3ad96df..32219f0f4 100644
--- a/app/views/devise/confirmations/invalid.html.erb
+++ b/app/views/devise/confirmations/invalid.html.erb
@@ -7,6 +7,5 @@
It looks like you have requested a newer join link than this one. Check your emails and follow the most recent link instead.
-
diff --git a/app/views/devise/confirmations/new.html.erb b/app/views/devise/confirmations/new.html.erb
index 5f36dc388..3fc34aee6 100644
--- a/app/views/devise/confirmations/new.html.erb
+++ b/app/views/devise/confirmations/new.html.erb
@@ -6,11 +6,11 @@
<%= content_for(:title) %>
<%= form_for(resource, as: resource_name, url: confirmation_path(resource_name), html: { method: :post }) do |f| %>
-
- For security reasons, your join link expired - get another one using the button below (valid for 3 hours).
+ For security reasons, your link expired - get another one using the button below (valid for 24 hours).
<%= f.hidden_field :email, value: (resource.pending_reconfirmation? ? resource.unconfirmed_email : resource.email) %>
- <%= f.govuk_submit "Get a new join link" %>
+
+ <%= f.govuk_submit "Get a new link" %>
<% end %>
diff --git a/spec/requests/auth/confirmations_controller_spec.rb b/spec/requests/auth/confirmations_controller_spec.rb
index aa7f93d65..e7e2e1815 100644
--- a/spec/requests/auth/confirmations_controller_spec.rb
+++ b/spec/requests/auth/confirmations_controller_spec.rb
@@ -40,7 +40,7 @@ RSpec.describe Auth::ConfirmationsController, type: :request do
end
it "shows the expired page" do
- expect(page).to have_content("For security reasons, your join link expired - get another one using the button below (valid for 3 hours).")
+ expect(page).to have_content("For security reasons, your link expired - get another one using the button below (valid for 24 hours).")
end
end
From aaa66a3f1fb88817d0df9d98df70c44a2c2cc882 Mon Sep 17 00:00:00 2001
From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com>
Date: Thu, 17 Aug 2023 12:59:49 +0100
Subject: [PATCH 4/4] Allow non stock owning orgs to create sales logs (#1849)
---
app/controllers/logs_controller.rb | 2 +-
spec/requests/sales_logs_controller_spec.rb | 55 +++++++++++++++++++++
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/app/controllers/logs_controller.rb b/app/controllers/logs_controller.rb
index de4a35bdb..599f3a7b9 100644
--- a/app/controllers/logs_controller.rb
+++ b/app/controllers/logs_controller.rb
@@ -63,7 +63,7 @@ private
end
def org_params
- owning_organisation_id = current_user.organisation.holds_own_stock? ? current_user.organisation.id : nil
+ owning_organisation_id = instance_of?(SalesLogsController) || current_user.organisation.holds_own_stock? ? current_user.organisation.id : nil
{
"owning_organisation_id" => owning_organisation_id,
"created_by_id" => current_user.id,
diff --git a/spec/requests/sales_logs_controller_spec.rb b/spec/requests/sales_logs_controller_spec.rb
index 0305690f8..8a21f3fa4 100644
--- a/spec/requests/sales_logs_controller_spec.rb
+++ b/spec/requests/sales_logs_controller_spec.rb
@@ -106,6 +106,61 @@ RSpec.describe SalesLogsController, type: :request do
expect(whodunnit_actor).to be_a(User)
expect(whodunnit_actor.id).to eq(user.id)
end
+
+ context "when creating a new log" do
+ context "when the user is support" do
+ let(:organisation) { FactoryBot.create(:organisation) }
+ let(:support_user) { FactoryBot.create(:user, :support, organisation:) }
+
+ before do
+ allow(support_user).to receive(:need_two_factor_authentication?).and_return(false)
+ sign_in support_user
+ post "/sales-logs", headers:
+ end
+
+ it "sets the stock-owning org as nil" do
+ created_id = response.location.match(/[0-9]+/)[0]
+ sales_log = SalesLog.find_by(id: created_id)
+ expect(sales_log.owning_organisation).to eq(nil)
+ end
+ end
+
+ context "when the user is not support" do
+ context "when the user's org holds stock" do
+ let(:organisation) { FactoryBot.create(:organisation, name: "User org", holds_own_stock: true) }
+ let(:user) { FactoryBot.create(:user, :data_coordinator, organisation:) }
+
+ before do
+ RequestHelper.stub_http_requests
+ sign_in user
+ post "/sales-logs", headers:
+ end
+
+ it "sets the stock-owning org as the user's org" do
+ created_id = response.location.match(/[0-9]+/)[0]
+ sales_log = SalesLog.find_by(id: created_id)
+ expect(sales_log.owning_organisation.name).to eq("User org")
+ end
+ end
+
+ context "when the user's org doesn't hold stock" do
+ let(:organisation) { FactoryBot.create(:organisation, name: "User org", holds_own_stock: false) }
+ let(:user) { FactoryBot.create(:user, :data_coordinator, organisation:) }
+
+ before do
+ RequestHelper.stub_http_requests
+ sign_in user
+ post "/sales-logs", headers:
+ end
+
+ it "sets the stock-owning org as user's org" do
+ created_id = response.location.match(/[0-9]+/)[0]
+ sales_log = SalesLog.find_by(id: created_id)
+ expect(sales_log.owning_organisation.name).to eq("User org")
+ end
+ end
+ end
+ end
end
end