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

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:)
 

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

lib/tasks/data_import_field.rake

@@ -1,9 +1,9 @@
 namespace :core do
-  desc "Update database field from data XMLs provided by Softwire"
+  desc "Update lettings log database field from data XMLs provided by Softwire"
-  task :data_import_field, %i[field path] => :environment do |_task, args|
+  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

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
+describe "data_import_field imports" do
-  subject(:task) { Rake::Task["core:data_import_field"] }
+  context "with rake core:lettings_data_import_field", type: :task do
-
+    subject(:task) { Rake::Task["core:lettings_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
 
-  context "when importing a lettings log field" do
+    let(:instance_name) { "paas_import_instance" }
-    let(:import_service) { instance_double(Imports::LettingsLogsFieldImportService) }
+    let(:storage_service) { instance_double(Storage::S3Service) }
-    let(:fixture_path) { "spec/fixtures/imports/logs" }
+    let(:env_config_service) { instance_double(Configuration::EnvConfigurationService) }
-    let(:archive_service) { instance_double(Storage::ArchiveService) }
+    let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) }
 
     before do
-      allow(import_service).to receive(:update_field)
+      Rake.application.rake_require("tasks/data_import_field")
-      allow(Storage::ArchiveService).to receive(:new).and_return(archive_service)
+      Rake::Task.define_task(:environment)
-      allow(archive_service).to receive(:folder_present?).with("logs").and_return(true)
+      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
+    context "when importing a lettings log field" do
-      let(:field) { "tenancycode" }
+      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
+      before do
-        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+        allow(import_service).to receive(:update_field)
-        expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs")
+        allow(Storage::ArchiveService).to receive(:new).and_return(archive_service)
-        expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
+        allow(archive_service).to receive(:folder_present?).with("logs").and_return(true)
-        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
+      context "and we update the tenancycode field" do
-        allow(ENV).to receive(:[]).with("VCAP_SERVICES")
+        let(:field) { "tenancycode" }
-        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")
+        it "updates the logs from the given XML file when the VCAP_SERVICES environment variable exists" do
-        expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
+          expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
-        expect(import_service).to receive(:update_field).with(field, "logs")
+          expect(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs")
-        task.invoke(field, fixture_path)
+          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
+      context "and we update the lettings_allocation fields" do
-      let(:field) { "lettings_allocation" }
+        let(:field) { "lettings_allocation" }
 
-      it "updates the logs from the given XML file" do
+        it "updates the logs from the given XML file" do
-        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+          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(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs")
-        expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
+          expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
-        expect(import_service).to receive(:update_field).with(field, "logs")
+          expect(import_service).to receive(:update_field).with(field, "logs")
-        task.invoke(field, fixture_path)
+          task.invoke(field, fixture_path)
+        end
       end
-    end
 
-    context "and we update the major repairs fields" do
+      context "and we update the major repairs fields" do
-      let(:field) { "major_repairs" }
+        let(:field) { "major_repairs" }
 
-      it "updates the logs from the given XML file" do
+        it "updates the logs from the given XML file" do
-        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+          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(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs")
-        expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
+          expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
-        expect(import_service).to receive(:update_field).with(field, "logs")
+          expect(import_service).to receive(:update_field).with(field, "logs")
-        task.invoke(field, fixture_path)
+          task.invoke(field, fixture_path)
+        end
       end
-    end
 
-    context "and we update the offered fields" do
+      context "and we update the offered fields" do
-      let(:field) { "offered" }
+        let(:field) { "offered" }
 
-      it "updates the logs from the given XML file" do
+        it "updates the logs from the given XML file" do
-        expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name)
+          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(storage_service).to receive(:get_file_io).with("spec/fixtures/imports/logs")
-        expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
+          expect(Imports::LettingsLogsFieldImportService).to receive(:new).with(archive_service)
-        expect(import_service).to receive(:update_field).with(field, "logs")
+          expect(import_service).to receive(:update_field).with(field, "logs")
-        task.invoke(field, fixture_path)
+          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
+      it "raises an exception if the field is not supported" do
-      expect { task.invoke }.to raise_error(/Usage/)
+        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
+    let(:instance_name) { "paas_import_instance" }
-      expect { task.invoke("one_parameter") }.to raise_error(/Usage/)
+    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
+    context "when importing a sales log field" do
-      expect { task.invoke("random_field", "my_path") }.to raise_error("Field random_field cannot be updated by data_import_field")
+      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

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