-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reorder social media worldwide org #9796
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
class Admin::EditionableSocialMediaAccountsController < Admin::BaseController | ||
include OrderableItemsConcern | ||
before_action :find_edition | ||
before_action :find_social_media_account, only: %i[confirm_destroy destroy edit update] | ||
|
||
|
@@ -46,6 +47,15 @@ def update | |
end | ||
end | ||
|
||
def reorder | ||
if request.put? | ||
update_ordering | ||
redirect_to admin_edition_social_media_accounts_path(@edition), notice: "Social media accounts reordered" | ||
else | ||
@reorderable_social_media_accounts = @edition.social_media_accounts.order(:ordering) | ||
end | ||
end | ||
|
||
private | ||
|
||
def find_edition | ||
|
@@ -62,8 +72,20 @@ def social_media_account_params | |
:social_media_service_id, | ||
:title, | ||
:url, | ||
:ordering, | ||
).merge( | ||
socialable: @edition, | ||
) | ||
end | ||
|
||
def fetch_ordered_items | ||
@ordered_items = extract_items_from_ordering_params(SocialMediaAccount) | ||
end | ||
|
||
def update_ordering | ||
ordered_accounts = fetch_ordered_items | ||
ordered_accounts.each_with_index do |account, index| | ||
account.update_order(index + 1) | ||
end | ||
Comment on lines
+87
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this done transactionally on other ordering endpoints? Feels like the sort of operation that should either succeed completely or fail. If there isn't a database transaction we might end up with ordering conflicts if it fails for one of the accounts for some reason (e.g. validation error) |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
class Admin::WorldwideOfficesController < Admin::BaseController | ||
include OrderableItemsConcern | ||
before_action :find_worldwide_organisation | ||
before_action :find_worldwide_office, only: %i[edit update confirm_destroy destroy] | ||
extend Admin::HomePageListController | ||
|
@@ -94,13 +95,8 @@ def handle_show_on_home_page_param | |
end | ||
end | ||
|
||
def extract_items_from_ordering_params | ||
item_ordering = params[:ordering] || {} | ||
item_ordering.permit!.to_h | ||
.map { |item_id, ordering| [WorldwideOffice.find_by(id: item_id), ordering.to_i] } | ||
.sort_by { |_, ordering| ordering } | ||
.map { |item, _| item } | ||
.compact | ||
def fetch_ordered_items | ||
@ordered_items = extract_items_from_ordering_params(WorldwideOffice) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be an instance variable? |
||
end | ||
|
||
def extract_show_on_home_page_param | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
module OrderableItemsConcern | ||
extend ActiveSupport::Concern | ||
|
||
def extract_items_from_ordering_params(model_class) | ||
item_ordering = params[:ordering] || {} | ||
item_ordering.permit!.to_h | ||
.map { |item_id, ordering| [model_class.find_by(id: item_id), ordering.to_i] } | ||
.sort_by { |_, ordering| ordering } | ||
.map { |item, _| item } | ||
.compact | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,4 +24,9 @@ def service_name | |
def display_name | ||
title.presence || service_name | ||
end | ||
|
||
def update_order(new_order) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably just use the update method from whatever object calls this - or, if you find there should be a transaction, you might want to figure out how to do a bulk update to the ordering column. |
||
self.ordering = new_order | ||
save! | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
<% content_for :page_title, "Reorder social media accounts" %> | ||
<% content_for :title, "Reorder social media accounts" %> | ||
<% content_for :title_margin_bottom, 6 %> | ||
|
||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-full"> | ||
<%= form_with url: reorder_admin_edition_social_media_accounts_path(@edition), method: :put do %> | ||
<%= render "govuk_publishing_components/components/reorderable_list", { | ||
items: @reorderable_social_media_accounts.map do |account| | ||
{ | ||
id: account.id, | ||
title: account.title, | ||
} | ||
end, | ||
} %> | ||
|
||
<div class="govuk-button-group govuk-!-margin-bottom-6"> | ||
<%= render "govuk_publishing_components/components/button", { | ||
text: "Update order", | ||
} %> | ||
|
||
<%= link_to("Cancel", admin_edition_social_media_accounts_path(@edition), class: "govuk-link govuk-link--no-visited-state") %> | ||
</div> | ||
<% end %> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
class AddOrderingToSocialMediaAccounts < ActiveRecord::Migration[7.1] | ||
def change | ||
add_column :social_media_accounts, :ordering, :integer | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not sure about serving two routes from the same controller method, would be interested to get the rest of the team's thoughts on that. I don't think it's a major issue but I don't think I've seen this done much in other places in Whitehall