From b23bc61ca24b047dbccd72f66236f2a68626a01b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Meny?= <smeny@users.noreply.github.com>
Date: Mon, 11 Jul 2022 16:47:35 +0100
Subject: [PATCH] Code review fixes Add unique indexes for legacy IDs on
 locations

---
 app/services/imports/scheme_import_service.rb | 25 +++++++++----------
 .../imports/scheme_location_import_service.rb |  2 +-
 ...0711152629_add_unique_index_for_old_ids.rb |  5 ++++
 db/schema.rb                                  |  3 ++-
 .../imports/scheme_import_service_spec.rb     |  1 +
 .../scheme_location_import_service_spec.rb    | 10 ++++++++
 6 files changed, 31 insertions(+), 15 deletions(-)
 create mode 100644 db/migrate/20220711152629_add_unique_index_for_old_ids.rb

diff --git a/app/services/imports/scheme_import_service.rb b/app/services/imports/scheme_import_service.rb
index c045d1e2f..b68e7e7da 100644
--- a/app/services/imports/scheme_import_service.rb
+++ b/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
diff --git a/app/services/imports/scheme_location_import_service.rb b/app/services/imports/scheme_location_import_service.rb
index aa958632c..8a30f7d0b 100644
--- a/app/services/imports/scheme_location_import_service.rb
+++ b/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)
diff --git a/db/migrate/20220711152629_add_unique_index_for_old_ids.rb b/db/migrate/20220711152629_add_unique_index_for_old_ids.rb
new file mode 100644
index 000000000..401ece0bd
--- /dev/null
+++ b/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
diff --git a/db/schema.rb b/db/schema.rb
index bd7ee6b18..3aa1c43ca 100644
--- a/db/schema.rb
+++ b/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
 
diff --git a/spec/services/imports/scheme_import_service_spec.rb b/spec/services/imports/scheme_import_service_spec.rb
index 95d93ad26..3cfb76c96 100644
--- a/spec/services/imports/scheme_import_service_spec.rb
+++ b/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
diff --git a/spec/services/imports/scheme_location_import_service_spec.rb b/spec/services/imports/scheme_location_import_service_spec.rb
index 3173c9a44..16f6a8d7d 100644
--- a/spec/services/imports/scheme_location_import_service_spec.rb
+++ b/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