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

Return a 500 status code when there is a genuine error with a payment #357

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ushkarev
Copy link
Contributor

… which can be caused by failing to communicate with the mtp-api or GOV.UK Pay or if GOV.UK Pay themselves experienced an error downstream

@ushkarev ushkarev added the wip label May 15, 2020
@ushkarev ushkarev requested a review from a team as a code owner May 15, 2020 10:39
@ushkarev
Copy link
Contributor Author

returning a 500 status was suggested as a comment on #353 but there would be an annoying side effect. we frequently see P0050 errors in GOV.UK Pay payments and these are generally ignored unless there's a surprising spike. so our monitoring would also pick up lots of 500 response codes and i don't want to get into the habit of ignoring 500s as well!

mattbee
mattbee previously approved these changes May 15, 2020
Copy link

@mattbee mattbee left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mattbee
Copy link

mattbee commented May 15, 2020

returning a 500 status was suggested as a comment on #353 but there would be an annoying side effect. we frequently see P0050 errors in GOV.UK Pay payments and these are generally ignored unless there's a surprising spike. so our monitoring would also pick up lots of 500 response codes and i don't want to get into the habit of ignoring 500s as well!

Unless we do a cloudflare approach and make up a 5XX error. No idea of implications or if this would even be a good idea, but cloudflare added additional 5XX errors for their service.

@ushkarev
Copy link
Contributor Author

hadn't thought of that actually. don't know what downsides there may be. i think our monitoring picks up 5xx equally, but we could tweak that instead i guess.

other than "doing the right thing" by web standards, what benefit is there of returning 500? this is for human-facing views rather than an api.

alternatively, we could be a little hacky and return 500 only for non P0050 errors?

@mattbee
Copy link

mattbee commented May 15, 2020

hadn't thought of that actually. don't know what downsides there may be. i think our monitoring picks up 5xx equally, but we could tweak that instead i guess.

other than "doing the right thing" by web standards, what benefit is there of returning 500? this is for human-facing views rather than an api.

alternatively, we could be a little hacky and return 500 only for non P0050 errors?

I can't think of any downsides to setting our own error status code for P0050 errors and 500 for others. So long as we don't pick one with a standardised meaning yet (from quick research, just avoiding any listed here would be OK) and keep 500 errors as something we still don't ignore. Maybe Sentry can swap 5XX for P0050 errors to a warning or something. Just bike shedding here though really, so long as we don't create a habit of ignoring 500 errors as you warn might happen.

@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from 24fa41d to fe0d471 Compare May 27, 2020 13:11
@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from fe0d471 to f4ee96b Compare July 31, 2020 11:46
@kjdchapman
Copy link
Contributor

@ushkarev @Staberinde any objections to closing this as stale?

@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from f4ee96b to 80d4f48 Compare November 30, 2020 10:29
@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from 80d4f48 to 25285db Compare February 2, 2021 14:23
@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from 25285db to b014b35 Compare March 5, 2021 12:13
@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from b014b35 to b0625d3 Compare March 17, 2021 14:59
@ushkarev ushkarev force-pushed the payment-errors-return-500 branch 3 times, most recently from 1c2f1fb to 159f2ca Compare April 26, 2021 10:13
… which can be caused by failing to communicate with the mtp-api or GOV.UK Pay or if GOV.UK Pay themselves experienced an error
@ushkarev ushkarev force-pushed the payment-errors-return-500 branch from 159f2ca to de0a5b0 Compare May 24, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants