Merge branch 'main' into CLDC-1235-conditional-shortfall

app/controllers/form_controller.rb

@@ -73,7 +73,7 @@ private
     end
     if session["fields"]
       session["fields"].each do |field, value|
-        unless @case_log.form.get_question(field, @case_log).type == "date"
+        unless @case_log.form.get_question(field, @case_log)&.type == "date"
           @case_log[field] = value
         end
       end
@@ -161,10 +161,11 @@ private
 
   def question_missing_response?(responses_for_page, question)
     if %w[checkbox validation_override].include?(question.type)
-      question.answer_options.keys.reject { |x| x.match(/divider/) }.all? do |option|
+      answered = question.answer_options.keys.reject { |x| x.match(/divider/) }.map do |option|
         session["fields"][option] = @case_log[option] = params["case_log"][question.id].include?(option) ? 1 : 0
         params["case_log"][question.id].exclude?(option)
       end
+      answered.all?
     else
       session["fields"][question.id] = @case_log[question.id] = responses_for_page[question.id]
       responses_for_page[question.id].nil? || responses_for_page[question.id].blank?

app/helpers/navigation_items_helper.rb

@@ -9,7 +9,7 @@ module NavigationItemsHelper
         NavigationItem.new("Logs", case_logs_path, logs_current?(path)),
         NavigationItem.new("Schemes", "/schemes", supported_housing_schemes_current?(path)),
       ]
-    elsif current_user.data_coordinator?
+    elsif current_user.data_coordinator? && current_user.organisation.holds_own_stock?
       [
         NavigationItem.new("Logs", case_logs_path, logs_current?(path)),
         NavigationItem.new("Schemes", "/schemes", subnav_supported_housing_schemes_path?(path)),

app/services/imports/user_import_service.rb

@@ -13,8 +13,8 @@ module Imports
     def create_user(xml_document)
       organisation = Organisation.find_by(old_org_id: user_field_value(xml_document, "institution"))
       old_user_id = user_field_value(xml_document, "id")
-      name = user_field_value(xml_document, "full-name")
       email = user_field_value(xml_document, "email").downcase.strip
+      name = user_field_value(xml_document, "full-name") || email
       deleted = user_field_value(xml_document, "deleted")
 
       if User.find_by(old_user_id:, organisation:)

app/services/storage_service.rb

@@ -13,6 +13,11 @@ class StorageService
            .flat_map { |response| response.contents.map(&:key) }
   end
 
+  def folder_present?(folder)
+    response = @client.list_objects_v2(bucket: @configuration.bucket_name, prefix: folder, max_keys: 1)
+    response.key_count == 1
+  end
+
   def get_file_io(file_name)
     @client.get_object(bucket: @configuration.bucket_name, key: file_name)
            .body

lib/tasks/data_import.rake

@@ -1,5 +1,5 @@
 namespace :core do
-  desc "Import data XMLs from Softwire system"
+  desc "Import data XMLs from legacy CORE"
   task :data_import, %i[type path] => :environment do |_task, args|
     type = args[:type]
     path = args[:path]

lib/tasks/full_import.rake

@@ -0,0 +1,30 @@
+Import = Struct.new("Import", :import_class, :import_method, :folder)
+
+namespace :core do
+  desc "Import all data XMLs from legacy CORE"
+  task :full_import, %i[path] => :environment do |_task, args|
+    path = args[:path]
+    raise "Usage: rake core:full_import['path/to/main_folder']" if path.blank?
+
+    storage_service = StorageService.new(PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"])
+
+    import_list = [
+      Import.new(Imports::OrganisationImportService, :create_organisations, "institution"),
+      Import.new(Imports::SchemeImportService, :create_schemes, "mgmtgroups"),
+      Import.new(Imports::SchemeLocationImportService, :create_scheme_locations, "schemes"),
+      Import.new(Imports::UserImportService, :create_users, "user"),
+      Import.new(Imports::DataProtectionConfirmationImportService, :create_data_protection_confirmations, "dataprotect"),
+      Import.new(Imports::OrganisationRentPeriodImportService, :create_organisation_rent_periods, "rent-period"),
+      Import.new(Imports::CaseLogsImportService, :create_logs, "logs"),
+    ]
+
+    import_list.each do |import|
+      folder_path = File.join(path, import.folder, "")
+      if storage_service.folder_present?(folder_path)
+        import.import_class.new(storage_service).send(import.import_method, folder_path)
+      else
+        Rails.logger.info("#{folder_path} does not exist, skipping #{import.import_class}")
+      end
+    end
+  end
+end

spec/factories/organisation.rb

@@ -8,6 +8,7 @@ FactoryBot.define do
     postcode { "SW1P 4DF" }
     created_at { Time.zone.now }
     updated_at { Time.zone.now }
+    holds_own_stock { true }
   end
 
   factory :organisation_la do

spec/features/form/checkboxes_spec.rb

@@ -38,4 +38,27 @@ RSpec.describe "Checkboxes" do
       expect(case_log["housingneeds_h"]).to eq(1)
     end
   end
+
+  context "when a checkbox question is submitted with invalid answers" do
+    before do
+      allow(case_log).to receive(:update).and_return(false)
+    end
+
+    it "shows an error summary" do
+      visit("/logs/#{id}/accessibility-requirements")
+      page.check("case-log-accessibility-requirements-housingneeds-a-field")
+      page.check("case-log-accessibility-requirements-housingneeds-c-field")
+      click_button("Save and continue")
+      expect(page).to have_title("Error")
+    end
+
+    it "persists the original selections" do
+      visit("/logs/#{id}/accessibility-requirements")
+      page.check("case-log-accessibility-requirements-housingneeds-a-field")
+      page.check("case-log-accessibility-requirements-housingneeds-c-field")
+      click_button("Save and continue")
+      expect(page).to have_checked_field("case-log-accessibility-requirements-housingneeds-a-field")
+      expect(page).to have_checked_field("case-log-accessibility-requirements-housingneeds-c-field")
+    end
+  end
 end

spec/features/organisation_spec.rb

@@ -39,6 +39,28 @@ RSpec.describe "User Features" do
         click_link("About your organisation")
         expect(page).to have_current_path("/organisations/#{org_id}/details")
       end
+
+      context "when the user is a coordinator and the organisation does not hold housing stock" do
+        before do
+          organisation.update(holds_own_stock: false)
+        end
+
+        it "does not show schemes in the navigation bar" do
+          visit("/logs")
+          expect(page).not_to have_link("Schemes", href: "/schemes")
+        end
+      end
+
+      context "when the user is a coordinator and the organisation holds housing stock" do
+        before do
+          organisation.update(holds_own_stock: true)
+        end
+
+        it "shows schemes in the navigation bar" do
+          visit("/logs")
+          expect(page).to have_link("Schemes", href: "/schemes")
+        end
+      end
     end
 
     context "when users are part of organisation" do

spec/lib/tasks/full_import_spec.rb

@@ -0,0 +1,85 @@
+require "rails_helper"
+require "rake"
+
+describe "rake core:full_import", type: :task do
+  subject(:task) { Rake::Task["core:full_import"] }
+
+  let(:instance_name) { "paas_import_instance" }
+  let(:storage_service) { instance_double(StorageService) }
+  let(:paas_config_service) { instance_double(PaasConfigurationService) }
+
+  before do
+    Rake.application.rake_require("tasks/full_import")
+    Rake::Task.define_task(:environment)
+    task.reenable
+
+    allow(StorageService).to receive(:new).and_return(storage_service)
+    allow(PaasConfigurationService).to receive(:new).and_return(paas_config_service)
+    allow(ENV).to receive(:[])
+    allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name)
+  end
+
+  context "when starting a full import" do
+    let(:fixture_path) { "spec/fixtures/imports" }
+    let(:case_logs_service) { instance_double(Imports::CaseLogsImportService) }
+    let(:rent_period_service) { instance_double(Imports::OrganisationRentPeriodImportService) }
+    let(:data_protection_service) { instance_double(Imports::DataProtectionConfirmationImportService) }
+    let(:user_service) { instance_double(Imports::UserImportService) }
+    let(:location_service) { instance_double(Imports::SchemeLocationImportService) }
+    let(:scheme_service) { instance_double(Imports::SchemeImportService) }
+    let(:organisation_service) { instance_double(Imports::OrganisationImportService) }
+
+    before do
+      allow(Imports::OrganisationImportService).to receive(:new).and_return(organisation_service)
+      allow(Imports::SchemeImportService).to receive(:new).and_return(scheme_service)
+      allow(Imports::SchemeLocationImportService).to receive(:new).and_return(location_service)
+      allow(Imports::UserImportService).to receive(:new).and_return(user_service)
+      allow(Imports::DataProtectionConfirmationImportService).to receive(:new).and_return(data_protection_service)
+      allow(Imports::OrganisationRentPeriodImportService).to receive(:new).and_return(rent_period_service)
+      allow(Imports::CaseLogsImportService).to receive(:new).and_return(case_logs_service)
+    end
+
+    it "raises an exception if no parameters are provided" do
+      expect { task.invoke }.to raise_error(/Usage/)
+    end
+
+    context "with all folders being present" do
+      before { allow(storage_service).to receive(:folder_present?).and_return(true) }
+
+      it "calls every import method with the correct folder" do
+        expect(organisation_service).to receive(:create_organisations).with("#{fixture_path}/institution/")
+        expect(scheme_service).to receive(:create_schemes).with("#{fixture_path}/mgmtgroups/")
+        expect(location_service).to receive(:create_scheme_locations).with("#{fixture_path}/schemes/")
+        expect(user_service).to receive(:create_users).with("#{fixture_path}/user/")
+        expect(data_protection_service).to receive(:create_data_protection_confirmations).with("#{fixture_path}/dataprotect/")
+        expect(rent_period_service).to receive(:create_organisation_rent_periods).with("#{fixture_path}/rent-period/")
+        expect(case_logs_service).to receive(:create_logs).with("#{fixture_path}/logs/")
+
+        task.invoke(fixture_path)
+      end
+    end
+
+    context "when a specific folders are missing" do
+      before do
+        allow(storage_service).to receive(:folder_present?).and_return(true)
+        allow(storage_service).to receive(:folder_present?).with("#{fixture_path}/mgmtgroups/").and_return(false)
+        allow(storage_service).to receive(:folder_present?).with("#{fixture_path}/schemes/").and_return(false)
+      end
+
+      it "only calls import methods for existing folders" do
+        expect(organisation_service).to receive(:create_organisations)
+        expect(user_service).to receive(:create_users)
+        expect(data_protection_service).to receive(:create_data_protection_confirmations)
+        expect(rent_period_service).to receive(:create_organisation_rent_periods)
+        expect(case_logs_service).to receive(:create_logs)
+
+        expect(scheme_service).not_to receive(:create_schemes)
+        expect(location_service).not_to receive(:create_scheme_locations)
+        expect(Rails.logger).to receive(:info).with("spec/fixtures/imports/mgmtgroups/ does not exist, skipping Imports::SchemeImportService")
+        expect(Rails.logger).to receive(:info).with("spec/fixtures/imports/schemes/ does not exist, skipping Imports::SchemeLocationImportService")
+
+        task.invoke(fixture_path)
+      end
+    end
+  end
+end

spec/services/storage_service_spec.rb

@@ -99,7 +99,7 @@ RSpec.describe StorageService do
     end
   end
 
-  context "when we create a storage service and list files" do
+  context "when we create a storage service" do
     subject(:storage_service) { described_class.new(PaasConfigurationService.new, instance_name) }
 
     let(:s3_client_stub) { Aws::S3::Client.new(stub_responses: true) }
@@ -110,22 +110,59 @@ RSpec.describe StorageService do
       allow(Aws::S3::Client).to receive(:new).and_return(s3_client_stub)
     end
 
-    it "returns a list with all present file names in a given folder" do
-      expected_filenames = %w[my_folder/my_file1.xml my_folder/my_file2.xml]
-      s3_client_stub.stub_responses(:list_objects_v2, {
-        contents: [
-          {
-            key: expected_filenames[0],
-          },
-          {
-            key: expected_filenames[1],
-          },
-        ],
-      })
+    context "and we list files based on a prefix" do
+      let(:expected_filenames) { %w[my_folder/my_file1.xml my_folder/my_file2.xml] }
+
+      before do
+        s3_client_stub.stub_responses(:list_objects_v2, {
+          contents: [
+            {
+              key: expected_filenames[0],
+            },
+            {
+              key: expected_filenames[1],
+            },
+          ],
+        })
+      end
+
+      it "returns a list with all present file names in a given folder" do
+        filenames = storage_service.list_files("my_folder")
+        expect(filenames).to eq(expected_filenames)
+      end
+    end
 
-      filenames = storage_service.list_files("my_folder")
+    context "and we check for an existing folder" do
+      before do
+        expected_filenames = %w[my_folder/my_file1.xml]
+        s3_client_stub.stub_responses(:list_objects_v2, {
+          key_count: 1,
+          contents: [
+            {
+              key: expected_filenames[0],
+            },
+          ],
+        })
+      end
+
+      it "returns true" do
+        response = storage_service.folder_present?("my_folder")
+        expect(response).to be_truthy
+      end
+    end
 
-      expect(filenames).to eq(expected_filenames)
+    context "and we check for a folder that does not exists" do
+      before do
+        s3_client_stub.stub_responses(:list_objects_v2, {
+          key_count: 0,
+          contents: [],
+        })
+      end
+
+      it "returns false" do
+        response = storage_service.folder_present?("my_folder")
+        expect(response).to be_falsey
+      end
     end
   end
 end