Merge branch 'main' into full-import-optimisation

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,

app/models/form_handler.rb

@@ -158,6 +158,7 @@ class FormHandler
 
   def use_real_forms!
     @directories = ["config/forms"]
+    @lettings_forms = get_lettings_forms
     @forms = get_all_forms
   end
 

app/views/devise/confirmations/invalid.html.erb

@@ -7,6 +7,5 @@
     </h1>
 
     <p class="govuk-body">It looks like you have requested a newer join link than this one. Check your emails and follow the most recent link instead.</p>
-
   </div>
 </div>

app/views/devise/confirmations/new.html.erb

@@ -6,11 +6,11 @@
       <%= content_for(:title) %>
     </h1>
     <%= form_for(resource, as: resource_name, url: confirmation_path(resource_name), html: { method: :post }) do |f| %>
-
-      <p class="govuk-body">For security reasons, your join link expired - get another one using the button below (valid for 3 hours).</p>
+      <p class="govuk-body">For security reasons, your link expired - get another one using the button below (valid for 24 hours).</p>
 
       <%= 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 %>
 
   </div>

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

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 = [
@@ -37,7 +37,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
@@ -54,7 +54,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),
@@ -78,7 +78,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

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

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
 

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

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
 

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
 

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") }

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