Code review fixes

Add unique indexes for legacy IDs on locations

app/services/imports/scheme_import_service.rb

@@ -8,19 +8,18 @@ module Imports
       old_id = string_or_nil(xml_document, "id")
       status = string_or_nil(xml_document, "status")
 
-      return unless status == "Approved"
-
-      Scheme.create!(
-        owning_organisation_id: find_owning_organisation_id(xml_document),
-        managing_organisation_id: find_managing_organisation_id(xml_document),
-        service_name: string_or_nil(xml_document, "name"),
-        arrangement_type: string_or_nil(xml_document, "arrangement_type"),
-        old_id:,
-        old_visible_id: safe_string_as_integer(xml_document, "visible-id"),
-      )
-    rescue ActiveRecord::RecordNotUnique
-      name = string_or_nil(xml_document, "name")
-      @logger.warn("Scheme #{name} is already present with legacy ID #{old_id}, skipping.")
+      if status == "Approved"
+        Scheme.create!(
+          owning_organisation_id: find_owning_organisation_id(xml_document),
+          managing_organisation_id: find_managing_organisation_id(xml_document),
+          service_name: string_or_nil(xml_document, "name"),
+          arrangement_type: string_or_nil(xml_document, "arrangement_type"),
+          old_id:,
+          old_visible_id: safe_string_as_integer(xml_document, "visible-id"),
+        )
+      else
+        @logger.warn("Scheme with legacy ID #{old_id} is not approved (#{status}), skipping")
+      end
     end
 
   private

app/services/imports/scheme_location_import_service.rb

@@ -9,7 +9,7 @@ module Imports
       schemes = Scheme.where(old_id: management_group)
       raise "Scheme not found with legacy ID #{management_group}" if schemes.empty?
 
-      if schemes.size == 1 && schemes.first&.locations&.empty?
+      if schemes.size == 1 && schemes.first.locations&.empty?
         scheme = update_scheme(schemes.first, xml_document)
       else
         scheme = find_scheme_to_merge(schemes, xml_document)

db/migrate/20220711152629_add_unique_index_for_old_ids.rb

@@ -0,0 +1,5 @@
+class AddUniqueIndexForOldIds < ActiveRecord::Migration[7.0]
+  def change
+    add_index :locations, :old_id, unique: true
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2022_07_11_081400) do
+ActiveRecord::Schema[7.0].define(version: 2022_07_11_152629) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -251,6 +251,7 @@ ActiveRecord::Schema[7.0].define(version: 2022_07_11_081400) do
     t.integer "type_of_unit"
     t.string "old_id"
     t.integer "old_visible_id"
+    t.index ["old_id"], name: "index_locations_on_old_id", unique: true
     t.index ["scheme_id"], name: "index_locations_on_scheme_id"
   end
 

spec/services/imports/scheme_import_service_spec.rb

@@ -55,6 +55,7 @@ RSpec.describe Imports::SchemeImportService do
       before { scheme_xml.at_xpath("//mgmtgroup:status").content = "Temporary" }
 
       it "does not create the scheme" do
+        expect(logger).to receive(:warn).with("Scheme with legacy ID 6d6d7618b58affe2a150a5ef2e9f4765fa6cd05d is not approved (Temporary), skipping")
         expect { scheme_service.create_scheme(scheme_xml) }
           .not_to change(Scheme, :count)
       end

spec/services/imports/scheme_location_import_service_spec.rb

@@ -143,5 +143,15 @@ RSpec.describe Imports::SchemeLocationImportService do
           .not_to change(Location, :count)
       end
     end
+
+    context "and we import the same location twice" do
+      before { location_service.create_scheme_location(location_xml) }
+
+      it "does not create the location" do
+        expect(logger).to receive(:warn).with("Location is already present with legacy ID 0ae7ad6dc0f1cf7ef33c18cc8c108bebc1b4923e, skipping")
+        expect { location_service.create_scheme_location(location_xml) }
+          .not_to change(Location, :count)
+      end
+    end
   end
 end