-
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?
Conversation
99a8309
to
5385699
Compare
6cb2cb2
to
1a301ef
Compare
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.
A few thoughts from me - on the face of it I think there should be a database transaction involved for the reordering, but I have a vague recollection of discussing this with @davidgisbey and that wasn't viable for some reason 🤔
.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 comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an instance variable?
@@ -46,6 +47,15 @@ def update | |||
end | |||
end | |||
|
|||
def reorder | |||
if request.put? |
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
ordered_accounts.each_with_index do |account, index| | ||
account.update_order(index + 1) | ||
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.
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)
@@ -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 comment
The 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.
In fact I've just noticed we have both https://github.com/alphagov/whitehall/blob/main/app/models/concerns/user_orderable.rb and https://github.com/alphagov/whitehall/blob/main/app/models/user_orderable_extension.rb. We should definitely be using one of them - I'll leave it to you to figure out which and why they both exist because I can't remember! |
We've had requests to introduce an ordering feature for social media accounts on worldwide organisations.
FCDO content team recently conducted an audit of all WWO pages and a significant number had social media accounts in a counter-intuitive order (eg vice consuls above Ambassadors), Publishing overhead of maintaining correct order through manual re-order means regional publishers not consistently maintaining to best-practice standard.
Zendesk
Trello
GOVERNMENT FRONTEND - BEFORE
WHITEHALL - REORDER VIEW
GOVERNMENT FRONTEND - AFTER