Merge pull request #8540 from alphagov/refactor-cabinet-ministers-con…

…troller

Refactor cabinet ministers controller

app/controllers/admin/cabinet_ministers_controller.rb

@@ -12,64 +12,53 @@ def reorder_cabinet_minister_roles
     @roles = MinisterialRole.includes(:translations).where(cabinet_member: true).order(:seniority)
   end
 
+  def order_cabinet_minister_roles
+    params["ordering"].each do |id, ordering|
+      Role.find(id).update_column(:seniority, ordering)
+    end
+
+    redirect_to admin_cabinet_ministers_path(anchor: "cabinet_minister")
+  end
+
   def reorder_also_attends_cabinet_roles
     @roles = MinisterialRole.includes(:translations).also_attends_cabinet.order(:seniority)
   end
 
+  def order_also_attends_cabinet_roles
+    params["ordering"].each do |id, ordering|
+      Role.find(id).update_column(:seniority, ordering)
+    end
+
+    redirect_to admin_cabinet_ministers_path(anchor: "also_attends_cabinet")
+  end
+
   def reorder_whip_roles
     @roles = MinisterialRole.includes(:translations).whip.order(:whip_ordering)
   end
 
+  def order_whip_roles
+    params["ordering"].each do |id, ordering|
+      Role.find(id).update_column(:whip_ordering, ordering)
+    end
+
+    redirect_to admin_cabinet_ministers_path(anchor: "whips")
+  end
+
   def reorder_ministerial_organisations
     @organisations = Organisation.ministerial_departments.excluding_govuk_status_closed.order(:ministerial_ordering)
   end
 
-  def update
-    update_ordering(:roles, :seniority)
-    update_ordering(:whips, :whip_ordering)
-    update_organisation_ordering
+  def order_ministerial_organisations
+    params["ordering"].each do |id, ordering|
+      Organisation.find(id).update_column(:ministerial_ordering, ordering)
+    end
 
-    redirect_to admin_cabinet_ministers_path + add_anchor_if_arrived_from_reorder_page
+    redirect_to admin_cabinet_ministers_path(anchor: "organisations")
   end
 
 private
 
   def enforce_permissions!
     enforce_permission!(:reorder_cabinet_ministers, MinisterialRole)
   end
-
-  def add_anchor_if_arrived_from_reorder_page
-    return "" if request.referer.blank?
-
-    case URI(request.referer).path
-    when reorder_cabinet_minister_roles_admin_cabinet_ministers_path
-      "#cabinet_minister"
-    when reorder_also_attends_cabinet_roles_admin_cabinet_ministers_path
-      "#also_attends_cabinet"
-    when reorder_whip_roles_admin_cabinet_ministers_path
-      "#whips"
-    when reorder_ministerial_organisations_admin_cabinet_ministers_path
-      "#organisations"
-    else
-      ""
-    end
-  end
-
-  def update_ordering(key, column)
-    return unless params.include?(key)
-
-    params[key]["ordering"].keys.each do |id|
-      Role.where(id:).update_all("#{column}": params[key]["ordering"][id.to_s])
-    end
-  end
-
-  def update_organisation_ordering
-    return unless params.include?(:organisation)
-
-    params[:organisation]["ordering"].each_pair do |id, order|
-      Organisation.where(id:).update_all(
-        ministerial_ordering: order,
-      )
-    end
-  end
 end

app/views/admin/cabinet_ministers/_reorder.html.erb

@@ -2,14 +2,13 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
-    <%= form_with url: admin_cabinet_ministers_path, method: :patch do %>
+    <%= form_with url: form_url, method: :patch do %>
       <%= render "govuk_publishing_components/components/hint", {
         text: "Use the up and down buttons to reorder attachments, or select and hold on an attachment to reorder using drag and drop.",
         margin_bottom: 4,
       } %>
 
       <%= render "govuk_publishing_components/components/reorderable_list", {
-        input_name: name,
         items: list.map do |item|
           {
             id: item.id,

app/views/admin/cabinet_ministers/reorder_also_attends_cabinet_roles.html.erb

@@ -1,4 +1,7 @@
 <% content_for :page_title, "Reorder also attends cabinet ordering" %>
 <% content_for :context, "Also attends cabinet ordering" %>
 
-<%= render "reorder", list: @roles, name: "roles[ordering]", cancel_path: "#{admin_cabinet_ministers_path}#also_attends_cabinet" %>
+<%= render "reorder",
+  list: @roles,
+  form_url: order_also_attends_cabinet_roles_admin_cabinet_ministers_path,
+  cancel_path: admin_cabinet_ministers_path(anchor: "also_attends_cabinet") %>

app/views/admin/cabinet_ministers/reorder_cabinet_minister_roles.html.erb

@@ -1,4 +1,7 @@
 <% content_for :page_title, "Reorder Cabinet minister ordering" %>
 <% content_for :context, "Cabinet minister ordering" %>
 
-<%= render "reorder", list: @roles, name: "roles[ordering]", cancel_path: "#{admin_cabinet_ministers_path}#cabinet_minister" %>
+<%= render "reorder",
+  list: @roles,
+  form_url: order_cabinet_minister_roles_admin_cabinet_ministers_path,
+  cancel_path: admin_cabinet_ministers_path(anchor: "cabinet_minister") %>

app/views/admin/cabinet_ministers/reorder_ministerial_organisations.html.erb

@@ -1,4 +1,7 @@
-<% content_for :page_title, "Reorder Whip ordering" %>
-<% content_for :context, "Whip ordering" %>
+<% content_for :page_title, "Reorder Ministerial organisations ordering" %>
+<% content_for :context, "Ministerial organisations ordering" %>
 
-<%= render "reorder", list: @organisations, name: "organisation[ordering]", cancel_path: "#{admin_cabinet_ministers_path}#organisations" %>
+<%= render "reorder",
+  list: @organisations,
+  form_url: order_ministerial_organisations_admin_cabinet_ministers_path,
+  cancel_path: admin_cabinet_ministers_path(anchor: "organisations") %>

app/views/admin/cabinet_ministers/reorder_whip_roles.html.erb

@@ -1,4 +1,7 @@
 <% content_for :page_title, "Reorder Whips ordering" %>
 <% content_for :context, "Whips ordering" %>
 
-<%= render "reorder", list: @roles, name: "whips[ordering]", cancel_path: "#{admin_cabinet_ministers_path}#whips" %>
+<%= render "reorder",
+  list: @roles,
+  form_url: order_cabinet_minister_roles_admin_cabinet_ministers_path,
+  cancel_path: admin_cabinet_ministers_path(anchor: "whips") %>

config/routes.rb

@@ -303,11 +303,18 @@ def redirect(path, options = { prefix: Whitehall.router_prefix })
         get :confirm_destroy, on: :member
       end
 
-      resource :cabinet_ministers, only: %i[show update] do
-        get :reorder_cabinet_minister_roles, on: :member
-        get :reorder_also_attends_cabinet_roles, on: :member
-        get :reorder_whip_roles, on: :member
-        get :reorder_ministerial_organisations, on: :member
+      resource :cabinet_ministers, only: %i[show] do
+        get :reorder_cabinet_minister_roles
+        patch :order_cabinet_minister_roles
+
+        get :reorder_also_attends_cabinet_roles
+        patch :order_also_attends_cabinet_roles
+
+        get :reorder_whip_roles
+        patch :order_whip_roles
+
+        get :reorder_ministerial_organisations
+        patch :order_ministerial_organisations
       end
 
       resources :roles, except: [:show] do

features/cabinet-ministers.feature

@@ -10,7 +10,7 @@ Feature: Reordering of Cabinet ministers and Organisations
     Given there are multiple Cabinet minister roles
     When I visit the Cabinet ministers order page
     And I click the reorder link in the "#cabinet_minister" tab
-    And I set the order of the roles for the "roles" ordering field to:
+    And I set the order of the roles to:
       | name   | order |
       | Role 2 | 0     |
       | Role 1 | 1     |
@@ -23,7 +23,7 @@ Feature: Reordering of Cabinet ministers and Organisations
     Given there are multiple Also attends cabinet roles
     When I visit the Cabinet ministers order page
     And I click the reorder link in the "#also_attends_cabinet" tab
-    And I set the order of the roles for the "roles" ordering field to:
+    And I set the order of the roles to:
       | name   | order |
       | Role 2 | 0     |
       | Role 1 | 1     |
@@ -36,7 +36,7 @@ Feature: Reordering of Cabinet ministers and Organisations
     Given there are multiple Whip roles
     When I visit the Cabinet ministers order page
     And I click the reorder link in the "#whips" tab
-    And I set the order of the roles for the "whips" ordering field to:
+    And I set the order of the roles to:
       | name   | order |
       | Role 2 | 0     |
       | Role 1 | 1     |
@@ -49,7 +49,7 @@ Feature: Reordering of Cabinet ministers and Organisations
     Given there are multiple organisations with ministerial ordering
     When I visit the Cabinet ministers order page
     And I click the reorder link in the "#organisations" tab
-    And I set the order of the organisations for the "organisation" ordering field to:
+    And I set the order of the organisations to:
       | name   | order |
       | Org 2 | 0     |
       | Org 1 | 1     |

features/step_definitions/cabinet_ministers_steps.rb

@@ -14,15 +14,15 @@
   end
 end
 
-And(/^I set the order of the (roles|organisations) for the "([^"]*)" ordering field to:$/) do |type, name, ordering|
+And(/^I set the order of the (roles|organisations) to:$/) do |type, ordering|
   ordering.hashes.each do |hash|
     model = if type == "roles"
               Role.find_by!(name: hash[:name])
             else
               Organisation.find_by!(name: hash[:name])
             end
 
-    fill_in "#{name}[ordering][#{model.id}]", with: hash[:order]
+    fill_in "ordering[#{model.id}]", with: hash[:order]
   end
 
   click_button "Update order"

test/functional/admin/cabinet_ministers_controller_test.rb

@@ -11,81 +11,69 @@ def organisation
     @organisation ||= create(:organisation)
   end
 
-  test "should reorder ministerial roles" do
-    @request.env["HTTP_REFERER"] = Plek.website_root + reorder_cabinet_minister_roles_admin_cabinet_ministers_path
+  test "PATCH :order_cabinet_minister_roles should reorder ministerial roles" do
     role2 = create(:ministerial_role, name: "Non-Executive Director", cabinet_member: true, organisations: [organisation])
     role1 = create(:ministerial_role, name: "Prime Minister", cabinet_member: true, organisations: [organisation])
 
-    put :update,
-        params: {
-          roles: {
+    patch :order_cabinet_minister_roles,
+          params: {
             ordering: {
               role1.id.to_s => 0,
               role2.id.to_s => 1,
             },
-          },
-        }
+          }
 
-    assert_equal MinisterialRole.cabinet.order(:seniority).to_a, [role1, role2]
-    assert_redirected_to "#{admin_cabinet_ministers_path}#cabinet_minister"
+    assert_equal MinisterialRole.cabinet.order(:seniority), [role1, role2]
+    assert_redirected_to admin_cabinet_ministers_path(anchor: "cabinet_minister")
   end
 
-  test "should reorder people who also attend cabinet" do
-    @request.env["HTTP_REFERER"] = Plek.website_root + reorder_also_attends_cabinet_roles_admin_cabinet_ministers_path
+  test "PATCH :order_also_attends_cabinet_roles should reorder people who also attend cabinet" do
     role2 = create(:ministerial_role, name: "Chief Whip and Parliamentary Secretary to the Treasury", attends_cabinet_type_id: 2, organisations: [organisation])
     role1 = create(:ministerial_role, name: "Minister without Portfolio", attends_cabinet_type_id: 1, organisations: [organisation])
 
-    put :update,
-        params: {
-          roles: {
+    patch :order_also_attends_cabinet_roles,
+          params: {
             ordering: {
               role1.id.to_s => 0,
               role2.id.to_s => 1,
             },
-          },
-        }
+          }
 
-    assert_equal MinisterialRole.also_attends_cabinet.order(:seniority).to_a, [role1, role2]
-    assert_redirected_to "#{admin_cabinet_ministers_path}#also_attends_cabinet"
+    assert_equal MinisterialRole.also_attends_cabinet.order(:seniority), [role1, role2]
+    assert_redirected_to admin_cabinet_ministers_path(anchor: "also_attends_cabinet")
   end
 
-  test "should reorder whips as part of the same request" do
-    @request.env["HTTP_REFERER"] = Plek.website_root + reorder_whip_roles_admin_cabinet_ministers_path
+  test "PATCH :order_whip_roles should reorder whips" do
     role2 = create(:ministerial_role, name: "Whip 1", whip_organisation_id: 2, organisations: [organisation])
     role1 = create(:ministerial_role, name: "Whip 2", whip_organisation_id: 2, organisations: [organisation])
 
-    put :update,
-        params: {
-          whips: {
+    patch :order_whip_roles,
+          params: {
             ordering: {
               role1.id.to_s => 0,
               role2.id.to_s => 1,
             },
-          },
-        }
+          }
 
-    assert_equal MinisterialRole.whip.order(:seniority).to_a, [role2, role1]
-    assert_equal MinisterialRole.whip.order(:whip_ordering).to_a, [role1, role2]
-    assert_redirected_to "#{admin_cabinet_ministers_path}#whips"
+    assert_equal MinisterialRole.whip.order(:seniority), [role2, role1]
+    assert_equal MinisterialRole.whip.order(:whip_ordering), [role1, role2]
+    assert_redirected_to admin_cabinet_ministers_path(anchor: "whips")
   end
 
-  test "should reorder ministerial organisations" do
-    @request.env["HTTP_REFERER"] = Plek.website_root + reorder_ministerial_organisations_admin_cabinet_ministers_path
+  test "PATCH :order_ministerial_organisations should reorder ministerial organisations" do
     org2 = create(:organisation)
     org1 = create(:organisation)
 
-    put :update,
+    put :order_ministerial_organisations,
         params: {
-          organisation: {
-            ordering: {
-              org1.id.to_s => 0,
-              org2.id.to_s => 1,
-            },
+          ordering: {
+            org1.id.to_s => 0,
+            org2.id.to_s => 1,
           },
         }
 
     assert_equal Organisation.order(:ministerial_ordering), [org1, org2]
-    assert_redirected_to "#{admin_cabinet_ministers_path}#organisations"
+    assert_redirected_to admin_cabinet_ministers_path(anchor: "organisations")
   end
 
   view_test "should list cabinet ministers and ministerial organisations in separate tabs, in the correct order, with reorder links" do