Remove "important board members" image constraint

This feature was added in #4162 (counterpart in Collections: alphagov/collections#754). Neither PR really explains/justifies the feature: it appears to have been added as a workaround for a specific organisation, where they'd neglected to add an image for one of their board members. Instead of adding an image for them, this feature was added to impose an artificial limit on the number of images to display on the page. This has since caused numerous support tickets for the publishing team, with publishers struggling to understand why board member photos are missing from the org page. The limit, tucked away in the organisation settings, is not well known and is not intuitive. We've decided to remove the feature, removing complexity from both Whitehall and Collections. It should result in fewer support tickets for the team, and be a net positive for organisations in terms of an easier-to-understand system. If we find there is a requirement to re-introduce something like this in future, we can approach it more holistically. For now, the simplest thing is to remove the feature, as we suspect it's not widely used. Trello: https://trello.com/c/mz0WLkIR/3282-remove-display-management-team-images-feature
alphagov · Dec 20, 2024 · fdb307a · fdb307a
1 parent 8dfee82
commit fdb307a
Show file tree

Hide file tree

Showing 12 changed files with 2 additions and 115 deletions.
diff --git a/app/components/admin/organisations/show/summary_list_component.rb b/app/components/admin/organisations/show/summary_list_component.rb
@@ -224,15 +224,6 @@ def featured_links_row
     end
   end
 
-  def management_team_row
-    return if organisation.important_board_members.blank?
-
-    {
-      field: "Management team images on homepage",
-      value: organisation.important_board_members,
-    }
-  end
-
   def foi_exempt_row
     return unless organisation.foi_exempt
 

diff --git a/app/controllers/admin/organisations_controller.rb b/app/controllers/admin/organisations_controller.rb
@@ -107,7 +107,6 @@ def organisation_params
       :public_meetings,
       :public_minutes,
       :regulatory_function,
-      :important_board_members,
       :custom_jobs_url,
       :homepage_type,
       :political,

diff --git a/app/presenters/publishing_api/organisation_presenter.rb b/app/presenters/publishing_api/organisation_presenter.rb
@@ -90,7 +90,6 @@ def details
         ordered_featured_links: featured_links,
         ordered_featured_documents: featured_documents(item, item.featured_documents_display_limit),
         ordered_promotional_features: promotional_features,
-        important_board_members:,
         organisation_featuring_priority:,
         organisation_govuk_status:,
         organisation_type:,
@@ -335,10 +334,6 @@ def people_content_ids(role:)
         .map(&:content_id)
     end
 
-    def important_board_members
-      item.important_board_members
-    end
-
     def organisation_featuring_priority
       item.homepage_type
     end

diff --git a/app/views/admin/organisations/_form.html.erb b/app/views/admin/organisations/_form.html.erb
@@ -319,25 +319,6 @@
     </div>
   <% end %>
 
-  <div class="govuk-!-margin-top-6">
-    <% if can?(:manage_important_board_members, @organisation) && @organisation.management_roles.any? %>
-      <%= render "govuk_publishing_components/components/select", {
-        name: "organisation[important_board_members]",
-        id: "organisation_important_board_members",
-        label: "Display management team images (required)",
-        heading_size: "l",
-        error_message: errors_for_input(@organisation.errors, :important_board_members),
-        options: (1..@organisation.management_roles.count).map do |n|
-          {
-            text: n,
-            value: n,
-            selected: @organisation.important_board_members == n,
-          }
-        end,
-      } %>
-    <% end %>
-  </div>
-
   <div class="app-view-organisation__form__non-departmental-public-body-field js-view-organisation__form__non-departmental-public-body-fields <%= "app-view-organisation__form__non-departmental-public-body-fields--hidden" unless organisation.type&.non_departmental_public_body? %>">
     <%= render "govuk_publishing_components/components/fieldset", {
       legend_text: "Non-Departmental Public Body Information",

diff --git a/...migration/20230518160200_reinstate_independent_agricultural_appeals_panel_organisation.rb b/...migration/20230518160200_reinstate_independent_agricultural_appeals_panel_organisation.rb
@@ -8,7 +8,6 @@ def recreate_organisation_and_dependencies
     organisation_logo_type_id: 2,
     analytics_identifier: "PB210",
     handles_fatalities: false,
-    important_board_members: 1,
     default_news_organisation_image_data_id: nil,
     closed_at: nil,
     organisation_brand_colour_id: 7,

diff --git a/lib/whitehall/authority/rules/organisation_rules.rb b/lib/whitehall/authority/rules/organisation_rules.rb
@@ -8,8 +8,6 @@ def can?(action)
         actor.gds_admin? || actor_is_from_organisation_or_parent?(actor, subject)
       when :manage_featured_links
         actor.gds_admin? || actor.gds_editor? || managing_editor_for_org?(actor, subject)
-      when :manage_important_board_members
-        actor.gds_admin? || actor.gds_editor? || managing_editor_for_org?(actor, subject) || departmental_editor_for_org?(actor, subject)
       else
         false
       end

diff --git a/test/components/admin/organisations/show/summary_list_component_test.rb b/test/components/admin/organisations/show/summary_list_component_test.rb
@@ -38,10 +38,8 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
     assert_selector ".govuk-summary-list__row:nth-child(5) .govuk-summary-list__value", text: organisation.govuk_status.titleize
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Featured link position"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: "News priority"
-    assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__key", text: "Management team images on homepage"
-    assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__value", text: organisation.important_board_members
-    assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__key", text: "Analytics identifier"
-    assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__value", text: organisation.analytics_identifier
+    assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__key", text: "Analytics identifier"
+    assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__value", text: organisation.analytics_identifier
   end
 
   test "renders acronym_row if the organisation has an acronym" do

diff --git a/test/functional/admin/organisations_controller_test.rb b/test/functional/admin/organisations_controller_test.rb
@@ -91,16 +91,6 @@ def example_organisation_attributes
     assert_match %r{logo.png}, Organisation.last.logo.file.filename
   end
 
-  test "POST create can set number of important board members" do
-    post :create,
-         params: {
-           organisation: example_organisation_attributes
-                           .merge(important_board_members: 1),
-         }
-
-    assert_equal 1, Organisation.last.important_board_members
-  end
-
   test "POST on :create with invalid data re-renders the new form" do
     attributes = example_organisation_attributes
 
@@ -138,34 +128,6 @@ def example_organisation_attributes
     assert_equal organisation, assigns(:organisation)
   end
 
-  view_test "GET on :edit allows entry of important board members only data to Editors and above" do
-    organisation = create(:organisation)
-    junior_board_member_role = create(:board_member_role)
-    senior_board_member_role = create(:board_member_role)
-
-    create(:organisation_role, organisation:, role: senior_board_member_role)
-    create(:organisation_role, organisation:, role: junior_board_member_role)
-
-    managing_editor = create(:managing_editor, organisation:)
-    departmental_editor = create(:departmental_editor, organisation:)
-    world_editor = create(:world_editor, organisation:)
-
-    get :edit, params: { id: organisation }
-    assert_select "select#organisation_important_board_members option", count: 2
-
-    login_as(departmental_editor)
-    get :edit, params: { id: organisation }
-    assert_select "select#organisation_important_board_members option", count: 2
-
-    login_as(managing_editor)
-    get :edit, params: { id: organisation }
-    assert_select "select#organisation_important_board_members option", count: 2
-
-    login_as(world_editor)
-    get :edit, params: { id: organisation }
-    assert_select "select#organisation_important_board_members option", count: 0
-  end
-
   view_test "GET :edit renders hidden id field for default news image" do
     organisation = create(:organisation, :with_default_news_image)
 

diff --git a/test/unit/app/presenters/publishing_api/organisation_presenter_test.rb b/test/unit/app/presenters/publishing_api/organisation_presenter_test.rb
@@ -25,7 +25,6 @@ def govspeak_to_html(govspeak)
       analytics_identifier: "O123",
       parent_organisations: [parent_organisation],
       url: "https://www.gov.uk/oot",
-      important_board_members: 5,
       default_news_image: news_image,
     )
     create(
@@ -75,7 +74,6 @@ def govspeak_to_html(govspeak)
         ordered_featured_links: [],
         ordered_featured_documents: [],
         ordered_promotional_features: [],
-        important_board_members: 5,
         organisation_featuring_priority: "news",
         organisation_govuk_status: {
           status: "live",

diff --git a/test/unit/lib/whitehall/authority/department_editor_test.rb b/test/unit/lib/whitehall/authority/department_editor_test.rb
@@ -164,18 +164,6 @@ def department_editor(id = 1)
     assert_not enforcer_for(user, other_org).can?(:manage_featured_links)
   end
 
-  test "can manage important board members for their organisation" do
-    user = department_editor
-
-    editors_org = user.organisation
-    other_org = build(:organisation)
-    child_org = create(:organisation, parent_organisations: [editors_org])
-
-    assert enforcer_for(user, editors_org).can?(:manage_important_board_members)
-    assert_not enforcer_for(user, child_org).can?(:manage_important_board_members)
-    assert_not enforcer_for(user, other_org).can?(:manage_important_board_members)
-  end
-
   test "can export editions" do
     assert enforcer_for(department_editor, Edition).can?(:export)
   end

diff --git a/test/unit/lib/whitehall/authority/gds_editor_test.rb b/test/unit/lib/whitehall/authority/gds_editor_test.rb
@@ -177,16 +177,6 @@ def gds_editor(id = 1)
     assert enforcer_for(user, other_org).can?(:manage_featured_links)
   end
 
-  test "can manage important board members for any organisation" do
-    user = gds_editor
-
-    editors_org = user.organisation
-    other_org = build(:organisation)
-
-    assert enforcer_for(user, editors_org).can?(:manage_important_board_members)
-    assert enforcer_for(user, other_org).can?(:manage_important_board_members)
-  end
-
   test "can mark editions as political" do
     assert enforcer_for(gds_editor, normal_edition).can?(:mark_political)
   end

diff --git a/test/unit/lib/whitehall/authority/managing_editor_test.rb b/test/unit/lib/whitehall/authority/managing_editor_test.rb
@@ -174,18 +174,6 @@ def managing_editor(id = 1)
     assert_not enforcer_for(user, other_org).can?(:manage_featured_links)
   end
 
-  test "can manage important board members for their organisation and child organisation" do
-    user = managing_editor
-
-    editors_org = user.organisation
-    child_org = create(:organisation, parent_organisations: [editors_org])
-    other_org = build(:organisation)
-
-    assert enforcer_for(user, editors_org).can?(:manage_important_board_members)
-    assert enforcer_for(user, child_org).can?(:manage_important_board_members)
-    assert_not enforcer_for(user, other_org).can?(:manage_important_board_members)
-  end
-
   test "can export editions" do
     assert enforcer_for(managing_editor, Edition).can?(:export)
   end