-
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
Make unpublishing operation asynchronous #9870
base: main
Are you sure you want to change the base?
Conversation
d974741
to
8cc90ea
Compare
We'll be calling this in the next commit.
Whitehall and Publishing API don't always agree on what a valid redirect URL looks like. Consequently, unpublishing might appear to succeed in Whitehall but be rejected downstream by Publishing API. This caused a support ticket recently: https://govuk.zendesk.com/agent/tickets/6016851 Switching the unpublish technique to synchronous means that any 4xx/5xx errors from Publishing API will be caught on submission, meaning Whitehall will catch the error and display it to the user, and roll back any Whitehall state changes. See https://github.com/alphagov/whitehall/blob/7c5f793c56fc47e4bd6aa07839885d4387fc9b2b/app/services/edition_service.rb#L13-L17 Trello: https://trello.com/c/xlvNUI3c/3382-make-whitehalls-unpublishing-call-to-publishing-api-synchronous
8cc90ea
to
4ae8543
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.
Test name needs correcting I think and I'd like to know a bit about the performance implications
@@ -188,6 +189,20 @@ def assert_sets_redirect_url_in_asset_manager_to(redirect_url) | |||
end | |||
|
|||
def assert_redirected_in_publishing_api(content_id, redirect_path) |
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.
This method is wrongly named I think, should be assert_gone_in_publishing_api
?
when "withdraw" | ||
edition.translations.each do |translation| | ||
api.publish_withdrawal_async( | ||
api.publish_withdrawal_sync( |
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 wonder how many editions there are with lots of translations... could get sloooow. Might be worth checking integration to see if this is acceptable (i.e. if only a few editions have say > 2 translations we can probably live with it).
The ‘unpublish’ action in Whitehall currently creates a worker to call Publishing API asynchronously. It should be updated to call Publishing API synchronously.
We should then listen for the response from Publishing API, and should catch any 4xx error and avoid updating the state of the document in Whitehall, and display an error message to the user.
Trello: https://trello.com/c/xlvNUI3c/3382-make-whitehalls-unpublishing-call-to-publishing-api-synchronous
Follow these steps if you are doing a Rails upgrade.