From a6a6e05f9c241817a3faf10ac17758a5ca24f2dc Mon Sep 17 00:00:00 2001 From: Alex Newton Date: Fri, 15 May 2026 14:22:17 +0100 Subject: [PATCH 1/2] Block supporting org editors from locking themselves out --- app/controllers/admin/editions_controller.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/editions_controller.rb b/app/controllers/admin/editions_controller.rb index df07efe1c66..da050d28323 100644 --- a/app/controllers/admin/editions_controller.rb +++ b/app/controllers/admin/editions_controller.rb @@ -106,9 +106,17 @@ def edit end def update - @edition.assign_attributes(edition_params) + saved = ApplicationRecord.transaction do + @edition.assign_attributes(edition_params) - if updater.can_perform? && @edition.save_as(current_user) + if updater.can_perform? && @edition.save_as(current_user) + true + else + raise ActiveRecord::Rollback + end + end + + if saved updater.perform! if @edition.link_check_report @@ -409,6 +417,8 @@ def delete_absent_edition_organisations end if edition_params[:supporting_organisation_ids] edition_params[:supporting_organisation_ids] = edition_params[:supporting_organisation_ids].reject(&:blank?) + elsif edition_params.key?(:lead_organisation_ids) + edition_params[:supporting_organisation_ids] = [] end end From 2dee8cd73b7cd9b226b5abc446b553d8c403d7dd Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Tue, 19 May 2026 17:45:32 +0100 Subject: [PATCH 2/2] WIP - Write a unit test that exercises the behaviour we're seeing in the bug - Figure out if there is an impact to always sending lead/supporting org arrays even on editions that don't support them - Consider if an integration test is needed for this scenario (perhaps covered by the wider A&P test audit). NB: I tried adding this to editions_controller_test, but there is no `update` route on Edition - only on concrete STI descendents of Edition. I tried adding the test to generic_editions_controller_test but the 'generic' edition doesn't support organisation associations. So all things considered, I've put this test in the Standard Editions Controller tests, even though the impact of the change is more far-reaching than StandardEdition alone. --- app/controllers/admin/editions_controller.rb | 40 +++++++++++-------- .../standard_editions_controller_test.rb | 39 ++++++++++++++++++ 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/app/controllers/admin/editions_controller.rb b/app/controllers/admin/editions_controller.rb index da050d28323..4bdda6bfdfc 100644 --- a/app/controllers/admin/editions_controller.rb +++ b/app/controllers/admin/editions_controller.rb @@ -106,17 +106,9 @@ def edit end def update - saved = ApplicationRecord.transaction do - @edition.assign_attributes(edition_params) + @edition.assign_attributes(edition_params) - if updater.can_perform? && @edition.save_as(current_user) - true - else - raise ActiveRecord::Rollback - end - end - - if saved + if updater.can_perform? && @edition.save_as(current_user) updater.perform! if @edition.link_check_report @@ -409,16 +401,32 @@ def set_default_edition_locations end end + # TODO: not sure this is the way forward. We can't reset orgs to [] because some update calls will + # naturally omit orgs, and we get a whole bunch of failing tests. + # I'm also not convinced we should set supporting orgs to [] if lead_orgs association is present. + # Feels a bit unclean - if we wanted to configure 'supporting orgs only' then that should perhaps + # be possible. + # Instead it feels like we should have some mechanism for saying: + # - Here's the current edition + # - Here's what the edition will be if the save completes + # - Has the current user lost access in this transition? If so, raise validation error def delete_absent_edition_organisations return if edition_params.empty? - if edition_params[:lead_organisation_ids] - edition_params[:lead_organisation_ids] = edition_params[:lead_organisation_ids].reject(&:blank?) + if @edition.respond_to?(:lead_organisation_ids) + edition_params[:lead_organisation_ids] = if edition_params[:lead_organisation_ids].blank? + [] + else + edition_params[:lead_organisation_ids].reject(&:blank?) + end end - if edition_params[:supporting_organisation_ids] - edition_params[:supporting_organisation_ids] = edition_params[:supporting_organisation_ids].reject(&:blank?) - elsif edition_params.key?(:lead_organisation_ids) - edition_params[:supporting_organisation_ids] = [] + + if @edition.respond_to?(:supporting_organisation_ids) + edition_params[:supporting_organisation_ids] = if edition_params[:supporting_organisation_ids].blank? + [] + else + edition_params[:supporting_organisation_ids].reject(&:blank?) + end end end diff --git a/test/functional/admin/standard_editions_controller_test.rb b/test/functional/admin/standard_editions_controller_test.rb index 02547e0a472..f4f6648c80c 100644 --- a/test/functional/admin/standard_editions_controller_test.rb +++ b/test/functional/admin/standard_editions_controller_test.rb @@ -1181,6 +1181,45 @@ class Admin::StandardEditionsControllerTest < ActionController::TestCase assert_select "textarea[name='edition[block_content][sidebar]']", text: "My sidebar content" end + test "treats absence of supporting_organisation_ids as an empty array" do + login_as create(:user, organisation: nil) + + configurable_document_type = build_configurable_document_type( + "test_type", + { + "forms" => { + "documents" => { + "fields" => { + "supporting_organisations" => { + "title" => "Supporting organisations", + "block" => "select_with_search_tagging", + "container" => "organisations", + "attribute_path" => %w[supporting_organisation_ids], + "translatable" => false, + }, + }, + }, + }, + }, + ) + ConfigurableDocumentType.setup_test_types(configurable_document_type) + + edition = create(:standard_edition, configurable_document_type: "test_type", supporting_organisation_ids: [create(:organisation).id]) + assert_not edition.supporting_organisation_ids.empty? + + patch :update, params: { + id: edition, + edition: { + # minimal permitted attrs to avoid strong params rejection + title: edition.title, + summary: edition.summary, + }, + save: "save", + } + assert_response :redirect + assert edition.reload.supporting_organisation_ids.empty? + end + view_test "GET edit renders hidden current_tab field for the default tab when document type defines tabs" do configurable_document_type = build_configurable_document_type("test_type", { "forms" => {