Merge pull request #8596 from alphagov/implement-class-reorder-concern

Add concern which handles reordering records within a class

app/controllers/admin/cabinet_ministers_controller.rb

@@ -13,9 +13,8 @@ def reorder_cabinet_minister_roles
   end
 
   def order_cabinet_minister_roles
-    params["ordering"].each do |id, ordering|
-      Role.find(id).update_column(:seniority, ordering)
-    end
+    MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :seniority)
+    PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter")
 
     redirect_to admin_cabinet_ministers_path(anchor: "cabinet_minister")
   end
@@ -25,9 +24,8 @@ def reorder_also_attends_cabinet_roles
   end
 
   def order_also_attends_cabinet_roles
-    params["ordering"].each do |id, ordering|
-      Role.find(id).update_column(:seniority, ordering)
-    end
+    MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :seniority)
+    PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter")
 
     redirect_to admin_cabinet_ministers_path(anchor: "also_attends_cabinet")
   end
@@ -37,9 +35,8 @@ def reorder_whip_roles
   end
 
   def order_whip_roles
-    params["ordering"].each do |id, ordering|
-      Role.find(id).update_column(:whip_ordering, ordering)
-    end
+    MinisterialRole.reorder_without_callbacks!(order_ministerial_roles_params, :whip_ordering)
+    PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter")
 
     redirect_to admin_cabinet_ministers_path(anchor: "whips")
   end
@@ -49,9 +46,8 @@ def reorder_ministerial_organisations
   end
 
   def order_ministerial_organisations
-    params["ordering"].each do |id, ordering|
-      Organisation.find(id).update_column(:ministerial_ordering, ordering)
-    end
+    Organisation.reorder_without_callbacks!(order_ministerial_organisations_params, :ministerial_ordering)
+    PresentPageToPublishingApiWorker.perform_async("PublishingApi::MinistersIndexPresenter")
 
     redirect_to admin_cabinet_ministers_path(anchor: "organisations")
   end
@@ -61,4 +57,12 @@ def order_ministerial_organisations
   def enforce_permissions!
     enforce_permission!(:reorder_cabinet_ministers, MinisterialRole)
   end
+
+  def order_ministerial_roles_params
+    params.require(:ministerial_roles)["ordering"]
+  end
+
+  def order_ministerial_organisations_params
+    params.require(:ministerial_organisations)["ordering"]
+  end
 end

app/controllers/admin/take_part_pages_controller.rb

@@ -18,6 +18,7 @@ def new
   def create
     @take_part_page = TakePartPage.new(take_part_page_params)
     if @take_part_page.save
+      TakePartPage.patch_getinvolved_page_links
       redirect_to [:admin, TakePartPage], notice: %(Take part page "#{@take_part_page.title}" created!)
     else
       @take_part_page.build_image if @take_part_page.image.blank?
@@ -33,6 +34,7 @@ def edit
   def update
     @take_part_page = TakePartPage.friendly.find(params[:id])
     if @take_part_page.update(take_part_page_params)
+      TakePartPage.patch_getinvolved_page_links
       redirect_to [:admin, TakePartPage], notice: %(Take part page "#{@take_part_page.title}" updated!)
     else
       render :edit
@@ -46,6 +48,7 @@ def confirm_destroy
   def destroy
     @take_part_page = TakePartPage.friendly.find(params[:id])
     @take_part_page.destroy!
+    TakePartPage.patch_getinvolved_page_links
     redirect_to [:admin, TakePartPage], notice: %(Take part page "#{@take_part_page.title}" deleted!)
   end
 
@@ -54,8 +57,8 @@ def update_order
   end
 
   def reorder
-    new_ordering = (params.permit!.to_h[:ordering] || []).sort_by { |_id, ordering| ordering.to_i }.map(&:first)
-    TakePartPage.reorder!(new_ordering)
+    TakePartPage.reorder!(order_params, :ordering)
+    TakePartPage.patch_getinvolved_page_links
     redirect_to admin_take_part_pages_path, notice: "Take part pages reordered!"
   end
 
@@ -71,6 +74,10 @@ def take_part_page_params
     )
   end
 
+  def order_params
+    params.require(:take_part_pages)["ordering"]
+  end
+
   def clean_take_part_page_params
     if take_part_page_params.dig(:image_attributes, :file_cache).present? && take_part_page_params.dig(:image_attributes, :file).present?
       take_part_page_params[:image_attributes].delete(:file_cache)

app/models/ministerial_role.rb

@@ -1,4 +1,6 @@
 class MinisterialRole < Role
+  include UserOrderable
+
   has_many :editions, -> { distinct }, through: :role_appointments
   has_many :consultations, -> { where("editions.type" => "Consultation").distinct }, through: :role_appointments
   has_many :news_articles, -> { where("editions.type" => "NewsArticle").distinct }, through: :role_appointments

app/models/organisation.rb

@@ -4,6 +4,7 @@ class Organisation < ApplicationRecord
   include Searchable
   include Organisation::OrganisationSearchIndexConcern
   include Organisation::OrganisationTypeConcern
+  include UserOrderable
 
   date_attributes(:closed_at)
 

app/models/take_part_page.rb

@@ -1,18 +1,20 @@
 class TakePartPage < ApplicationRecord
+  include PublishesToPublishingApi
+  include UserOrderable
+
+  GET_INVOLVED_CONTENT_ID = "dbe329f1-359c-43f7-8944-580d4742aa91".freeze
+
   validates_with SafeHtmlValidator
   validates :title, :summary, presence: true, length: { maximum: 255 }
   validates :body, presence: true, length: { maximum: (16.megabytes - 1) }
   validates_with NoFootnotesInGovspeakValidator, attribute: :body
 
   before_save :ensure_ordering!
-  after_commit :patch_getinvolved_page_links
   scope :in_order, -> { order(:ordering) }
 
   extend FriendlyId
   friendly_id :title
 
-  include PublishesToPublishingApi
-
   has_one :image, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable
   accepts_nested_attributes_for :image, reject_if: :all_blank
 
@@ -35,18 +37,6 @@ def self.next_ordering
     (TakePartPage.maximum(:ordering) || 0) + 1
   end
 
-  def self.reorder!(ids_in_new_ordering)
-    return if ids_in_new_ordering.empty?
-
-    ids_in_new_ordering = ids_in_new_ordering.map(&:to_s)
-    TakePartPage.transaction do
-      TakePartPage.where(id: ids_in_new_ordering).find_each do |page|
-        page.update(ordering: ids_in_new_ordering.index(page.id.to_s) + 1)
-      end
-      TakePartPage.where("id NOT IN (?)", ids_in_new_ordering).update_all(ordering: ids_in_new_ordering.size + 1)
-    end
-  end
-
   def base_path
     "/government/get-involved/take-part/#{slug}"
   end
@@ -63,24 +53,23 @@ def publishing_api_presenter
     PublishingApi::TakePartPresenter
   end
 
-protected
-
-  def ensure_ordering!
-    self.ordering = TakePartPage.next_ordering if ordering.nil?
-  end
-
-  def patch_getinvolved_page_links
-    get_involved_content_id = "dbe329f1-359c-43f7-8944-580d4742aa91"
+  def self.patch_getinvolved_page_links
     pages = TakePartPage.in_order.map(&:content_id)
 
     Services.publishing_api.patch_links(
-      get_involved_content_id,
+      GET_INVOLVED_CONTENT_ID,
       links: {
         take_part_pages: pages,
       },
     )
   end
 
+protected
+
+  def ensure_ordering!
+    self.ordering = TakePartPage.next_ordering if ordering.nil?
+  end
+
   def image_is_present
     errors.add(:"image.file", "can't be blank") if image.blank?
   end

app/models/user_orderable.rb

@@ -0,0 +1,21 @@
+module UserOrderable
+  extend ActiveSupport::Concern
+
+  included do
+    def self.reorder_without_callbacks!(new_order, column)
+      transaction do
+        new_order.each do |id, ordering|
+          find(id).update_column(column, ordering)
+        end
+      end
+    end
+
+    def self.reorder!(new_order, column)
+      transaction do
+        new_order.each do |id, ordering|
+          find(id).update!("#{column}": ordering)
+        end
+      end
+    end
+  end
+end

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

@@ -9,6 +9,7 @@
       } %>
 
       <%= 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

@@ -4,4 +4,5 @@
 <%= 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") %>
+  cancel_path: admin_cabinet_ministers_path(anchor: "also_attends_cabinet"),
+  name: "ministerial_roles[ordering]" %>

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

@@ -4,4 +4,5 @@
 <%= render "reorder",
   list: @roles,
   form_url: order_cabinet_minister_roles_admin_cabinet_ministers_path,
-  cancel_path: admin_cabinet_ministers_path(anchor: "cabinet_minister") %>
+  cancel_path: admin_cabinet_ministers_path(anchor: "cabinet_minister"),
+  name: "ministerial_roles[ordering]" %>

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

@@ -4,4 +4,5 @@
 <%= render "reorder",
   list: @organisations,
   form_url: order_ministerial_organisations_admin_cabinet_ministers_path,
-  cancel_path: admin_cabinet_ministers_path(anchor: "organisations") %>
+  cancel_path: admin_cabinet_ministers_path(anchor: "organisations"),
+  name: "ministerial_organisations[ordering]" %>

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

@@ -4,4 +4,5 @@
 <%= render "reorder",
   list: @roles,
   form_url: order_cabinet_minister_roles_admin_cabinet_ministers_path,
-  cancel_path: admin_cabinet_ministers_path(anchor: "whips") %>
+  cancel_path: admin_cabinet_ministers_path(anchor: "whips"),
+  name: "ministerial_roles[ordering]" %>

app/views/admin/take_part_pages/update_order.html.erb

@@ -12,6 +12,7 @@
       } %>
 
       <%= render "govuk_publishing_components/components/reorderable_list", {
+        input_name: "take_part_pages[ordering]",
         items: @take_part_pages.map do |take_part_page|
           {
             id: take_part_page.id,

features/step_definitions/cabinet_ministers_steps.rb

@@ -16,13 +16,13 @@
 
 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 "ordering[#{model.id}]", with: hash[:order]
+    if type == "roles"
+      role = Role.find_by!(name: hash[:name])
+      fill_in "ministerial_roles[ordering][#{role.id}]", with: hash[:order]
+    else
+      organisation = Organisation.find_by!(name: hash[:name])
+      fill_in "ministerial_organisations[ordering][#{organisation.id}]", with: hash[:order]
+    end
   end
 
   click_button "Update order"