CLDC-2310 add creation method to both log import services, lettings log field import and create sales log field import (#1744)

* some minor refactoring
remove methods from child class that replicate methods on the parent class
tidy up check for nil
remove gubbins and inline method body given only used once

* update import services for lettings and sales to import creation method
write tests to cover this

* create sales log field import service and associated spec file, with methods and tests for importing the creation method of logs that have already been imported

* update lettings log field import service and related spec to allow importing creation method of logs

* use the methods dynamically created by active record in all relevant places, removing obsolete methods in teh process.
various tests tweaked to suppor this change.
rake task from another ticket folded into this ticket to prevent merge conflicts

* rename method for ruby conventions

* update PR for altered spec
upload id now decided to be a better indicator of bulk upload status, import service amended accordingly
tests updated in line with this

* update field import services in line with import services to use upload id rather than upload method as the source of truth for how a log was created

* slight refactor to reduce nesting and dodge linter complaints

* minor amendment to log creator spec in bulk upload to use enum dynamic methods

app/components/check_answers_summary_list_card_component.rb

@@ -35,17 +35,13 @@ class CheckAnswersSummaryListCardComponent < ViewComponent::Base
 private
 
   def unanswered_value
-    if bulk_uploaded?
+    if log.creation_method_bulk_upload?
       "<span class=\"app-!-colour-red\">You still need to answer this question</span>".html_safe
     else
       "<span class=\"app-!-colour-muted\">You didn’t answer this question</span>".html_safe
     end
   end
 
-  def bulk_uploaded?
-    log.bulk_upload
-  end
-
   def number_of_buyers
     log[:jointpur] == 1 ? 2 : 1
   end

app/helpers/check_answers_helper.rb

@@ -54,14 +54,10 @@ private
   end
 
   def unanswered_value(log:)
-    if bulk_uploaded?(log:)
+    if log.creation_method_bulk_upload?
       "<span class=\"app-!-colour-red\">You still need to answer this question</span>".html_safe
     else
       "<span class=\"app-!-colour-muted\">You didn’t answer this question</span>".html_safe
     end
   end
-
-  def bulk_uploaded?(log:)
-    log.bulk_upload
-  end
 end

app/helpers/log_actions_helper.rb

@@ -16,7 +16,7 @@ private
 
   def back_button_for(log)
     if log.completed?
-      if log.bulk_uploaded?
+      if log.creation_method_bulk_upload?
         if log.lettings?
           govuk_button_link_to "Back to uploaded logs", resume_bulk_upload_lettings_result_path(log.bulk_upload)
         else

app/models/log.rb

@@ -24,7 +24,7 @@ class Log < ApplicationRecord
     "single log" => 1,
     "bulk upload" => 2,
   }.freeze
-  enum creation_method: CREATION_METHOD
+  enum creation_method: CREATION_METHOD, _prefix: true
 
   scope :visible, -> { where(status: %w[not_started in_progress completed]) }
   scope :exportable, -> { where(status: %w[not_started in_progress completed deleted]) }
@@ -184,10 +184,6 @@ class Log < ApplicationRecord
     end
   end
 
-  def bulk_uploaded?
-    bulk_upload_id.present?
-  end
-
   def collection_closed_for_editing?
     form.edit_end_date < Time.zone.now || older_than_previous_collection_year?
   end

app/services/bulk_upload/lettings/log_creator.rb

@@ -14,7 +14,7 @@ class BulkUpload::Lettings::LogCreator
 
       row_parser.log.blank_invalid_non_setup_fields!
       row_parser.log.bulk_upload = bulk_upload
-      row_parser.log.creation_method = "bulk upload"
+      row_parser.log.creation_method_bulk_upload!
       row_parser.log.skip_update_status = true
       row_parser.log.status = "pending"
       row_parser.log.status_cache = row_parser.log.calculate_status

app/services/bulk_upload/sales/log_creator.rb

@@ -14,7 +14,7 @@ class BulkUpload::Sales::LogCreator
 
       row_parser.log.blank_invalid_non_setup_fields!
       row_parser.log.bulk_upload = bulk_upload
-      row_parser.log.creation_method = "bulk upload"
+      row_parser.log.creation_method_bulk_upload!
       row_parser.log.skip_update_status = true
       row_parser.log.status = "pending"
       row_parser.log.status_cache = row_parser.log.calculate_status

app/services/imports/lettings_logs_field_import_service.rb

@@ -10,6 +10,8 @@ module Imports
         import_from(folder, :update_lettings_allocation)
       when "offered"
         import_from(folder, :update_offered)
+      when "creation_method"
+        import_from(folder, :update_creation_method)
       else
         raise "Updating #{field} is not supported by the field import service"
       end
@@ -36,6 +38,24 @@ module Imports
       end
     end
 
+    def update_creation_method(xml_doc)
+      old_id = meta_field_value(xml_doc, "document-id")
+      log = LettingsLog.find_by(old_id:)
+
+      return @logger.warn "lettings log with old id #{old_id} not found" unless log
+
+      upload_id = meta_field_value(xml_doc, "upload-id")
+
+      if upload_id.nil?
+        @logger.info "lettings log with old id #{old_id} entered manually, no need for update"
+      elsif log.creation_method_bulk_upload?
+        @logger.info "lettings log #{log.id} creation method already set to bulk upload, no need for update"
+      else
+        log.creation_method_bulk_upload!
+        @logger.info "lettings log #{log.id} creation method set to bulk upload"
+      end
+    end
+
     def update_lettings_allocation(xml_doc)
       return if meta_field_value(xml_doc, "form-name").include?("Sales")
 
@@ -110,31 +130,5 @@ module Imports
         @logger.warn("Could not find record matching legacy ID #{old_id}")
       end
     end
-
-    def compose_date(xml_doc, day_str, month_str, year_str)
-      day = Integer(field_value(xml_doc, "xmlns", day_str), exception: false)
-      month = Integer(field_value(xml_doc, "xmlns", month_str), exception: false)
-      year = Integer(field_value(xml_doc, "xmlns", year_str), exception: false)
-      if day.nil? || month.nil? || year.nil?
-        nil
-      else
-        Time.zone.local(year, month, day)
-      end
-    end
-
-    def string_or_nil(xml_doc, attribute)
-      str = field_value(xml_doc, "xmlns", attribute)
-      str.presence
-    end
-
-    # Unsafe: A string that has more than just the integer value
-    def unsafe_string_as_integer(xml_doc, attribute)
-      str = string_or_nil(xml_doc, attribute)
-      if str.nil?
-        nil
-      else
-        str.to_i
-      end
-    end
   end
 end

app/services/imports/lettings_logs_import_service.rb

@@ -66,6 +66,7 @@ module Imports
       attributes["startdate"] = compose_date(xml_doc, "DAY", "MONTH", "YEAR")
       attributes["owning_organisation_id"] = find_organisation_id(xml_doc, "OWNINGORGID")
       attributes["managing_organisation_id"] = find_organisation_id(xml_doc, "MANINGORGID")
+      attributes["creation_method"] = creation_method(xml_doc)
       attributes["joint"] = unsafe_string_as_integer(xml_doc, "joint")
       attributes["startertenancy"] = unsafe_string_as_integer(xml_doc, "_2a")
       attributes["tenancy"] = unsafe_string_as_integer(xml_doc, "Q2b")

app/services/imports/logs_import_service.rb

@@ -11,11 +11,7 @@ module Imports
     # Unsafe: A string that has more than just the integer value
     def unsafe_string_as_integer(xml_doc, attribute)
       str = string_or_nil(xml_doc, attribute)
-      if str.nil?
+      str&.to_i
-        nil
-      else
-        str.to_i
-      end
     end
 
     def compose_date(xml_doc, day_str, month_str, year_str)
@@ -29,6 +25,11 @@ module Imports
       end
     end
 
+    def creation_method(xml_doc)
+      upload_id = meta_field_value(xml_doc, "upload-id")
+      upload_id.present? ? "bulk upload" : "single log"
+    end
+
     def find_organisation_id(xml_doc, id_field)
       old_visible_id = string_or_nil(xml_doc, id_field)
       organisation = Organisation.find_by(old_visible_id:)

app/services/imports/sales_logs_field_import_service.rb

@@ -0,0 +1,32 @@
+module Imports
+  class SalesLogsFieldImportService < LogsImportService
+    def update_field(field, folder)
+      case field
+      when "creation_method"
+        import_from(folder, :update_creation_method)
+      else
+        raise "Updating #{field} is not supported by the field import service"
+      end
+    end
+
+  private
+
+    def update_creation_method(xml_doc)
+      old_id = meta_field_value(xml_doc, "document-id")
+      log = SalesLog.find_by(old_id:)
+
+      return @logger.warn "sales log with old id #{old_id} not found" unless log
+
+      upload_id = meta_field_value(xml_doc, "upload-id")
+
+      if upload_id.nil?
+        @logger.info "sales log with old id #{old_id} entered manually, no need for update"
+      elsif log.creation_method_bulk_upload?
+        @logger.info "sales log #{log.id} creation method already set to bulk upload, no need for update"
+      else
+        log.creation_method_bulk_upload!
+        @logger.info "sales log #{log.id} creation method set to bulk upload"
+      end
+    end
+  end
+end

app/services/imports/sales_logs_import_service.rb

@@ -31,6 +31,7 @@ module Imports
       attributes["owning_organisation_id"] = find_organisation_id(xml_doc, "OWNINGORGID")
       attributes["type"] = unsafe_string_as_integer(xml_doc, "DerSaleType")
       attributes["old_id"] = meta_field_value(xml_doc, "document-id")
+      attributes["creation_method"] = creation_method(xml_doc)
       attributes["created_at"] = Time.zone.parse(meta_field_value(xml_doc, "created-date"))
       attributes["updated_at"] = Time.zone.parse(meta_field_value(xml_doc, "modified-date"))
       attributes["purchid"] = string_or_nil(xml_doc, "PurchaserCode")

lib/tasks/creation_method.rake

@@ -0,0 +1,5 @@
+desc "set creation method to bulk upload if a log has a bulk upload id"
+task set_creation_method: :environment do
+  LettingsLog.where.not(bulk_upload_id: nil).find_each(&:creation_method_bulk_upload!)
+  SalesLog.where.not(bulk_upload_id: nil).find_each(&:creation_method_bulk_upload!)
+end

spec/components/check_answers_summary_list_card_component_spec.rb

@@ -40,8 +40,7 @@ RSpec.describe CheckAnswersSummaryListCardComponent, type: :component do
       context "when log was created via a bulk upload and has an unanswered question" do
         subject(:component) { described_class.new(questions:, log:, user:) }
 
-        let(:bulk_upload) { build(:bulk_upload, :lettings) }
+        let(:log) { build(:lettings_log, :in_progress, creation_method: "bulk upload", age2: 99, startdate: Time.zone.local(2021, 5, 1)) }
-        let(:log) { build(:lettings_log, :in_progress, bulk_upload:, age2: 99, startdate: Time.zone.local(2021, 5, 1)) }
 
         it "displays tweaked copy in red" do
           expect(rendered).to have_selector("span", class: "app-!-colour-red", text: "You still need to answer this question")

spec/fixtures/imports/logs/166fc004-392e-47a8-acb8-1c018734882b.xml

@@ -9,7 +9,8 @@
     <meta:modified-date>2022-04-12T14:10:59.953121Z</meta:modified-date>
     <meta:status>submitted-valid</meta:status>
     <meta:reporting-year>2021</meta:reporting-year>
-    <meta:upload-method>Manual Entry</meta:upload-method>
+    <meta:upload-method>Bulk Upload</meta:upload-method>
+    <meta:upload-id>8dda8f1a-f5a1-4827-8d82-dd7fd9258eab</meta:upload-id>
     <meta:schema assert-valid="true"/>
     <meta:rules assert-valid="true"/>
   </meta:metadata>

spec/fixtures/imports/sales_logs/shared_ownership_sales_log2.xml

@@ -9,7 +9,8 @@
     <meta:modified-date>2023-02-22T11:00:06.575832Z</meta:modified-date>
     <meta:status>submitted-valid</meta:status>
     <meta:reporting-year>2022</meta:reporting-year>
-    <meta:upload-method>Manual Entry</meta:upload-method>
+    <meta:upload-method>Bulk Upload</meta:upload-method>
+    <meta:upload-id>8dda8f1a-f5a1-4827-8d82-dd7fd9258eab</meta:upload-id>
     <meta:schema assert-valid="true"/>
     <meta:rules assert-valid="true"/>
   </meta:metadata>

spec/helpers/check_answers_helper_spec.rb

@@ -39,8 +39,7 @@ RSpec.describe CheckAnswersHelper do
   describe "#get_answer_label" do
     context "when unanswered and bulk upload" do
       let(:question) { log.form.questions.sample }
-      let(:bulk_upload) { build(:bulk_upload, :sales) }
+      let(:log) { build(:sales_log, creation_method: "bulk upload") }
-      let(:log) { build(:sales_log, bulk_upload:) }
 
       it "is red" do
         expect(get_answer_label(question, log)).to include("red")

spec/services/bulk_upload/lettings/log_creator_spec.rb

@@ -31,7 +31,7 @@ RSpec.describe BulkUpload::Lettings::LogCreator do
       it "sets the creation method" do
         service.call
 
-        expect(LettingsLog.last.creation_method).to eq "bulk upload"
+        expect(LettingsLog.last.creation_method_bulk_upload?).to be true
       end
     end
 

spec/services/bulk_upload/sales/log_creator_spec.rb

@@ -38,7 +38,7 @@ RSpec.describe BulkUpload::Sales::LogCreator do
       it "sets the creation method" do
         service.call
 
-        expect(SalesLog.last.creation_method).to eq "bulk upload"
+        expect(SalesLog.last.creation_method_bulk_upload?).to be true
       end
     end
 

spec/services/imports/lettings_logs_field_import_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Imports::LettingsLogsFieldImportService do
   let(:fixture_directory) { "spec/fixtures/imports/logs" }
 
   let(:lettings_log_id) { "0ead17cb-1668-442d-898c-0d52879ff592" }
-  let(:lettings_log_file) { open_file(fixture_directory, lettings_log_id) }
+  let(:lettings_log_file) { File.open("#{fixture_directory}/#{lettings_log_id}.xml") }
   let(:lettings_log_xml) { Nokogiri::XML(lettings_log_file) }
   let(:remote_folder) { "lettings_logs" }
   let(:old_user_id) { "c3061a2e6ea0b702e6f6210d5c52d2a92612d2aa" }
@@ -21,12 +21,6 @@ RSpec.describe Imports::LettingsLogsFieldImportService do
       Singleton.__init__(FormHandler)
       example.run
     end
-    Timecop.return
-    Singleton.__init__(FormHandler)
-  end
-
-  def open_file(directory, filename)
-    File.open("#{directory}/#{filename}.xml")
   end
 
   before do
@@ -83,7 +77,51 @@ RSpec.describe Imports::LettingsLogsFieldImportService do
     end
   end
 
-  context "when updating letings allocation values" do
+  context "when updating creation method" do
+    let(:field) { "creation_method" }
+    let(:lettings_log) { LettingsLog.find_by(old_id: lettings_log_id) }
+
+    before do
+      Imports::LettingsLogsImportService.new(storage_service, logger).create_logs(fixture_directory)
+      lettings_log_file.rewind
+    end
+
+    context "and the log was manually entered" do
+      it "logs that bulk upload id does not need setting" do
+        expect(logger).to receive(:info).with("lettings log with old id #{lettings_log_id} entered manually, no need for update")
+        expect { import_service.update_field(field, remote_folder) }.not_to(change { lettings_log.reload.creation_method })
+      end
+    end
+
+    context "and the log was bulk uploaded and the creation method is already correct" do
+      let(:lettings_log_id) { "166fc004-392e-47a8-acb8-1c018734882b" }
+
+      it "logs that bulk upload id does not need setting" do
+        expect(logger).to receive(:info).with(/lettings log \d+ creation method already set to bulk upload, no need for update/)
+        expect { import_service.update_field(field, remote_folder) }.not_to(change { lettings_log.reload.creation_method })
+      end
+    end
+
+    context "and the log was bulk uploaded and the creation method requires updating" do
+      let(:lettings_log_id) { "166fc004-392e-47a8-acb8-1c018734882b" }
+
+      it "logs that bulk upload id does not need setting" do
+        lettings_log.creation_method_single_log!
+        expect(logger).to receive(:info).with(/lettings log \d+ creation method set to bulk upload/)
+        expect { import_service.update_field(field, remote_folder) }.to change { lettings_log.reload.creation_method }.to "bulk upload"
+      end
+    end
+
+    context "and the log was not previously imported" do
+      it "logs a warning that the log has not been found in the db" do
+        lettings_log.destroy!
+        expect(logger).to receive(:warn).with("lettings log with old id #{lettings_log_id} not found")
+        import_service.update_field(field, remote_folder)
+      end
+    end
+  end
+
+  context "when updating lettings allocation values" do
     let(:field) { "lettings_allocation" }
     let(:lettings_log) { LettingsLog.find_by(old_id: lettings_log_id) }
 

spec/services/imports/lettings_logs_import_service_spec.rb

@@ -478,6 +478,29 @@ RSpec.describe Imports::LettingsLogsImportService do
         end
       end
 
+      context "when the log being imported was manually entered" do
+        it "sets the creation method correctly" do
+          lettings_log_service.send(:create_log, lettings_log_xml)
+
+          lettings_log = LettingsLog.find_by(old_id: lettings_log_id)
+          expect(lettings_log.creation_method_single_log?).to be true
+        end
+      end
+
+      context "when the log being imported was bulk uploaded" do
+        before do
+          metadata = lettings_log_xml.at_xpath("//meta:metadata", { "meta" => "http://data.gov.uk/core/metadata" })
+          metadata << "<meta:upload-id>#{SecureRandom.uuid}</meta:upload-id>"
+        end
+
+        it "sets the creation method correctly" do
+          lettings_log_service.send(:create_log, lettings_log_xml)
+
+          lettings_log = LettingsLog.find_by(old_id: lettings_log_id)
+          expect(lettings_log.creation_method_bulk_upload?).to be true
+        end
+      end
+
       context "and income over the max" do
         before do
           lettings_log_xml.at_xpath("//xmlns:Q8Money").content = "25000"

spec/services/imports/sales_logs_field_import_service_spec.rb

@@ -0,0 +1,76 @@
+require "rails_helper"
+
+RSpec.describe Imports::SalesLogsFieldImportService do
+  subject(:import_service) { described_class.new(storage_service, logger) }
+
+  let(:storage_service) { instance_double(Storage::S3Service) }
+  let(:logger) { instance_double(ActiveSupport::Logger) }
+
+  let(:fixture_directory) { "spec/fixtures/imports/sales_logs" }
+  let(:sales_log_filename) { "shared_ownership_sales_log" }
+  let(:sales_log_file) { File.open("#{fixture_directory}/#{sales_log_filename}.xml") }
+  let(:organisation) { create(:organisation, old_visible_id: "1") }
+  let(:old_user_id) { "c3061a2e6ea0b702e6f6210d5c52d2a92612d2aa" }
+
+  let(:remote_folder) { "sales_logs" }
+
+  before do
+    create(:user, old_user_id:, organisation:)
+
+    allow(storage_service)
+      .to receive(:list_files)
+      .and_return(["#{sales_log_filename}.xml"])
+    allow(storage_service)
+      .to receive(:get_file_io)
+      .with("#{sales_log_filename}.xml")
+      .and_return(sales_log_file)
+  end
+
+  context "when updating creation method" do
+    let(:field) { "creation_method" }
+    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
+    end
+
+    context "and the log was manually entered" do
+      let(:sales_log_filename) { "shared_ownership_sales_log" }
+
+      it "logs that bulk upload id does not need setting" do
+        expect(logger).to receive(:info).with("sales log with old id #{sales_log_filename} entered manually, no need for update")
+        expect { import_service.update_field(field, remote_folder) }.not_to(change { sales_log.reload.creation_method })
+      end
+    end
+
+    context "and the log was bulk uploaded and the creation method is already correct" do
+      let(:sales_log_filename) { "shared_ownership_sales_log2" }
+
+      it "logs that bulk upload id does not need setting" do
+        expect(logger).to receive(:info).with(/sales log \d+ creation method already set to bulk upload, no need for update/)
+        expect { import_service.update_field(field, remote_folder) }.not_to(change { sales_log.reload.creation_method })
+      end
+    end
+
+    context "and the log was bulk uploaded and the creation method requires updating" do
+      let(:sales_log_filename) { "shared_ownership_sales_log2" }
+
+      it "logs that bulk upload id does not need setting" do
+        sales_log.creation_method_single_log!
+        expect(logger).to receive(:info).with(/sales log \d+ creation method set to bulk upload/)
+        expect { import_service.update_field(field, remote_folder) }.to change { sales_log.reload.creation_method }.to "bulk upload"
+      end
+    end
+
+    context "and the log was not previously imported" do
+      let(:sales_log_filename) { "shared_ownership_sales_log" }
+
+      it "logs a warning that the log has not been found in the db" do
+        sales_log.destroy!
+        expect(logger).to receive(:warn).with("sales log with old id #{sales_log_filename} not found")
+        import_service.update_field(field, remote_folder)
+      end
+    end
+  end
+end

spec/services/imports/sales_logs_import_service_spec.rb

@@ -263,8 +263,6 @@ RSpec.describe Imports::SalesLogsImportService do
           Singleton.__init__(FormHandler)
           example.run
         end
-        Timecop.return
-        Singleton.__init__(FormHandler)
       end
 
       before do
@@ -1056,6 +1054,28 @@ RSpec.describe Imports::SalesLogsImportService do
         end
       end
 
+      context "when the log being imported was manually entered" do
+        let(:sales_log_id) { "shared_ownership_sales_log" }
+
+        it "sets the creation method correctly" do
+          sales_log_service.send(:create_log, sales_log_xml)
+
+          sales_log = SalesLog.find_by(old_id: sales_log_id)
+          expect(sales_log.creation_method_single_log?).to be true
+        end
+      end
+
+      context "when the log being imported was bulk uploaded" do
+        let(:sales_log_id) { "shared_ownership_sales_log2" }
+
+        it "sets the creation method correctly" do
+          sales_log_service.send(:create_log, sales_log_xml)
+
+          sales_log = SalesLog.find_by(old_id: sales_log_id)
+          expect(sales_log.creation_method_bulk_upload?).to be true
+        end
+      end
+
       context "when inferring age known" do
         let(:sales_log_id) { "discounted_ownership_sales_log" }
 

spec/views/logs/edit.html.erb_spec.rb

@@ -69,7 +69,7 @@ RSpec.describe "logs/edit.html.erb" do
 
     context "when lettings log is bulk uploaded" do
       let(:bulk_upload) { create(:bulk_upload, :lettings) }
-      let(:log) { create(:lettings_log, :completed, bulk_upload:) }
+      let(:log) { create(:lettings_log, :completed, bulk_upload:, creation_method: "bulk upload") }
 
       it "has link 'Back to uploaded logs'" do
         render
@@ -90,7 +90,7 @@ RSpec.describe "logs/edit.html.erb" do
 
     context "when sales log is bulk uploaded" do
       let(:bulk_upload) { create(:bulk_upload, :sales) }
-      let(:log) { create(:sales_log, :completed, bulk_upload:) }
+      let(:log) { create(:sales_log, :completed, bulk_upload:, creation_method: "bulk upload") }
 
       it "has link 'Back to uploaded logs'" do
         render