From 16bd44b841a48d6c94c52a7bed1095d87bff44e8 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Tue, 11 Jun 2024 15:10:42 +0100 Subject: [PATCH 01/10] Extract `republish_ministerial_pages_to_publishing_api` to concern The same definition (with occasional extra conditionals) was spread across three locations, making it hard to make any changes to the logic around republishing ministerial pages. Extracting these to a concern should make it easier and more reliable to make changes. --- app/models/concerns/reshuffle_mode.rb | 10 ++++++++++ app/models/ministerial_role.rb | 6 +----- app/models/person.rb | 13 +++++++------ app/models/role_appointment.rb | 11 +++-------- 4 files changed, 21 insertions(+), 19 deletions(-) create mode 100644 app/models/concerns/reshuffle_mode.rb diff --git a/app/models/concerns/reshuffle_mode.rb b/app/models/concerns/reshuffle_mode.rb new file mode 100644 index 00000000000..7ac5d4c80aa --- /dev/null +++ b/app/models/concerns/reshuffle_mode.rb @@ -0,0 +1,10 @@ +module ReshuffleMode + extend ActiveSupport::Concern + + included do + def republish_ministerial_pages_to_publishing_api + PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") + PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + end + end +end diff --git a/app/models/ministerial_role.rb b/app/models/ministerial_role.rb index 7929fc3c835..46ac22b096d 100644 --- a/app/models/ministerial_role.rb +++ b/app/models/ministerial_role.rb @@ -1,4 +1,5 @@ class MinisterialRole < Role + include ReshuffleMode include UserOrderable has_many :editions, -> { distinct }, through: :role_appointments @@ -39,9 +40,4 @@ def destroyable? def default_person_name name end - - def republish_ministerial_pages_to_publishing_api - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") - end end diff --git a/app/models/person.rb b/app/models/person.rb index a24fdc33b4b..0955e41b831 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -1,5 +1,6 @@ class Person < ApplicationRecord include PublishesToPublishingApi + include ReshuffleMode has_many :role_appointments, lambda { @@ -37,7 +38,8 @@ class Person < ApplicationRecord translates :biography before_destroy :prevent_destruction_if_appointed - after_update :touch_role_appointments, :republish_ministerial_pages_to_publishing_api + 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? def biography_without_markup Govspeak::Document.new(biography).to_text @@ -156,12 +158,11 @@ def truncated_biography biography&.split(/\n/)&.first end - def republish_ministerial_pages_to_publishing_api - if role_appointments.any?(&:ministerial?) - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") - end + def has_ministerial_appointments? + role_appointments.any?(&:ministerial?) + end + def republish_past_prime_ministers_page_to_publishing_api if current_or_previous_prime_minister? historical_account.republish_to_publishing_api_async if historical_account.present? PresentPageToPublishingApiWorker.perform_async("PublishingApi::HistoricalAccountsIndexPresenter") diff --git a/app/models/role_appointment.rb b/app/models/role_appointment.rb index 37596a47e1a..54f0d1f032a 100644 --- a/app/models/role_appointment.rb +++ b/app/models/role_appointment.rb @@ -2,6 +2,7 @@ class RoleAppointment < ApplicationRecord include DateValidation include HasContentId include PublishesToPublishingApi + include ReshuffleMode date_attributes(:started_at, :ended_at) @@ -73,7 +74,8 @@ def validate(record) after_create :make_other_current_appointments_non_current before_destroy :prevent_destruction_unless_destroyable - 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_ministerial_pages_to_publishing_api, :republish_role_to_publishing_api + after_save :republish_ministerial_pages_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 def republish_associated_editions_to_publishing_api @@ -94,13 +96,6 @@ def republish_worldwide_organisations_to_publishing_api end end - def republish_ministerial_pages_to_publishing_api - if ministerial? - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") - end - end - def republish_prime_ministers_index_page_to_publishing_api PresentPageToPublishingApiWorker.perform_async("PublishingApi::HistoricalAccountsIndexPresenter") unless current? || role.slug != "prime-minister" || has_historical_account? end From 87b6715635fe0e211654eddb8e78a307cb5264e4 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Tue, 11 Jun 2024 15:33:30 +0100 Subject: [PATCH 02/10] Encapsulate remaining MinistersIndex / HowGovWorks presenter calls These all now go through the ReshuffleMode class. --- .../admin/cabinet_ministers_controller.rb | 9 +++++---- app/models/concerns/reshuffle_mode.rb | 8 ++++++++ app/models/organisation.rb | 15 +++++---------- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/app/controllers/admin/cabinet_ministers_controller.rb b/app/controllers/admin/cabinet_ministers_controller.rb index a6cfbdc4d3e..7f92dd83a75 100644 --- a/app/controllers/admin/cabinet_ministers_controller.rb +++ b/app/controllers/admin/cabinet_ministers_controller.rb @@ -1,4 +1,5 @@ class Admin::CabinetMinistersController < Admin::BaseController + include ReshuffleMode before_action :enforce_permissions! def show @@ -14,7 +15,7 @@ def reorder_cabinet_minister_roles def order_cabinet_minister_roles MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :seniority) - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + republish_ministers_index_page_to_publishing_api redirect_to admin_cabinet_ministers_path(anchor: "cabinet_minister") end @@ -25,7 +26,7 @@ def reorder_also_attends_cabinet_roles def order_also_attends_cabinet_roles MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :seniority) - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + republish_ministers_index_page_to_publishing_api redirect_to admin_cabinet_ministers_path(anchor: "also_attends_cabinet") end @@ -36,7 +37,7 @@ def reorder_whip_roles def order_whip_roles MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :whip_ordering) - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + republish_ministers_index_page_to_publishing_api redirect_to admin_cabinet_ministers_path(anchor: "whips") end @@ -47,7 +48,7 @@ def reorder_ministerial_organisations def order_ministerial_organisations Organisation.reorder_without_callbacks!(order_ministerial_organisations_params, :ministerial_ordering) - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + republish_ministers_index_page_to_publishing_api redirect_to admin_cabinet_ministers_path(anchor: "organisations") end diff --git a/app/models/concerns/reshuffle_mode.rb b/app/models/concerns/reshuffle_mode.rb index 7ac5d4c80aa..70ea16dcffd 100644 --- a/app/models/concerns/reshuffle_mode.rb +++ b/app/models/concerns/reshuffle_mode.rb @@ -6,5 +6,13 @@ def republish_ministerial_pages_to_publishing_api PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") end + + def republish_ministers_index_page_to_publishing_api + PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + end + + def republish_how_government_works_page_to_publishing_api + PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") + end end end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 8d45ca033d2..4f32a79be30 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -1,6 +1,7 @@ class Organisation < ApplicationRecord include DateValidation include PublishesToPublishingApi + include ReshuffleMode include Searchable include Organisation::OrganisationSearchIndexConcern include Organisation::OrganisationTypeConcern @@ -206,8 +207,10 @@ 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_ministers_index_page_to_publishing_api, :republish_organisations_index_page_to_publishing_api - after_destroy :republish_ministers_index_page_to_publishing_api, :republish_organisations_index_page_to_publishing_api + 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_destroy :republish_organisations_index_page_to_publishing_api + after_destroy :republish_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 @@ -225,14 +228,6 @@ def foi_contacts end end - def republish_how_government_works_page_to_publishing_api - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") - end - - def republish_ministers_index_page_to_publishing_api - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") if ministerial_department? - end - def republish_organisations_index_page_to_publishing_api PresentPageToPublishingApiWorker.perform_async("PublishingApi::OrganisationsIndexPresenter") end From 9d749c144d5862072a42203cca8c8d3f5843ad65 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Wed, 12 Jun 2024 09:32:46 +0100 Subject: [PATCH 03/10] Consolidate 'reshuffle_in_progress?' method into ReshuffleMode This logic was being duplicated in the two presenters. DRYing up by moving into the new ReshuffleMode concern. This will also make it easier to write our next sets of tests as we'll be able to mock the return value of this method. --- app/models/concerns/reshuffle_mode.rb | 4 ++++ .../how_government_works_presenter.rb | 5 +---- .../ministers_index_presenter.rb | 5 +---- test/unit/app/models/reshuffle_mode_test.rb | 22 +++++++++++++++++++ 4 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 test/unit/app/models/reshuffle_mode_test.rb diff --git a/app/models/concerns/reshuffle_mode.rb b/app/models/concerns/reshuffle_mode.rb index 70ea16dcffd..35c792a0459 100644 --- a/app/models/concerns/reshuffle_mode.rb +++ b/app/models/concerns/reshuffle_mode.rb @@ -14,5 +14,9 @@ def republish_ministers_index_page_to_publishing_api def republish_how_government_works_page_to_publishing_api PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") end + + def reshuffle_in_progress? + SitewideSetting.find_by(key: :minister_reshuffle_mode)&.on || false + end end end diff --git a/app/presenters/publishing_api/how_government_works_presenter.rb b/app/presenters/publishing_api/how_government_works_presenter.rb index 98f63a3105d..5334447b691 100644 --- a/app/presenters/publishing_api/how_government_works_presenter.rb +++ b/app/presenters/publishing_api/how_government_works_presenter.rb @@ -1,5 +1,6 @@ module PublishingApi class HowGovernmentWorksPresenter + include ReshuffleMode attr_accessor :update_type def initialize(update_type: nil) @@ -79,10 +80,6 @@ def ministerial_role_counts private - def reshuffle_in_progress? - SitewideSetting.find_by(key: :minister_reshuffle_mode)&.on || false - end - def prime_minister 1 end diff --git a/app/presenters/publishing_api/ministers_index_presenter.rb b/app/presenters/publishing_api/ministers_index_presenter.rb index f2bb274bf98..9872b8471c9 100644 --- a/app/presenters/publishing_api/ministers_index_presenter.rb +++ b/app/presenters/publishing_api/ministers_index_presenter.rb @@ -1,6 +1,7 @@ module PublishingApi class MinistersIndexPresenter include GovspeakHelper + include ReshuffleMode attr_accessor :update_type @@ -79,10 +80,6 @@ def details end end - def reshuffle_in_progress? - SitewideSetting.find_by(key: :minister_reshuffle_mode)&.on || false - end - def ordered_cabinet_ministers_content_ids ministers = MinisterialRole .cabinet diff --git a/test/unit/app/models/reshuffle_mode_test.rb b/test/unit/app/models/reshuffle_mode_test.rb new file mode 100644 index 00000000000..289f989d908 --- /dev/null +++ b/test/unit/app/models/reshuffle_mode_test.rb @@ -0,0 +1,22 @@ +require "test_helper" + +class ReshuffleModeTest < ActiveSupport::TestCase + extend Minitest::Spec::DSL + + class ClassThatIncludesReshuffleMode < ApplicationRecord + self.table_name = "organisations" + include ReshuffleMode + end + + describe "#reshuffle_in_progress?" do + it "returns false when reshuffle mode is switched off" do + assert_not ClassThatIncludesReshuffleMode.new.reshuffle_in_progress? + end + + it "returns true when reshuffle mode is switched on" do + create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) + + assert ClassThatIncludesReshuffleMode.new.reshuffle_in_progress? + end + end +end From afde4dbe2d770e275a20e0191f159a64710b7c73 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Tue, 11 Jun 2024 16:14:54 +0100 Subject: [PATCH 04/10] Added `update_live` parameter to `PresentPageToPublishingApiWorker` This will default to `true`. In a future commit, we will change the worker such that if the parameter is `false`, it will only update the draft stack. This is to support previews for the Ministers index page. --- app/models/concerns/reshuffle_mode.rb | 15 ++++++++--- .../present_page_to_publishing_api_worker.rb | 6 +++-- test/unit/app/models/reshuffle_mode_test.rb | 25 +++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/reshuffle_mode.rb b/app/models/concerns/reshuffle_mode.rb index 35c792a0459..3f3dd562b2a 100644 --- a/app/models/concerns/reshuffle_mode.rb +++ b/app/models/concerns/reshuffle_mode.rb @@ -3,20 +3,27 @@ module ReshuffleMode included do def republish_ministerial_pages_to_publishing_api - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter") + 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") + PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter", update_live?) end def republish_how_government_works_page_to_publishing_api - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") + PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter", update_live?) end def reshuffle_in_progress? SitewideSetting.find_by(key: :minister_reshuffle_mode)&.on || false end end + + def update_live? + # should be false when reshuffle mode is on - we'll reuse + # https://github.com/alphagov/whitehall/blob/685e3ce1687872ed5567bc1d907b822fbd77035e/app/presenters/publishing_api/ministers_index_presenter.rb#L71-L73 + # in a future commit + true + end end diff --git a/app/workers/present_page_to_publishing_api_worker.rb b/app/workers/present_page_to_publishing_api_worker.rb index c1954bab1b6..281c305f4f8 100644 --- a/app/workers/present_page_to_publishing_api_worker.rb +++ b/app/workers/present_page_to_publishing_api_worker.rb @@ -1,5 +1,7 @@ class PresentPageToPublishingApiWorker < WorkerBase - def perform(presenter) - PresentPageToPublishingApi.new.publish(presenter.constantize) + def perform(presenter, update_live = true) + if update_live + PresentPageToPublishingApi.new.publish(presenter.constantize) + end end end diff --git a/test/unit/app/models/reshuffle_mode_test.rb b/test/unit/app/models/reshuffle_mode_test.rb index 289f989d908..867444bf1c1 100644 --- a/test/unit/app/models/reshuffle_mode_test.rb +++ b/test/unit/app/models/reshuffle_mode_test.rb @@ -19,4 +19,29 @@ class ClassThatIncludesReshuffleMode < ApplicationRecord assert ClassThatIncludesReshuffleMode.new.reshuffle_in_progress? end end + + describe "#republish_ministerial_pages_to_publishing_api" do + it "republishes 'How Government Works' and 'Ministers Index' pages" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksPresenter", true).once + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexPresenter", true).once + + ClassThatIncludesReshuffleMode.new.republish_ministerial_pages_to_publishing_api + end + end + + describe "#republish_ministers_index_page_to_publishing_api" do + it "republishes the 'Ministers Index' page" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexPresenter", true).once + + ClassThatIncludesReshuffleMode.new.republish_ministers_index_page_to_publishing_api + end + end + + describe "#republish_how_government_works_page_to_publishing_api" do + it "republishes the 'How Government Works' page" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksPresenter", true).once + + ClassThatIncludesReshuffleMode.new.republish_how_government_works_page_to_publishing_api + end + end end From 4a843b01c011a526e7b649f877d2b50a8279ff8c Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Tue, 11 Jun 2024 16:44:03 +0100 Subject: [PATCH 05/10] Update draft stack only when reshuffle mode is enabled --- app/models/concerns/reshuffle_mode.rb | 5 +- .../present_page_to_publishing_api_worker.rb | 2 + lib/present_page_to_publishing_api.rb | 6 +++ test/unit/app/models/reshuffle_mode_test.rb | 25 +++++++++ ...sent_page_to_publishing_api_worker_test.rb | 9 ++++ .../present_page_to_publishing_api_test.rb | 53 ++++++++++++------- 6 files changed, 78 insertions(+), 22 deletions(-) diff --git a/app/models/concerns/reshuffle_mode.rb b/app/models/concerns/reshuffle_mode.rb index 3f3dd562b2a..cf0afdbbb5c 100644 --- a/app/models/concerns/reshuffle_mode.rb +++ b/app/models/concerns/reshuffle_mode.rb @@ -21,9 +21,6 @@ def reshuffle_in_progress? end def update_live? - # should be false when reshuffle mode is on - we'll reuse - # https://github.com/alphagov/whitehall/blob/685e3ce1687872ed5567bc1d907b822fbd77035e/app/presenters/publishing_api/ministers_index_presenter.rb#L71-L73 - # in a future commit - true + !reshuffle_in_progress? end end diff --git a/app/workers/present_page_to_publishing_api_worker.rb b/app/workers/present_page_to_publishing_api_worker.rb index 281c305f4f8..b08e14ff4ca 100644 --- a/app/workers/present_page_to_publishing_api_worker.rb +++ b/app/workers/present_page_to_publishing_api_worker.rb @@ -2,6 +2,8 @@ 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 end end diff --git a/lib/present_page_to_publishing_api.rb b/lib/present_page_to_publishing_api.rb index 6448d77e88e..3865406be52 100644 --- a/lib/present_page_to_publishing_api.rb +++ b/lib/present_page_to_publishing_api.rb @@ -5,4 +5,10 @@ def publish(presenter_class) Services.publishing_api.patch_links(payload.content_id, links: payload.links) Services.publishing_api.publish(payload.content_id, nil, locale: payload.content[:locale]) end + + def save_draft(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 diff --git a/test/unit/app/models/reshuffle_mode_test.rb b/test/unit/app/models/reshuffle_mode_test.rb index 867444bf1c1..3bdfa729d8d 100644 --- a/test/unit/app/models/reshuffle_mode_test.rb +++ b/test/unit/app/models/reshuffle_mode_test.rb @@ -27,6 +27,15 @@ class ClassThatIncludesReshuffleMode < ApplicationRecord ClassThatIncludesReshuffleMode.new.republish_ministerial_pages_to_publishing_api end + + it "only saves to the draft stack when reshuffle mode is switched on" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksPresenter", false).once + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexPresenter", false).once + + reshuffle_mode = ClassThatIncludesReshuffleMode.new + reshuffle_mode.stubs(:reshuffle_in_progress?).returns(true) + reshuffle_mode.republish_ministerial_pages_to_publishing_api + end end describe "#republish_ministers_index_page_to_publishing_api" do @@ -35,6 +44,14 @@ class ClassThatIncludesReshuffleMode < ApplicationRecord ClassThatIncludesReshuffleMode.new.republish_ministers_index_page_to_publishing_api end + + it "only saves to the draft stack when reshuffle mode is switched on" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexPresenter", false).once + + reshuffle_mode = ClassThatIncludesReshuffleMode.new + reshuffle_mode.stubs(:reshuffle_in_progress?).returns(true) + reshuffle_mode.republish_ministers_index_page_to_publishing_api + end end describe "#republish_how_government_works_page_to_publishing_api" do @@ -43,5 +60,13 @@ class ClassThatIncludesReshuffleMode < ApplicationRecord ClassThatIncludesReshuffleMode.new.republish_how_government_works_page_to_publishing_api end + + it "only saves to the draft stack when reshuffle mode is switched on" do + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksPresenter", false).once + + reshuffle_mode = ClassThatIncludesReshuffleMode.new + reshuffle_mode.stubs(:reshuffle_in_progress?).returns(true) + reshuffle_mode.republish_how_government_works_page_to_publishing_api + end end end diff --git a/test/unit/app/workers/present_page_to_publishing_api_worker_test.rb b/test/unit/app/workers/present_page_to_publishing_api_worker_test.rb index e4adb2f511b..aa2690e27e0 100644 --- a/test/unit/app/workers/present_page_to_publishing_api_worker_test.rb +++ b/test/unit/app/workers/present_page_to_publishing_api_worker_test.rb @@ -9,4 +9,13 @@ class PresentPageToPublishingApiWorkerTest < ActiveSupport::TestCase PresentPageToPublishingApiWorker.new.perform("PublishingApi::HowGovernmentWorksPresenter") end + + test "calls `save_draft` method when `update_live` parameter is false" do + service = mock + PresentPageToPublishingApi.expects(:new).returns(service) + + service.expects(:save_draft).with(PublishingApi::HowGovernmentWorksPresenter) + + PresentPageToPublishingApiWorker.new.perform("PublishingApi::HowGovernmentWorksPresenter", false) + end end diff --git a/test/unit/lib/present_page_to_publishing_api_test.rb b/test/unit/lib/present_page_to_publishing_api_test.rb index 3fadd46a61e..de2c1385051 100644 --- a/test/unit/lib/present_page_to_publishing_api_test.rb +++ b/test/unit/lib/present_page_to_publishing_api_test.rb @@ -1,30 +1,36 @@ require "test_helper" class PresentPageToPublishingApiTest < ActiveSupport::TestCase - test "send the fields of operation index page to publishing api" do - assert_content_is_presented_to_publishing_api(PublishingApi::OperationalFieldsIndexPresenter) - end + extend Minitest::Spec::DSL - test "sends the how government works page to publishing api" do - assert_content_is_presented_to_publishing_api(PublishingApi::HowGovernmentWorksPresenter) - end + describe "#publish" do + it "send the fields of operation index page to publishing api" do + assert_content_is_presented_to_publishing_api(PublishingApi::OperationalFieldsIndexPresenter) + end - test "sends the past prime ministers index page to publishing api" do - role = create(:prime_minister_role) - person = create(:person, forename: "Some", surname: "Person") - create(:historic_role_appointment, person:, role:, started_at: Date.civil(1950), ended_at: Date.civil(1960)) - create(:historical_account, person:, born: "1900", died: "1975", role:) + it "sends the past prime ministers index page to publishing api" do + role = create(:prime_minister_role) + person = create(:person, forename: "Some", surname: "Person") + create(:historic_role_appointment, person:, role:, started_at: Date.civil(1950), ended_at: Date.civil(1960)) + create(:historical_account, person:, born: "1900", died: "1975", role:) - assert_content_is_presented_to_publishing_api(PublishingApi::HistoricalAccountsIndexPresenter) - end + assert_content_is_presented_to_publishing_api(PublishingApi::HistoricalAccountsIndexPresenter) + end + + it "should update content, patch links and publish new document with locale" do + I18n.with_locale(:cy) do + assert_content_is_presented_to_publishing_api(PublishingApi::HowGovernmentWorksPresenter, locale: "cy") + end + end - test "should update content, patch links and publish new document with locale" do - I18n.with_locale(:cy) do - assert_content_is_presented_to_publishing_api(PublishingApi::HowGovernmentWorksPresenter, locale: "cy") + it "sends the world index page to publishing api" do + assert_content_is_presented_to_publishing_api(PublishingApi::WorldIndexPresenter) end end - test "sends the world index page to publishing api" do - assert_content_is_presented_to_publishing_api(PublishingApi::WorldIndexPresenter) + describe "#save_draft" do + it "saves the operation index page to publishing api draft stack" do + assert_content_is_presented_to_publishing_api_draft_stack(PublishingApi::OperationalFieldsIndexPresenter) + end end def assert_content_is_presented_to_publishing_api(presenter_class, locale: "en") @@ -37,4 +43,15 @@ def assert_content_is_presented_to_publishing_api(presenter_class, locale: "en") PresentPageToPublishingApi.new.publish(presenter_class) end + + def assert_content_is_presented_to_publishing_api_draft_stack(presenter_class, locale: "en") + presenter = presenter_class.new + expected_content = presenter.content + + Services.publishing_api.expects(:put_content).with(presenter.content_id, expected_content).once + Services.publishing_api.expects(:patch_links).with(presenter.content_id, links: presenter.links).once + Services.publishing_api.expects(:publish).with(presenter.content_id, nil, locale:).never + + PresentPageToPublishingApi.new.save_draft(presenter_class) + end end From 49b6515a11cae20199c4d0b03d33fd0f07610099 Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Wed, 12 Jun 2024 11:22:51 +0100 Subject: [PATCH 06/10] Extract presenters to be called only when reshuffling The logic for splitting functionality depending on reshuffling mode used to live in the presenters themselves, checking `reshuffle_in_progress?`. We have now moved that logic within `ReshuffleMode`, and are sending content either to the draft or live stack depending on whether reshuffle mode is on. We need the presenters to send full data (not empty objects) to the required draft/live stack, so we are removing the checks from the presenters. We have created new presenters to deal with sending blank links and body for when we are enabling the reshuffle mode the first time. --- ...rnment_works_enable_reshuffle_presenter.rb | 15 +++++ .../how_government_works_presenter.rb | 18 ++---- ...isters_index_enable_reshuffle_presenter.rb | 24 ++++++++ .../ministers_index_presenter.rb | 46 +++++---------- ...t_works_enable_reshuffle_presenter_test.rb | 58 +++++++++++++++++++ .../how_government_works_presenter_test.rb | 33 +---------- ...s_index_enable_reshuffle_presenter_test.rb | 41 +++++++++++++ .../ministers_index_presenter_test.rb | 38 +----------- 8 files changed, 159 insertions(+), 114 deletions(-) create mode 100644 app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter.rb create mode 100644 app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter.rb create mode 100644 test/unit/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter_test.rb create mode 100644 test/unit/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter_test.rb diff --git a/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter.rb b/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter.rb new file mode 100644 index 00000000000..26afd469f81 --- /dev/null +++ b/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter.rb @@ -0,0 +1,15 @@ +module PublishingApi + class HowGovernmentWorksEnableReshufflePresenter < HowGovernmentWorksPresenter + def links + { + current_prime_minister: [], + } + end + + def details + { + reshuffle_in_progress: true, + } + end + end +end diff --git a/app/presenters/publishing_api/how_government_works_presenter.rb b/app/presenters/publishing_api/how_government_works_presenter.rb index 5334447b691..7694186a141 100644 --- a/app/presenters/publishing_api/how_government_works_presenter.rb +++ b/app/presenters/publishing_api/how_government_works_presenter.rb @@ -40,25 +40,17 @@ def base_path end def links - return {} if reshuffle_in_progress? - { current_prime_minister: [MinisterialRole.find_by(slug: "prime-minister")&.current_person&.content_id], } end def details - if reshuffle_in_progress? - { - reshuffle_in_progress: reshuffle_in_progress?, - } - else - { - department_counts:, - ministerial_role_counts:, - reshuffle_in_progress: reshuffle_in_progress?, - } - end + { + department_counts:, + ministerial_role_counts:, + reshuffle_in_progress: reshuffle_in_progress?, + } end def department_counts diff --git a/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter.rb b/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter.rb new file mode 100644 index 00000000000..82a0b509f60 --- /dev/null +++ b/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter.rb @@ -0,0 +1,24 @@ +module PublishingApi + class MinistersIndexEnableReshufflePresenter < MinistersIndexPresenter + def links + { + ordered_cabinet_ministers: [], + ordered_also_attends_cabinet: [], + ordered_ministerial_departments: [], + ordered_house_of_commons_whips: [], + ordered_junior_lords_of_the_treasury_whips: [], + ordered_assistant_whips: [], + ordered_house_lords_whips: [], + ordered_baronesses_and_lords_in_waiting_whips: [], + } + end + + private + + def details + { + reshuffle: { message: bare_govspeak_to_html(SitewideSetting.find_by(key: :minister_reshuffle_mode).govspeak) }, + } + end + end +end diff --git a/app/presenters/publishing_api/ministers_index_presenter.rb b/app/presenters/publishing_api/ministers_index_presenter.rb index 9872b8471c9..449f17593a5 100644 --- a/app/presenters/publishing_api/ministers_index_presenter.rb +++ b/app/presenters/publishing_api/ministers_index_presenter.rb @@ -1,7 +1,6 @@ module PublishingApi class MinistersIndexPresenter include GovspeakHelper - include ReshuffleMode attr_accessor :update_type @@ -33,29 +32,16 @@ def content end def links - if reshuffle_in_progress? - { - ordered_cabinet_ministers: [], - ordered_also_attends_cabinet: [], - ordered_ministerial_departments: [], - ordered_house_of_commons_whips: [], - ordered_junior_lords_of_the_treasury_whips: [], - ordered_assistant_whips: [], - ordered_house_lords_whips: [], - ordered_baronesses_and_lords_in_waiting_whips: [], - } - else - { - ordered_cabinet_ministers: ordered_cabinet_ministers_content_ids, - ordered_also_attends_cabinet: ordered_also_attends_cabinet_content_ids, - ordered_ministerial_departments: ordered_ministerial_departments_content_ids, - ordered_house_of_commons_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::WhipsHouseOfCommons), - ordered_junior_lords_of_the_treasury_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::JuniorLordsoftheTreasury), - ordered_assistant_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::AssistantWhips), - ordered_house_lords_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::WhipsHouseofLords), - ordered_baronesses_and_lords_in_waiting_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::BaronessAndLordsInWaiting), - } - end + { + ordered_cabinet_ministers: ordered_cabinet_ministers_content_ids, + ordered_also_attends_cabinet: ordered_also_attends_cabinet_content_ids, + ordered_ministerial_departments: ordered_ministerial_departments_content_ids, + ordered_house_of_commons_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::WhipsHouseOfCommons), + ordered_junior_lords_of_the_treasury_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::JuniorLordsoftheTreasury), + ordered_assistant_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::AssistantWhips), + ordered_house_lords_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::WhipsHouseofLords), + ordered_baronesses_and_lords_in_waiting_whips: ordered_whips_content_ids(Whitehall::WhipOrganisation::BaronessAndLordsInWaiting), + } end def title @@ -69,15 +55,9 @@ def base_path private def details - if reshuffle_in_progress? - { - reshuffle: { message: bare_govspeak_to_html(SitewideSetting.find_by(key: :minister_reshuffle_mode).govspeak) }, - } - else - { - body: "Read biographies and responsibilities of Cabinet ministers and all ministers by department, as well as the whips who help co-ordinate parliamentary business.", - } - end + { + body: "Read biographies and responsibilities of Cabinet ministers and all ministers by department, as well as the whips who help co-ordinate parliamentary business.", + } end def ordered_cabinet_ministers_content_ids diff --git a/test/unit/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter_test.rb b/test/unit/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter_test.rb new file mode 100644 index 00000000000..91ef9331923 --- /dev/null +++ b/test/unit/app/presenters/publishing_api/how_government_works_enable_reshuffle_presenter_test.rb @@ -0,0 +1,58 @@ +require "test_helper" + +class PublishingApi::HowGovernmentWorksEnableReshufflePresenterTest < ActiveSupport::TestCase + extend Minitest::Spec::DSL + + setup do + @current_pm = create(:person) + pm_role = create(:prime_minister_role) + create(:role_appointment, person: @current_pm, role: pm_role) + + create(:role_appointment, person: create(:person), role: create(:ministerial_role, cabinet_member: true)) + create(:role_appointment, person: create(:person), role: create(:ministerial_role, cabinet_member: true)) + create(:role_appointment, person: create(:person), role: create(:ministerial_role)) + create(:role_appointment, person: create(:person), role: create(:ministerial_role)) + create(:role_appointment, person: create(:person), role: create(:ministerial_role)) + + create(:ministerial_department) + create(:non_ministerial_department) + create(:executive_agency) + end + + test "presents a valid content item with minimal details and links" do + expected_hash = { + base_path: "/government/how-government-works", + publishing_app: Whitehall::PublishingApp::WHITEHALL, + rendering_app: "government-frontend", + schema_name: "how_government_works", + document_type: "how_government_works", + title: "How government works", + description: "About the UK system of government. Understand who runs government, and how government is run.", + locale: "en", + routes: [ + { + path: "/government/how-government-works", + type: "exact", + }, + ], + update_type: "major", + redirects: [], + public_updated_at: Time.zone.now, + details: { + reshuffle_in_progress: true, + }, + } + + expected_links = { + current_prime_minister: [], + } + + presenter = PublishingApi::HowGovernmentWorksEnableReshufflePresenter.new + + assert_equal expected_hash, presenter.content + assert_valid_against_publisher_schema(presenter.content, "how_government_works") + + assert_equal expected_links, presenter.links + assert_valid_against_links_schema({ links: presenter.links }, "how_government_works") + end +end diff --git a/test/unit/app/presenters/publishing_api/how_government_works_presenter_test.rb b/test/unit/app/presenters/publishing_api/how_government_works_presenter_test.rb index 0bd9b34a976..46b51ffef4b 100644 --- a/test/unit/app/presenters/publishing_api/how_government_works_presenter_test.rb +++ b/test/unit/app/presenters/publishing_api/how_government_works_presenter_test.rb @@ -76,39 +76,10 @@ class PublishingApi::HowGovernmentWorksPresenterTest < ActiveSupport::TestCase create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) end - test "presents a valid content item without any details or links" do - expected_hash = { - base_path: "/government/how-government-works", - publishing_app: Whitehall::PublishingApp::WHITEHALL, - rendering_app: "government-frontend", - schema_name: "how_government_works", - document_type: "how_government_works", - title: "How government works", - description: "About the UK system of government. Understand who runs government, and how government is run.", - locale: "en", - routes: [ - { - path: "/government/how-government-works", - type: "exact", - }, - ], - update_type: "major", - redirects: [], - public_updated_at: Time.zone.now, - details: { - reshuffle_in_progress: true, - }, - } - - expected_links = {} - + test "sets 'reshuffle_in_progress' to true" do presenter = PublishingApi::HowGovernmentWorksPresenter.new - assert_equal expected_hash, presenter.content - assert_valid_against_publisher_schema(presenter.content, "how_government_works") - - assert_equal expected_links, presenter.links - assert_valid_against_links_schema({ links: presenter.links }, "how_government_works") + assert_equal true, presenter.content[:details][:reshuffle_in_progress] end end end diff --git a/test/unit/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter_test.rb b/test/unit/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter_test.rb new file mode 100644 index 00000000000..cbaf2612c46 --- /dev/null +++ b/test/unit/app/presenters/publishing_api/ministers_index_enable_reshuffle_presenter_test.rb @@ -0,0 +1,41 @@ +require "test_helper" + +class PublishingApi::MinistersIndexEnableReshufflePresenterTest < ActionView::TestCase + def presented_item + PublishingApi::MinistersIndexEnableReshufflePresenter.new + end + + test "presents a valid content item without information" do + I18n.with_locale(:en) do + create( + :sitewide_setting, + key: :minister_reshuffle_mode, + on: true, + govspeak: "Check [latest appointments](/government/news/ministerial-appointments-february-2023).", + ) + + expected_details = { + reshuffle: { + message: "

Check latest appointments.

\n", + }, + } + + expected_links = { + ordered_cabinet_ministers: [], + ordered_also_attends_cabinet: [], + ordered_ministerial_departments: [], + ordered_house_of_commons_whips: [], + ordered_junior_lords_of_the_treasury_whips: [], + ordered_assistant_whips: [], + ordered_house_lords_whips: [], + ordered_baronesses_and_lords_in_waiting_whips: [], + } + + assert_equal expected_details, presented_item.content[:details] + assert_valid_against_publisher_schema(presented_item.content, "ministers_index") + + assert_equal expected_links, presented_item.links + assert_valid_against_links_schema({ links: presented_item.links }, "ministers_index") + end + end +end diff --git a/test/unit/app/presenters/publishing_api/ministers_index_presenter_test.rb b/test/unit/app/presenters/publishing_api/ministers_index_presenter_test.rb index 4831f69f1f5..cf701fc909b 100644 --- a/test/unit/app/presenters/publishing_api/ministers_index_presenter_test.rb +++ b/test/unit/app/presenters/publishing_api/ministers_index_presenter_test.rb @@ -5,10 +5,8 @@ def presented_item PublishingApi::MinistersIndexPresenter.new end - test "presents a valid content item containing the correct information when reshuffle mode is off" do + test "presents a valid content item containing the correct information" do I18n.with_locale(:en) do - create(:sitewide_setting, key: :minister_reshuffle_mode, on: false) - ministerial_department_1 = create(:ministerial_department, ministerial_ordering: 2) ministerial_department_2 = create(:ministerial_department, ministerial_ordering: 3) ministerial_department_3 = create(:ministerial_department, ministerial_ordering: 1) @@ -98,38 +96,4 @@ def presented_item assert_valid_against_links_schema({ links: presented_item.links }, "ministers_index") end end - - test "presents a valid content item without information when reshuffle mode is on" do - I18n.with_locale(:en) do - create( - :sitewide_setting, - key: :minister_reshuffle_mode, - on: true, - govspeak: "Check [latest appointments](/government/news/ministerial-appointments-february-2023).", - ) - - expected_details = { - reshuffle: { - message: "

Check latest appointments.

\n", - }, - } - - expected_links = { - ordered_cabinet_ministers: [], - ordered_also_attends_cabinet: [], - ordered_ministerial_departments: [], - ordered_house_of_commons_whips: [], - ordered_junior_lords_of_the_treasury_whips: [], - ordered_assistant_whips: [], - ordered_house_lords_whips: [], - ordered_baronesses_and_lords_in_waiting_whips: [], - } - - assert_equal expected_details, presented_item.content[:details] - assert_valid_against_publisher_schema(presented_item.content, "ministers_index") - - assert_equal expected_links, presented_item.links - assert_valid_against_links_schema({ links: presented_item.links }, "ministers_index") - end - end end From 6bfc81cf5b14255cd1280ff8adb2524d56c24ee9 Mon Sep 17 00:00:00 2001 From: Laura Ghiorghisor Date: Wed, 12 Jun 2024 12:50:05 +0100 Subject: [PATCH 07/10] Get sitewide setting to call the correct presenters --- app/models/sitewide_setting.rb | 14 ++++---- test/unit/app/models/sitewide_setting_test.rb | 34 ++++++------------- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/app/models/sitewide_setting.rb b/app/models/sitewide_setting.rb index ae1cbd29211..4abede57dfd 100644 --- a/app/models/sitewide_setting.rb +++ b/app/models/sitewide_setting.rb @@ -17,11 +17,13 @@ def name def republish_downstream_if_reshuffle return unless key == "minister_reshuffle_mode" - payload = PublishingApi::MinistersIndexPresenter.new - - Services.publishing_api.put_content(payload.content_id, payload.content) - Services.publishing_api.publish(payload.content_id, nil, locale: "en") - - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter") + update_live = true + if on + PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", update_live) + PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexEnableReshufflePresenter", update_live) + else + PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter", update_live) + PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter", update_live) + end end end diff --git a/test/unit/app/models/sitewide_setting_test.rb b/test/unit/app/models/sitewide_setting_test.rb index 043402689f5..ef46682d703 100644 --- a/test/unit/app/models/sitewide_setting_test.rb +++ b/test/unit/app/models/sitewide_setting_test.rb @@ -1,35 +1,21 @@ require "test_helper" class SitewideSettingTest < ActiveSupport::TestCase - test "toggling reshuffle mode republished the ministers index page" do - payload = PublishingApi::MinistersIndexPresenter.new - - create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) - - requests = [ - stub_publishing_api_put_content(payload.content_id, payload.content), - stub_publishing_api_publish(payload.content_id, locale: "en", update_type: nil), - ] - - assert_all_requested(requests) - end + test "enabling reshuffle mode republishes custom ministers index and how government work pages" do + setting = create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) - # moved from duplicate file - test "should send the how government works page to publishing api when reshuffle mode is switched on" do - PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter) + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", true).once + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexEnableReshufflePresenter", true).once - Sidekiq::Testing.inline! do - create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) - end + setting.republish_downstream_if_reshuffle end - test "should send the how government works page to publishing api when reshuffle mode is switched off" do - setting = create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) + test "disabling reshuffle mode republishes ministers index and how government work pages" do + setting = create(:sitewide_setting, key: :minister_reshuffle_mode, on: false) - PresentPageToPublishingApi.any_instance.expects(:publish).with(PublishingApi::HowGovernmentWorksPresenter) + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksPresenter", true).once + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexPresenter", true).once - Sidekiq::Testing.inline! do - setting.update!(on: false) - end + setting.republish_downstream_if_reshuffle end end From 1ac21b008186777a7443a90b4c8e71cdc5efe5f8 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Wed, 12 Jun 2024 11:55:49 +0100 Subject: [PATCH 08/10] Add preview link on the cabinet ministers ordering page We did consider using the details component, which we use for displating shareable preview links on editions etc, but that was a bit misleading as it suggests a link that can be shared with folks who don't have signon access. In this case, we're only offering a shortcut to the draft-origin version of the ministers index page, so it does require signon access, and the 'preview' link here is not particularly shareable. The code for the details component approach, should we choose to use it in future, is available in https://github.com/alphagov/whitehall/commit/36275690793e5b832b1f5300df993ee1db617628 --- .../admin/cabinet_ministers_controller.rb | 2 ++ .../admin/cabinet_ministers/show.html.erb | 22 +++++++++++++++++++ features/cabinet-ministers.feature | 10 +++++++++ .../cabinet_ministers_steps.rb | 20 +++++++++++++---- 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/app/controllers/admin/cabinet_ministers_controller.rb b/app/controllers/admin/cabinet_ministers_controller.rb index 7f92dd83a75..428996d11fb 100644 --- a/app/controllers/admin/cabinet_ministers_controller.rb +++ b/app/controllers/admin/cabinet_ministers_controller.rb @@ -2,6 +2,8 @@ class Admin::CabinetMinistersController < Admin::BaseController include ReshuffleMode before_action :enforce_permissions! + helper_method :reshuffle_in_progress? + def show @cabinet_minister_roles = MinisterialRole.includes(:translations).where(cabinet_member: true).order(:seniority) @also_attends_cabinet_roles = MinisterialRole.includes(:translations).also_attends_cabinet.order(:seniority) diff --git a/app/views/admin/cabinet_ministers/show.html.erb b/app/views/admin/cabinet_ministers/show.html.erb index 4291a3f7a1e..89c136bd6f0 100644 --- a/app/views/admin/cabinet_ministers/show.html.erb +++ b/app/views/admin/cabinet_ministers/show.html.erb @@ -1,6 +1,28 @@ <% content_for :page_title, "Cabinet ministers ordering" %> <% content_for :title, "Cabinet ministers ordering" %> +<% if reshuffle_in_progress? %> + <%= render "govuk_publishing_components/components/heading", { + text: "Preview ministers page in reshuffle mode", + heading_level: 2, + font_size: "m", + margin_bottom: 3, + } %> + +

+ <%= link_to("Preview on website (opens in new tab)", + "#{Plek.external_url_for('draft-origin')}/government/ministers", + class: "govuk-link", + target: "_blank", + data: { + module: "gem-track-click", + "track-category": "button-clicked", + "track-action": "ministers-index-page-button", + "track-label": "Preview on website", + }, rel: "noopener") %>. Requires Signon access. +

+<% end %> + <%= render "govuk_publishing_components/components/tabs", { tabs: [ { diff --git a/features/cabinet-ministers.feature b/features/cabinet-ministers.feature index 37540b9f0ec..a13e95ded1e 100644 --- a/features/cabinet-ministers.feature +++ b/features/cabinet-ministers.feature @@ -6,6 +6,16 @@ Feature: Reordering of Cabinet ministers and Organisations Background: Given I am a GDS editor + Scenario: Previewing changes to the ministers index page during reshuffle + Given reshuffle mode is on + When I visit the Cabinet ministers order page + Then I should see a preview link to the ministers index page + + Scenario: No preview to the ministers index page when not in reshuffle mode + Given reshuffle mode is off + When I visit the Cabinet ministers order page + Then I should not see a preview link to the ministers index page + Scenario: Reordering Cabinet ministers Given there are multiple Cabinet minister roles When I visit the Cabinet ministers order page diff --git a/features/step_definitions/cabinet_ministers_steps.rb b/features/step_definitions/cabinet_ministers_steps.rb index 060e0fcee86..b74fef74b87 100644 --- a/features/step_definitions/cabinet_ministers_steps.rb +++ b/features/step_definitions/cabinet_ministers_steps.rb @@ -1,13 +1,25 @@ -Given(/^there are multiple Cabinet minister roles$/) do - organisation = create(:organisation) - create(:ministerial_role, name: "Role 1", cabinet_member: true, organisations: [organisation], seniority: 0) - create(:ministerial_role, name: "Role 2", cabinet_member: true, organisations: [organisation], seniority: 1) +Given(/^reshuffle mode is (on|off)$/) do |on_or_off| + create(:sitewide_setting, key: :minister_reshuffle_mode, on: on_or_off == "on") end When(/^I visit the Cabinet ministers order page$/) do visit admin_cabinet_ministers_path end +Then(/^I should see a preview link to the ministers index page$/) do + expect(page).to have_selector(".govuk-link[data-track-action=ministers-index-page-button]") +end + +Then(/^I should not see a preview link to the ministers index page$/) do + expect(page).to_not have_selector(".govuk-link[data-track-action=ministers-index-page-button]") +end + +Given(/^there are multiple Cabinet minister roles$/) do + organisation = create(:organisation) + create(:ministerial_role, name: "Role 1", cabinet_member: true, organisations: [organisation], seniority: 0) + create(:ministerial_role, name: "Role 2", cabinet_member: true, organisations: [organisation], seniority: 1) +end + When(/^I click the reorder link in the "([^"]*)" tab$/) do |tab| within tab do click_link "Reorder list" From 45cb84f42bf79acbaef541752422d364c50049bd Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Thu, 13 Jun 2024 11:52:56 +0100 Subject: [PATCH 09/10] Disable "manual republishing" of certain pages in reshuffle mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If someone were to use the new “Republish ministers” feature while in reshuffle mode, they’d inadvertently end up publishing whatever’s on the draft stack, to live. Therefore we are disabling the republish feature just for the "ministers index" and "how government works" pages when in reshuffle mode. --- .../admin/republishing_controller.rb | 6 +++++ .../admin/republishing_controller_test.rb | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/app/controllers/admin/republishing_controller.rb b/app/controllers/admin/republishing_controller.rb index c4f3dcf71d7..5a146cec7d9 100644 --- a/app/controllers/admin/republishing_controller.rb +++ b/app/controllers/admin/republishing_controller.rb @@ -1,5 +1,6 @@ class Admin::RepublishingController < Admin::BaseController include Admin::RepublishingHelper + include ReshuffleMode before_action :enforce_permissions! @@ -21,6 +22,11 @@ def republish_page page_to_republish = republishable_pages.find { |page| page[:slug] == params[:page_slug] } return render "admin/errors/not_found", status: :not_found unless page_to_republish + if reshuffle_in_progress? && %w[how-government-works ministers].include?(params[:page_slug]) + flash[:alert] = "Cannot republish #{params[:page_slug]} page while in reshuffle mode" + return redirect_to(admin_republishing_index_path) + end + action = "The page '#{page_to_republish[:title]}' has been scheduled for republishing" @republishing_event = build_republishing_event(action:, content_id: page_to_republish[:presenter].constantize.new.content_id) diff --git a/test/functional/admin/republishing_controller_test.rb b/test/functional/admin/republishing_controller_test.rb index b6c467b8aa8..fcca87d1164 100644 --- a/test/functional/admin/republishing_controller_test.rb +++ b/test/functional/admin/republishing_controller_test.rb @@ -77,6 +77,32 @@ class Admin::RepublishingControllerTest < ActionController::TestCase assert_template "confirm_page" end + def enable_reshuffle_mode! + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", true).once + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexEnableReshufflePresenter", true).once + create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) + end + + test "GDS Admin users should encounter an error when trying to POST :republish page with the ministers index page when in reshuffle mode" do + enable_reshuffle_mode! + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexPresenter").never + + post :republish_page, params: { page_slug: "ministers", reason: "Foo" } + + assert_equal "Cannot republish ministers page while in reshuffle mode", flash[:alert] + assert_redirected_to admin_republishing_index_path + end + + test "GDS Admin users should encounter an error when trying to POST :republish page with the 'how government works' page when in reshuffle mode" do + enable_reshuffle_mode! + PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksPresenter").never + + post :republish_page, params: { page_slug: "how-government-works", reason: "Foo" } + + assert_equal "Cannot republish how-government-works page while in reshuffle mode", flash[:alert] + assert_redirected_to admin_republishing_index_path + end + test "GDS Admin users should see a 404 page when trying to POST :republish_page with an unregistered page slug" do PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HistoricalAccountsIndexPresenter").never From 96432498fd035d7cf07123f16173468de18619b2 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Thu, 13 Jun 2024 13:31:41 +0100 Subject: [PATCH 10/10] Synchronously update live and draft stacks when enabling reshuffle Prior to this commit, there was no usable preview until after someone had made a reorder (or equivalent) after enabling reshuffle mode. We believe this would cause FUD, so it is worth making the initial calls synchronous and touching the draft stack again after enabling reshuffle mode, to force the preview to work right from the beginning. --- app/models/sitewide_setting.rb | 8 ++++++-- test/functional/admin/republishing_controller_test.rb | 2 -- test/unit/app/models/sitewide_setting_test.rb | 7 +++++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/app/models/sitewide_setting.rb b/app/models/sitewide_setting.rb index 4abede57dfd..5b75d10f912 100644 --- a/app/models/sitewide_setting.rb +++ b/app/models/sitewide_setting.rb @@ -19,8 +19,12 @@ def republish_downstream_if_reshuffle update_live = true if on - PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", update_live) - PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexEnableReshufflePresenter", update_live) + # 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) else PresentPageToPublishingApiWorker.perform_async("PublishingApi::HowGovernmentWorksPresenter", update_live) PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter", update_live) diff --git a/test/functional/admin/republishing_controller_test.rb b/test/functional/admin/republishing_controller_test.rb index fcca87d1164..7655cf39931 100644 --- a/test/functional/admin/republishing_controller_test.rb +++ b/test/functional/admin/republishing_controller_test.rb @@ -78,8 +78,6 @@ class Admin::RepublishingControllerTest < ActionController::TestCase end def enable_reshuffle_mode! - PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", true).once - PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexEnableReshufflePresenter", true).once create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) end diff --git a/test/unit/app/models/sitewide_setting_test.rb b/test/unit/app/models/sitewide_setting_test.rb index ef46682d703..a4dbcb130c3 100644 --- a/test/unit/app/models/sitewide_setting_test.rb +++ b/test/unit/app/models/sitewide_setting_test.rb @@ -4,8 +4,11 @@ class SitewideSettingTest < ActiveSupport::TestCase test "enabling reshuffle mode republishes custom ministers index and how government work pages" do setting = create(:sitewide_setting, key: :minister_reshuffle_mode, on: true) - PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", true).once - PresentPageToPublishingApiWorker.expects(:perform_async).with("PublishingApi::MinistersIndexEnableReshufflePresenter", true).once + mock_worker = mock + PresentPageToPublishingApiWorker.stubs(:new).returns(mock_worker) + mock_worker.expects(:perform).with("PublishingApi::HowGovernmentWorksEnableReshufflePresenter", true).once + mock_worker.expects(:perform).with("PublishingApi::MinistersIndexEnableReshufflePresenter", true).once + mock_worker.expects(:perform).with("PublishingApi::MinistersIndexPresenter", false).once setting.republish_downstream_if_reshuffle end