Merge pull request #1570 from alphagov/org-fetch-apha

Manually set APHA parent to Defra

lib/organisations_fetcher.rb

@@ -5,23 +5,26 @@
 # membership of organisations.
 class OrganisationsFetcher
   def call
-    organisation_relationships = {}
-    organisations.each do |organisation_data|
+    organisation_relationships = organisations.each_with_object({}) do |organisation_data, memo|
       update_or_create_organisation(organisation_data)
-      organisation_relationships[organisation_data["details"]["slug"]] = child_organisation_slugs(organisation_data)
+      memo[organisation_data["details"]["slug"]] = child_organisation_slugs(organisation_data)
     end
-    # Now that any new organisations have been created and any slug changes
-    # have been applied, we can safely tie together organisations
+
     update_ancestry(organisation_relationships)
+
+    fix_parents_manually
   rescue ActiveRecord::RecordInvalid => e
     raise "Couldn't save organisation #{e.record.slug} because: #{e.record.errors.full_messages.join(',')}"
   end
 
+  MANUAL_PARENT_FIXES = {
+    "animal-and-plant-health-agency" => "department-for-environment-food-rural-affairs",
+  }.freeze
+
 private
 
   def organisations
-    base_uri = Plek.new.website_root
-    GdsApi::Organisations.new(base_uri).organisations.with_subsequent_pages
+    @organisations ||= GdsApi.organisations.organisations.with_subsequent_pages
   end
 
   def update_or_create_organisation(organisation_data)
@@ -40,23 +43,35 @@ def update_or_create_organisation(organisation_data)
       abbreviation: organisation_data["details"]["abbreviation"],
       closed: organisation_data["details"]["govuk_status"] == "closed",
     }
+
     organisation.update!(update_data)
   end
 
   def child_organisation_slugs(organisation_data)
-    organisation_data["child_organisations"].collect { |child_organisation| child_organisation["id"].split("/").last }
+    organisation_data["child_organisations"].map { |child_organisation| child_organisation["id"].split("/").last }
   end
 
   def update_ancestry(organisation_relationships)
     organisation_relationships.each do |organisation_slug, child_organisation_slugs|
-      parent = Organisation.find_by(slug: organisation_slug)
-      Organisation.where(slug: child_organisation_slugs).map do |child_organisation|
-        # TODO: this ignores that organisations can have multiple parents. I think organisations will
-        # end up with the parent that appears last in the API response(s).
-        #
-        # Transition app implements this correctly.
+      parent = Organisation.find_by!(slug: organisation_slug)
+      Organisation.where(slug: child_organisation_slugs).find_each do |child_organisation|
+        # TODO: This ignores that organisations can have multiple parents. Instead the
+        # chosen parent will be the last parent in Whitehall, ordered alphabetically.
         child_organisation.update!(parent: parent)
       end
     end
   end
+
+  def fix_parents_manually
+    # Signon doesn't support multiple parents. Most of the time this is fine, but for
+    # certain organisations this leads to a poor user experience and frequent support
+    # tickets.
+    MANUAL_PARENT_FIXES.each do |child_slug, parent_slug|
+      child = Organisation.find_by(slug: child_slug)
+      parent = Organisation.find_by(slug: parent_slug)
+      next unless child && parent
+
+      child.update!(parent: parent)
+    end
+  end
 end

test/lib/organisations_fetcher_test.rb

@@ -113,4 +113,17 @@ class OrganisationsFetcherTest < ActiveSupport::TestCase
       OrganisationsFetcher.new.call
     end
   end
+
+  OrganisationsFetcher::MANUAL_PARENT_FIXES.each do |child_slug, parent_slug|
+    test "it manually fixes #{child_slug}" do
+      child = create(:organisation, name: "Child", slug: child_slug)
+      parent = create(:organisation, name: "Parent", slug: parent_slug)
+
+      stub_organisations_api_has_organisations([child.slug, parent.slug])
+
+      OrganisationsFetcher.new.call
+
+      assert_equal child.reload.parent, parent
+    end
+  end
 end