From 8448b559a8dbddb5093f98ae909d537c57b16547 Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Thu, 17 Aug 2023 14:52:48 +0100 Subject: [PATCH] Add owning org id to field import task for sales (#1850) * feat: add field import service and test * feat: use old visible org id * feat: tweak test * feat: update rake task * feat: split sales and lettings rake tasks * feat: update rake task * feat: PR comment tweaks * feat: rename and update tests * refactor: lint --- .../lettings_logs_field_import_service.rb | 2 + .../sales_logs_field_import_service.rb | 23 ++ lib/tasks/data_import_field.rake | 30 ++- spec/lib/tasks/date_import_field_spec.rb | 224 ++++++++++++------ .../sales_logs_field_import_service_spec.rb | 37 +++ 5 files changed, 235 insertions(+), 81 deletions(-) diff --git a/app/services/imports/lettings_logs_field_import_service.rb b/app/services/imports/lettings_logs_field_import_service.rb index e65625445..4e10c7e1c 100644 --- a/app/services/imports/lettings_logs_field_import_service.rb +++ b/app/services/imports/lettings_logs_field_import_service.rb @@ -39,6 +39,8 @@ module Imports end def update_creation_method(xml_doc) + return if meta_field_value(xml_doc, "form-name").include?("Sales") + old_id = meta_field_value(xml_doc, "document-id") log = LettingsLog.find_by(old_id:) diff --git a/app/services/imports/sales_logs_field_import_service.rb b/app/services/imports/sales_logs_field_import_service.rb index 4da19dbc3..e677cc8a0 100644 --- a/app/services/imports/sales_logs_field_import_service.rb +++ b/app/services/imports/sales_logs_field_import_service.rb @@ -4,6 +4,8 @@ module Imports case field when "creation_method" import_from(folder, :update_creation_method) + when "owning_organisation_id" + import_from(folder, :update_owning_organisation_id) else raise "Updating #{field} is not supported by the field import service" end @@ -12,6 +14,8 @@ module Imports private def update_creation_method(xml_doc) + return unless meta_field_value(xml_doc, "form-name").include?("Sales") + old_id = meta_field_value(xml_doc, "document-id") log = SalesLog.find_by(old_id:) @@ -28,5 +32,24 @@ module Imports @logger.info "sales log #{log.id} creation method set to bulk upload" end end + + def update_owning_organisation_id(xml_doc) + return unless meta_field_value(xml_doc, "form-name").include?("Sales") + + old_id = meta_field_value(xml_doc, "document-id") + record = SalesLog.find_by(old_id:) + + if record.present? + if record.owning_organisation_id.present? + @logger.info("sales log #{record.id} has a value for owning_organisation_id, skipping update") + else + owning_organisation_id = find_organisation_id(xml_doc, "OWNINGORGID") + record.update!(owning_organisation_id:) + @logger.info("sales log #{record.id}'s owning_organisation_id value has been set to #{owning_organisation_id}") + end + else + @logger.warn("sales log with old id #{old_id} not found") + end + end end end diff --git a/lib/tasks/data_import_field.rake b/lib/tasks/data_import_field.rake index a30a2ec43..d16ae752e 100644 --- a/lib/tasks/data_import_field.rake +++ b/lib/tasks/data_import_field.rake @@ -1,9 +1,9 @@ namespace :core do - desc "Update database field from data XMLs provided by Softwire" - task :data_import_field, %i[field path] => :environment do |_task, args| + desc "Update lettings log database field from data XMLs provided by Softwire" + task :lettings_data_import_field, %i[field path] => :environment do |_task, args| field = args[:field] path = args[:path] - raise "Usage: rake core:data_import_field['field','path/to/xml_files']" if path.blank? || field.blank? + raise "Usage: rake core:lettings_data_import_field['field','path/to/xml_files']" if path.blank? || field.blank? # We only allow a reduced list of known fields to be updatable case field @@ -17,7 +17,29 @@ namespace :core do Rails.logger.info("Imported") end else - raise "Field #{field} cannot be updated by data_import_field" + raise "Field #{field} cannot be updated by lettings_data_import_field" + end + end + + desc "Update sales log database field from data XMLs provided by Softwire" + task :sales_data_import_field, %i[field path] => :environment do |_task, args| + field = args[:field] + path = args[:path] + raise "Usage: rake core:sales_data_import_field['field','path/to/xml_files']" if path.blank? || field.blank? + + # We only allow a reduced list of known fields to be updatable + case field + when "owning_organisation_id" + s3_service = Storage::S3Service.new(PlatformHelper.is_paas? ? Configuration::PaasConfigurationService.new : Configuration::EnvConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) + archive_io = s3_service.get_file_io(path) + archive_service = Storage::ArchiveService.new(archive_io) + if archive_service.folder_present?("logs") + Rails.logger.info("Start importing field from folder logs") + Imports::SalesLogsFieldImportService.new(archive_service).update_field(field, "logs") + Rails.logger.info("Imported") + end + else + raise "Field #{field} cannot be updated by sales_data_import_field" end end end diff --git a/spec/lib/tasks/date_import_field_spec.rb b/spec/lib/tasks/date_import_field_spec.rb index c34427e04..cac433449 100644 --- a/spec/lib/tasks/date_import_field_spec.rb +++ b/spec/lib/tasks/date_import_field_spec.rb @@ -1,106 +1,176 @@ require "rails_helper" require "rake" -describe "rake core:data_import_field", type: :task do - subject(:task) { Rake::Task["core:data_import_field"] } - - let(:instance_name) { "paas_import_instance" } - let(:storage_service) { instance_double(Storage::S3Service) } - let(:env_config_service) { instance_double(Configuration::EnvConfigurationService) } - let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } - - before do - Rake.application.rake_require("tasks/data_import_field") - Rake::Task.define_task(:environment) - task.reenable - - allow(Storage::S3Service).to receive(:new).and_return(storage_service) - allow(Configuration::EnvConfigurationService).to receive(:new).and_return(env_config_service) - allow(Configuration::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) - allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("dummy") - allow(Imports::LettingsLogsFieldImportService).to receive(:new).and_return(import_service) - end +describe "data_import_field imports" do + context "with rake core:lettings_data_import_field", type: :task do + subject(:task) { Rake::Task["core:lettings_data_import_field"] } - context "when importing a lettings log field" do - let(:import_service) { instance_double(Imports::LettingsLogsFieldImportService) } - let(:fixture_path) { "spec/fixtures/imports/logs" } - let(:archive_service) { instance_double(Storage::ArchiveService) } + let(:instance_name) { "paas_import_instance" } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:env_config_service) { instance_double(Configuration::EnvConfigurationService) } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } before do - allow(import_service).to receive(:update_field) - allow(Storage::ArchiveService).to receive(:new).and_return(archive_service) - allow(archive_service).to receive(:folder_present?).with("logs").and_return(true) + Rake.application.rake_require("tasks/data_import_field") + Rake::Task.define_task(:environment) + task.reenable + + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(Configuration::EnvConfigurationService).to receive(:new).and_return(env_config_service) + allow(Configuration::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) + allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("dummy") + allow(Imports::LettingsLogsFieldImportService).to receive(:new).and_return(import_service) end - context "and we update the tenancycode field" do - let(:field) { "tenancycode" } + context "when importing a lettings log field" do + let(:import_service) { instance_double(Imports::LettingsLogsFieldImportService) } + let(:fixture_path) { "spec/fixtures/imports/logs" } + let(:archive_service) { instance_double(Storage::ArchiveService) } - it "updates the logs from the given XML file when the VCAP_SERVICES environment variable exists" do - expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) - expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") - expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) - expect(import_service).to receive(:update_field).with(field, "logs") - task.invoke(field, fixture_path) + before do + allow(import_service).to receive(:update_field) + allow(Storage::ArchiveService).to receive(:new).and_return(archive_service) + allow(archive_service).to receive(:folder_present?).with("logs").and_return(true) end - it "updates the logs from the given XML file when the VCAP_SERVICES environment variable does not exist" do - allow(ENV).to receive(:[]).with("VCAP_SERVICES") - expect(Storage::S3Service).to receive(:new).with(env_config_service, instance_name) - expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") - expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) - expect(import_service).to receive(:update_field).with(field, "logs") - task.invoke(field, fixture_path) + context "and we update the tenancycode field" do + let(:field) { "tenancycode" } + + it "updates the logs from the given XML file when the VCAP_SERVICES environment variable exists" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") + expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end + + it "updates the logs from the given XML file when the VCAP_SERVICES environment variable does not exist" do + allow(ENV).to receive(:[]).with("VCAP_SERVICES") + expect(Storage::S3Service).to receive(:new).with(env_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") + expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end end - end - context "and we update the lettings_allocation fields" do - let(:field) { "lettings_allocation" } + context "and we update the lettings_allocation fields" do + let(:field) { "lettings_allocation" } - it "updates the logs from the given XML file" do - expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) - expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") - expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) - expect(import_service).to receive(:update_field).with(field, "logs") - task.invoke(field, fixture_path) + it "updates the logs from the given XML file" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") + expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end end - end - context "and we update the major repairs fields" do - let(:field) { "major_repairs" } + context "and we update the major repairs fields" do + let(:field) { "major_repairs" } - it "updates the logs from the given XML file" do - expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) - expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") - expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) - expect(import_service).to receive(:update_field).with(field, "logs") - task.invoke(field, fixture_path) + it "updates the logs from the given XML file" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") + expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end end - end - context "and we update the offered fields" do - let(:field) { "offered" } + context "and we update the offered fields" do + let(:field) { "offered" } - it "updates the logs from the given XML file" do - expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) - expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") - expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) - expect(import_service).to receive(:update_field).with(field, "logs") - task.invoke(field, fixture_path) + it "updates the logs from the given XML file" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs") + expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end + end + + it "raises an exception if no parameters are provided" do + expect { task.invoke }.to raise_error(/Usage/) + end + + it "raises an exception if a single parameter is provided" do + expect { task.invoke("one_parameter") }.to raise_error(/Usage/) end - end - it "raises an exception if no parameters are provided" do - expect { task.invoke }.to raise_error(/Usage/) + it "raises an exception if the field is not supported" do + expect { task.invoke("random_field", "my_path") }.to raise_error("Field random_field cannot be updated by lettings_data_import_field") + end end + end + + context "with rake core:sales_data_import_field", type: :task do + subject(:task) { Rake::Task["core:sales_data_import_field"] } - it "raises an exception if a single parameter is provided" do - expect { task.invoke("one_parameter") }.to raise_error(/Usage/) + let(:instance_name) { "paas_import_instance" } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:env_config_service) { instance_double(Configuration::EnvConfigurationService) } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } + + before do + Rake.application.rake_require("tasks/data_import_field") + Rake::Task.define_task(:environment) + task.reenable + + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(Configuration::EnvConfigurationService).to receive(:new).and_return(env_config_service) + allow(Configuration::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) + allow(ENV).to receive(:[]).with("VCAP_SERVICES").and_return("dummy") + allow(Imports::SalesLogsFieldImportService).to receive(:new).and_return(import_service) end - it "raises an exception if the field is not supported" do - expect { task.invoke("random_field", "my_path") }.to raise_error("Field random_field cannot be updated by data_import_field") + context "when importing a sales log field" do + let(:import_service) { instance_double(Imports::SalesLogsFieldImportService) } + let(:fixture_path) { "spec/fixtures/imports/sales_logs" } + let(:archive_service) { instance_double(Storage::ArchiveService) } + + before do + allow(import_service).to receive(:update_field) + allow(Storage::ArchiveService).to receive(:new).and_return(archive_service) + allow(archive_service).to receive(:folder_present?).with("logs").and_return(true) + end + + context "and we update the owning_organisation_id field" do + let(:field) { "owning_organisation_id" } + + it "updates the logs from the given XML file when the VCAP_SERVICES environment variable exists" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/sales_logs") + expect(Imports::SalesLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end + + it "updates the logs from the given XML file when the VCAP_SERVICES environment variable does not exist" do + allow(ENV).to receive(:[]).with("VCAP_SERVICES") + expect(Storage::S3Service).to receive(:new).with(env_config_service, instance_name) + expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/sales_logs") + expect(Imports::SalesLogsFieldImportService).to receive(:new).with(archive_service) + expect(import_service).to receive(:update_field).with(field, "logs") + task.invoke(field, fixture_path) + end + end + + it "raises an exception if no parameters are provided" do + expect { task.invoke }.to raise_error(/Usage/) + end + + it "raises an exception if a single parameter is provided" do + expect { task.invoke("one_parameter") }.to raise_error(/Usage/) + end + + it "raises an exception if the field is not supported" do + expect { task.invoke("random_field", "my_path") }.to raise_error("Field random_field cannot be updated by sales_data_import_field") + end end end end diff --git a/spec/services/imports/sales_logs_field_import_service_spec.rb b/spec/services/imports/sales_logs_field_import_service_spec.rb index b5bdb22a4..238e91359 100644 --- a/spec/services/imports/sales_logs_field_import_service_spec.rb +++ b/spec/services/imports/sales_logs_field_import_service_spec.rb @@ -73,4 +73,41 @@ RSpec.describe Imports::SalesLogsFieldImportService do end end end + + context "when updating owning_organisation_id" do + let(:field) { "owning_organisation_id" } + let(:sales_log_filename) { "shared_ownership_sales_log" } + + context "when the sales log has no offered value" do + let(:sales_log) { SalesLog.find_by(old_id: sales_log_filename) } + + before do + Imports::SalesLogsImportService.new(storage_service, logger).create_logs(fixture_directory) + sales_log_file.rewind + sales_log.update!(owning_organisation_id: nil) + end + + it "updates the sales_log owning_organisation_id value" do + expect(logger).to receive(:info).with("sales log #{sales_log.id}'s owning_organisation_id value has been set to #{organisation.id}") + expect { import_service.send(:update_field, field, remote_folder) } + .to(change { sales_log.reload.owning_organisation_id }.from(nil).to(organisation.id)) + end + end + + context "when the sales log has a different offered value" do + let(:sales_log) { SalesLog.find_by(old_id: sales_log_filename) } + + before do + Imports::SalesLogsImportService.new(storage_service, logger).create_logs(fixture_directory) + sales_log_file.rewind + sales_log.update!(owning_organisation_id: organisation.id) + end + + it "does not update the sales_log owning_organisation_id value" do + expect(logger).to receive(:info).with(/sales log \d+ has a value for owning_organisation_id, skipping update/) + expect { import_service.send(:update_field, field, remote_folder) } + .not_to(change { sales_log.reload.owning_organisation_id }) + end + end + end end