Merge pull request #9179 from alphagov/stop-overwhelming-publishing-api

Update Publishing API in more predictable order when toggling reshuffle mode

app/controllers/admin/cabinet_ministers_controller.rb

@@ -17,7 +17,7 @@ def reorder_cabinet_minister_roles
 
   def order_cabinet_minister_roles
     MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :seniority)
-    republish_ministers_index_page_to_publishing_api
+    patch_links_ministers_index_page_to_publishing_api
 
     redirect_to admin_cabinet_ministers_path(anchor: "cabinet_minister")
   end
@@ -28,7 +28,7 @@ def reorder_also_attends_cabinet_roles
 
   def order_also_attends_cabinet_roles
     MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :seniority)
-    republish_ministers_index_page_to_publishing_api
+    patch_links_ministers_index_page_to_publishing_api
 
     redirect_to admin_cabinet_ministers_path(anchor: "also_attends_cabinet")
   end
@@ -39,7 +39,7 @@ def reorder_whip_roles
 
   def order_whip_roles
     MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :whip_ordering)
-    republish_ministers_index_page_to_publishing_api
+    patch_links_ministers_index_page_to_publishing_api
 
     redirect_to admin_cabinet_ministers_path(anchor: "whips")
   end
@@ -50,7 +50,7 @@ def reorder_ministerial_organisations
 
   def order_ministerial_organisations
     Organisation.reorder_without_callbacks!(order_ministerial_organisations_params, :ministerial_ordering)
-    republish_ministers_index_page_to_publishing_api
+    patch_links_ministers_index_page_to_publishing_api
 
     redirect_to admin_cabinet_ministers_path(anchor: "organisations")
   end

app/models/concerns/reshuffle_mode.rb

@@ -2,25 +2,20 @@ module ReshuffleMode
   extend ActiveSupport::Concern
 
   included do
-    def republish_ministerial_pages_to_publishing_api
-      PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter", update_live?)
-      PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter", update_live?)
-    end
-
-    def republish_ministers_index_page_to_publishing_api
-      PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter", update_live?)
+    def patch_links_ministers_index_page_to_publishing_api
+      PatchLinksPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter")
     end
 
     def republish_how_government_works_page_to_publishing_api
-      PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter", update_live?)
+      if reshuffle_in_progress?
+        PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksEnableReshufflePresenter")
+      else
+        PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter")
+      end
     end
 
     def reshuffle_in_progress?
       SitewideSetting.find_by(key: :minister_reshuffle_mode)&.on || false
     end
   end
-
-  def update_live?
-    !reshuffle_in_progress?
-  end
 end

app/models/ministerial_role.rb

@@ -7,7 +7,7 @@ class MinisterialRole < Role
   has_many :news_articles, -> { where("editions.type" => "NewsArticle").distinct }, through: :role_appointments
   has_many :speeches, through: :role_appointments
 
-  after_save :republish_ministerial_pages_to_publishing_api
+  after_save :patch_links_ministers_index_page_to_publishing_api, :republish_how_government_works_page_to_publishing_api
 
   def published_speeches(options = {})
     speeches

app/models/organisation.rb

@@ -208,9 +208,9 @@ def foi_contacts
   before_destroy { |r| throw :abort unless r.destroyable? }
   after_save :ensure_analytics_identifier
   after_save :republish_how_government_works_page_to_publishing_api, :republish_organisations_index_page_to_publishing_api
-  after_save :republish_ministers_index_page_to_publishing_api, if: :ministerial_department?
+  after_save :patch_links_ministers_index_page_to_publishing_api, if: :ministerial_department?
   after_destroy :republish_organisations_index_page_to_publishing_api
-  after_destroy :republish_ministers_index_page_to_publishing_api, if: :ministerial_department?
+  after_destroy :patch_links_ministers_index_page_to_publishing_api, if: :ministerial_department?
 
   after_save do
     # If the organisation has an about us page and the chart URL changes we need

app/models/person.rb

@@ -39,7 +39,7 @@ class Person < ApplicationRecord
 
   before_destroy :prevent_destruction_if_appointed
   after_update :touch_role_appointments, :republish_past_prime_ministers_page_to_publishing_api
-  after_update :republish_ministerial_pages_to_publishing_api, if: :has_ministerial_appointments?
+  after_update :patch_links_ministers_index_page_to_publishing_api, :republish_how_government_works_page_to_publishing_api, if: :has_ministerial_appointments?
 
   def biography_without_markup
     Govspeak::Document.new(biography).to_text

app/models/role_appointment.rb

@@ -74,7 +74,7 @@ def validate(record)
   after_create :make_other_current_appointments_non_current
   before_destroy :prevent_destruction_unless_destroyable
 
-  after_save :republish_ministerial_pages_to_publishing_api, if: :ministerial?
+  after_save :patch_links_ministers_index_page_to_publishing_api, :republish_how_government_works_page_to_publishing_api, if: :ministerial?
   after_save :republish_associated_editions_to_publishing_api, :republish_organisation_to_publishing_api, :republish_worldwide_organisations_to_publishing_api, :republish_prime_ministers_index_page_to_publishing_api, :republish_role_to_publishing_api
   after_destroy :republish_associated_editions_to_publishing_api, :republish_organisation_to_publishing_api, :republish_worldwide_organisations_to_publishing_api, :republish_prime_ministers_index_page_to_publishing_api, :republish_role_to_publishing_api
 

app/models/sitewide_setting.rb

@@ -17,17 +17,33 @@ def name
   def republish_downstream_if_reshuffle
     return unless key == "minister_reshuffle_mode"
 
-    update_live = true
+    ministers_index = PublishingApi::MinistersIndexPresenter.new
     if on
       # These have to be sent synchronously so we can guarantee the order in which they're processed.
-      # First, send 'page is currently being updated' message to Draft and promote to Live
-      PresentPageToPublishingApiWorker.new.perform("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", update_live)
-      PresentPageToPublishingApiWorker.new.perform("PublishingApi::MinistersIndexEnableReshufflePresenter", update_live)
-      # Finally, send normal ministers index payload to draft so that we can use it as a preview
-      PresentPageToPublishingApiWorker.new.perform("PublishingApi::MinistersIndexPresenter", false)
+      # First, send 'page is currently being updated' message to draft stack
+      Services.publishing_api.put_content(
+        ministers_index.content_id,
+        PublishingApi::MinistersIndexEnableReshufflePresenter.new.content,
+      )
+      # Then promote what's on draft, to live
+      Services.publishing_api.publish(ministers_index.content_id, nil, locale: ministers_index.content[:locale])
+
+      # Then, send normal ministers index payload to draft so that we can use it as a preview
+      Services.publishing_api.put_content(ministers_index.content_id, ministers_index.content)
+      # We're now done with the ministers index page. There is no 'patch links' step required here
+      # as we're only changing body content.
+      #
+      # Finally, we'll republish the How Government Works page, to remove the number of ministers
+      # section. There's no need to maintain separate draft/live versions of this page, so we
+      # can just use the normal worker.
+      PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksEnableReshufflePresenter")
     else
-      PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter", update_live)
-      PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter", update_live)
+      # Whatever is on the draft stack, let's publish to the live stack
+      Services.publishing_api.publish(ministers_index.content_id, nil, locale: ministers_index.content[:locale])
+
+      # Republish How Government Works page (needs link patching, putting to draft and promoting
+      # to live, so use the worker)
+      PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter")
     end
   end
 end

app/workers/patch_links_publishing_api_worker.rb

@@ -0,0 +1,5 @@
+class PatchLinksPublishingApiWorker < WorkerBase
+  def perform(presenter)
+    PresentPageToPublishingApi.new.patch_links(presenter.constantize)
+  end
+end

app/workers/present_page_to_publishing_api_worker.rb

@@ -1,9 +1,5 @@
 class PresentPageToPublishingApiWorker < WorkerBase
-  def perform(presenter, update_live = true)
-    if update_live
-      PresentPageToPublishingApi.new.publish(presenter.constantize)
-    else
-      PresentPageToPublishingApi.new.save_draft(presenter.constantize)
-    end
+  def perform(presenter)
+    PresentPageToPublishingApi.new.publish(presenter.constantize)
   end
 end

db/seeds.rb

@@ -90,7 +90,8 @@
     Person.skip_callback(:commit, :after, :publish_to_publishing_api)
     RoleAppointment.skip_callback(:commit, :after, :publish_to_publishing_api)
     RoleAppointment.skip_callback(:save, :after, :republish_prime_ministers_index_page_to_publishing_api)
-    RoleAppointment.skip_callback(:save, :after, :republish_ministerial_pages_to_publishing_api)
+    RoleAppointment.skip_callback(:save, :after, :patch_links_ministers_index_page_to_publishing_api)
+    RoleAppointment.skip_callback(:save, :after, :republish_how_government_works_page_to_publishing_api)
     HistoricalAccount.skip_callback(:commit, :after, :publish_to_publishing_api)
     HistoricalAccount.skip_callback(:save, :after, :republish_prime_ministers_index_page_to_publishing_api)
 

lib/present_page_to_publishing_api.rb

@@ -6,9 +6,8 @@ def publish(presenter_class)
     Services.publishing_api.publish(payload.content_id, nil, locale: payload.content[:locale])
   end
 
-  def save_draft(presenter_class)
+  def patch_links(presenter_class)
     payload = presenter_class.new
-    Services.publishing_api.put_content(payload.content_id, payload.content)
     Services.publishing_api.patch_links(payload.content_id, links: payload.links)
   end
 end

test/functional/admin/cabinet_ministers_controller_test.rb

@@ -11,13 +11,13 @@ def organisation
     @organisation ||= create(:organisation)
   end
 
-  test "PATCH :order_cabinet_minister_roles should reorder ministerial roles and republish the ministers index page" do
+  test "PATCH :order_cabinet_minister_roles should reorder ministerial roles and patches the ministers index page" do
     role2 = create(:ministerial_role, name: "Non-Executive Director", cabinet_member: true, organisations: [organisation])
     role1 = create(:ministerial_role, name: "Prime Minister", cabinet_member: true, organisations: [organisation])
 
     service = mock
     PresentPageToPublishingApi.expects(:new).returns(service)
-    service.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    service.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       patch :order_cabinet_minister_roles,
@@ -35,13 +35,13 @@ def organisation
     assert_redirected_to admin_cabinet_ministers_path(anchor: "cabinet_minister")
   end
 
-  test "PATCH :order_also_attends_cabinet_roles should reorder people who also attend cabinet and republish the ministers index page" do
+  test "PATCH :order_also_attends_cabinet_roles should reorder people who also attend cabinet and patches the ministers index page" do
     role2 = create(:ministerial_role, name: "Chief Whip and Parliamentary Secretary to the Treasury", attends_cabinet_type_id: 2, organisations: [organisation])
     role1 = create(:ministerial_role, name: "Minister without Portfolio", attends_cabinet_type_id: 1, organisations: [organisation])
 
     service = mock
     PresentPageToPublishingApi.expects(:new).returns(service)
-    service.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    service.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       patch :order_also_attends_cabinet_roles,
@@ -59,13 +59,13 @@ def organisation
     assert_redirected_to admin_cabinet_ministers_path(anchor: "also_attends_cabinet")
   end
 
-  test "PATCH :order_whip_roles should reorder whips and republish the ministers index page" do
+  test "PATCH :order_whip_roles should reorder whips and patches the ministers index page" do
     role2 = create(:ministerial_role, name: "Whip 1", whip_organisation_id: 2, organisations: [organisation])
     role1 = create(:ministerial_role, name: "Whip 2", whip_organisation_id: 2, organisations: [organisation])
 
     service = mock
     PresentPageToPublishingApi.expects(:new).returns(service)
-    service.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    service.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       patch :order_whip_roles,
@@ -84,13 +84,13 @@ def organisation
     assert_redirected_to admin_cabinet_ministers_path(anchor: "whips")
   end
 
-  test "PATCH :order_ministerial_organisations should reorder ministerial organisations and republish the ministers index page" do
+  test "PATCH :order_ministerial_organisations should reorder ministerial organisations and patches the ministers index page" do
     org2 = create(:organisation)
     org1 = create(:organisation)
 
     service = mock
     PresentPageToPublishingApi.expects(:new).returns(service)
-    service.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    service.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       put :order_ministerial_organisations,

test/unit/app/models/ministerial_role_test.rb

@@ -92,7 +92,7 @@ class MinisterialRoleTest < ActiveSupport::TestCase
     create(:ministerial_role_appointment, role:)
 
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter)
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       role.update(name: "New name")

test/unit/app/models/organisation_test.rb

@@ -1120,7 +1120,7 @@ class OrganisationTest < ActiveSupport::TestCase
 
   test "should send related pages to publishing api when a ministerial department is created" do
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter)
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::OrganisationsIndexPresenter)
 
     Sidekiq::Testing.inline! do
@@ -1132,7 +1132,7 @@ class OrganisationTest < ActiveSupport::TestCase
     organisation = create(:ministerial_department)
 
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter)
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::OrganisationsIndexPresenter)
 
     Sidekiq::Testing.inline! do
@@ -1143,7 +1143,7 @@ class OrganisationTest < ActiveSupport::TestCase
   test "should send related pages to publishing api when a ministerial department is deleted" do
     organisation = create(:ministerial_department)
 
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::OrganisationsIndexPresenter)
 
     Sidekiq::Testing.inline! do

test/unit/app/models/person_test.rb

@@ -245,7 +245,7 @@ class PersonTest < ActiveSupport::TestCase
     create(:ministerial_role_appointment, person:)
 
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter)
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       person.update(forename: "New first name")
@@ -258,7 +258,7 @@ class PersonTest < ActiveSupport::TestCase
     create(:role_appointment, person:, role:)
 
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter).never
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter).never
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter).never
 
     Sidekiq::Testing.inline! do
       person.update(forename: "New first name")
@@ -271,7 +271,7 @@ class PersonTest < ActiveSupport::TestCase
 
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HistoricalAccountsIndexPresenter)
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter)
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       person.update(forename: "New first name")
@@ -285,7 +285,7 @@ class PersonTest < ActiveSupport::TestCase
 
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HistoricalAccountsIndexPresenter)
     PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter)
-    PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::MinistersIndexPresenter)
+    PresentPageToPublishingApi.any_instance.expects(:patch_links).with(PublishingApi::MinistersIndexPresenter)
 
     Sidekiq::Testing.inline! do
       person.update(forename: "New first name")