Skip to content
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

Add IR failure template and handle failure #324

Merged
merged 21 commits into from
Oct 5, 2020

Conversation

JamesGardiner
Copy link
Contributor

What is the context of this PR?

Handles individual response publish failures by returning the individual response error template using a 500 error code.

@JamesGardiner
Copy link
Contributor Author

return (
render_template("errors/individual_response", retry_url=retry_url),
500,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to decide a consistent approach for rendering error pages on exceptions. In #318 this is handled in errors.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer the approach in #318 - it's at the least consistent with what we currently do

Comment on lines 578 to 589
try:
self._publish_fulfilment_request()
except PublicationFailed:
retry_url = url_for(
"individual_response.individual_response_post_address_confirm",
list_item_id=self._list_item_id,
**self._request_args,
)
return (
render_template("errors/individual_response", retry_url=retry_url),
500,
)
Copy link
Contributor

@MebinAbraham MebinAbraham Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the try except was moved into _publish_fulfilment_request you could avoid the duplication in the handle_post for mobile and post. You can work out the retry URL from the fact that you have a mobile number etc. Then, the method name might then be more appropriate as handle_fulfiilment_request or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about handle_fulfiilment_request - I think I prefer handle_fulfilment_request 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this gets uglier when you consider the handle_post then has to check the return value of the fulfilment request method to determine if it has failed or not - in this instance I think it is better to be explicit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't if you are going to redirect to the new page on the exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This disappears in any case if I follow the approach in #318, as I can handle the logic in the registered error handler.

I'm making this change now btw, so let me know what you prefer when I make the push

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in f18f331 - I've created a new Exception class, FulfilmentRequestFailedException, to handle this apart from generic PubSub PublicationFailed errors.

app/routes/errors.py Outdated Show resolved Hide resolved
templates/errors/fulfilment_request.html Outdated Show resolved Hide resolved
app/routes/errors.py Outdated Show resolved Hide resolved
templates/errors/fulfilment-request.html Outdated Show resolved Hide resolved
app/routes/errors.py Show resolved Hide resolved
app/views/handlers/individual_response.py Show resolved Hide resolved
@JamesGardiner JamesGardiner merged commit 410cf2c into master Oct 5, 2020
@JamesGardiner JamesGardiner deleted the individual-response-error-page branch October 5, 2020 10:23
@pricem14pc pricem14pc added this to the v3.50.0 milestone Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants