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

app/components/admin/organisations/show/summary_list_component.rb

@@ -35,7 +35,6 @@ def rows
       topical_events_row,
       featured_links_position_row,
       featured_links_row,
-      management_team_row,
       foi_exempt_row,
       analytics_identifier_row,
     ]
@@ -224,15 +223,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
 

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,

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

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",

db/data_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,

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

test/components/admin/organisations/show/summary_list_component_test.rb

@@ -25,7 +25,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 8
+    assert_selector ".govuk-summary-list__row", count: 7
     assert_selector ".govuk-summary-list__row:nth-child(1) .govuk-summary-list__key", text: "Name"
     assert_selector ".govuk-summary-list__row:nth-child(1) .govuk-summary-list__value", text: organisation.name
     assert_selector ".govuk-summary-list__row:nth-child(2) .govuk-summary-list__key", text: "Logo formatted name"
@@ -38,18 +38,16 @@ 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
     organisation = build_stubbed(:ministerial_department, acronym: "ACRO")
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(2) .govuk-summary-list__key", text: "Acronym"
     assert_selector ".govuk-summary-list__row:nth-child(2) .govuk-summary-list__value", text: organisation.acronym
   end
@@ -59,7 +57,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__key", text: "Brand colour"
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__value", text: organisation.organisation_brand_colour.title
   end
@@ -70,7 +68,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__key", text: "Default news image"
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__value img[src='#{news_image.file.url(:s300)}']"
   end
@@ -80,7 +78,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__key", text: "Organisation’s URL"
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__value", text: "http://parrot.org"
     assert_selector ".govuk-summary-list__row:nth-child(4) .govuk-summary-list__actions a[href='#{organisation.url}']", text: /View/
@@ -91,7 +89,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(5) .govuk-summary-list__key", text: "Accessible formats request email"
     assert_selector ".govuk-summary-list__row:nth-child(5) .govuk-summary-list__value", text: organisation.alternative_format_contact_email
   end
@@ -103,7 +101,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 11
+    assert_selector ".govuk-summary-list__row", count: 10
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Reason for closure"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: organisation.govuk_closed_status
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__key", text: "Organisation closed on"
@@ -121,7 +119,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 12
+    assert_selector ".govuk-summary-list__row", count: 11
     assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__key", text: "Superseding organisation 1"
     assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__value", text: superseding_organisation1.name
     assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__actions a[href='#{superseding_organisation1.public_url}']", text: /View/
@@ -135,7 +133,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Organisation chart URL"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: organisation.organisation_chart_url
   end
@@ -145,7 +143,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Recruitment URL"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: organisation.custom_jobs_url
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__actions a[href='#{organisation.custom_jobs_url}']", text: /View/
@@ -156,7 +154,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Publishes content associated with the current government"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: "Yes"
   end
@@ -168,7 +166,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Sponsoring organisation"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: parent_organisation.name
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__actions a[href='#{parent_organisation.public_url}']", text: /View/
@@ -182,7 +180,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 10
+    assert_selector ".govuk-summary-list__row", count: 9
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Sponsoring organisation 1"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: parent_organisation1.name
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__actions a[href='#{parent_organisation1.public_url}']", text: /View/
@@ -198,7 +196,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Topical event"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: topical_event.name
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__actions a[href='#{topical_event.public_url}']", text: /View/
@@ -212,7 +210,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 10
+    assert_selector ".govuk-summary-list__row", count: 9
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__key", text: "Topical event 1"
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__value", text: topical_event1.name
     assert_selector ".govuk-summary-list__row:nth-child(6) .govuk-summary-list__actions a[href='#{topical_event1.public_url}']", text: /View/
@@ -228,7 +226,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
+    assert_selector ".govuk-summary-list__row", count: 8
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__key", text: "Featured link"
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__value", text: featured_link.title
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__actions a[href='#{featured_link.url}']", text: /View/
@@ -242,7 +240,7 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 10
+    assert_selector ".govuk-summary-list__row", count: 9
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__key", text: "Featured link 1"
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__value", text: featured_link1.title
     assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__actions a[href='#{featured_link1.url}']", text: /View/
@@ -256,8 +254,8 @@ class Admin::Organisations::Show::SummaryListComponentTest < ViewComponent::Test
 
     render_inline(Admin::Organisations::Show::SummaryListComponent.new(organisation:))
 
-    assert_selector ".govuk-summary-list__row", count: 9
-    assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__key", text: "Exempt from Freedom of Information requests"
-    assert_selector ".govuk-summary-list__row:nth-child(8) .govuk-summary-list__value", text: "Yes"
+    assert_selector ".govuk-summary-list__row", count: 8
+    assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__key", text: "Exempt from Freedom of Information requests"
+    assert_selector ".govuk-summary-list__row:nth-child(7) .govuk-summary-list__value", text: "Yes"
   end
 end

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)
 

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",

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

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