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

PP-11601 Improve how we handle rejected authorisation for wallets. #3756

Merged
merged 4 commits into from
Nov 7, 2023

Conversation

olatomgds
Copy link
Contributor

As part of the review, a Pact test update was requested as per AC.

WHAT

We recently looked at this in PP-11470: We show a technical problems page when Google Pay payments are declined.

However, we are expecting a 400 to always mean the authorisation was rejected - however, it could also mean that the request to connector has an invalid payload which we should handle by showing an error page.

The original change involves the pay-connector service returning an “error_identifier":"AUTHORISATION_REJECTED" in the error response for declined payments for the authorise wallet payment endpoints so we can be sure the 400 means the payment has been declined.

This change involved updating the Pact test to cover this error handling implementation

HOW

Review and run Pact test connector-client-apple-authentication.pact.test.js and associated stub.

@olatomgds olatomgds marked this pull request as draft November 6, 2023 13:48
As part of the review, a Pact test update was requested as per AC.
As part of the review, a Pact test update was requested as per AC.

With secrets.baseline update
As part of the review, a Pact test update was requested as per AC.

With secrets.baseline update
@olatomgds olatomgds force-pushed the PP-11601-Handle-rejected-authorisation-Tests branch from c8bcd76 to ceb7f12 Compare November 7, 2023 11:27
@olatomgds olatomgds marked this pull request as ready for review November 7, 2023 12:07
Copy link
Collaborator

@nlsteers nlsteers left a comment

Choose a reason for hiding this comment

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

two of these commits have identical messages and should be squashed, I'd also suggest keeping any commit messages relevant to the changes in the PR.

for ex.

PP-11601 improve rejected authorisation handling for wallets

- updated apple wallet pact test wrt new error handling behaviour in connector

referencing 'the review' is meaningless for anyone looking back on these changes in the future, please can you tidy up the commit history prior to merging

otherwise looks fine 👍🏻 , I see the google pay side of this work has been moved into a separate story

@olatomgds
Copy link
Contributor Author

two of these commits have identical messages and should be squashed, I'd also suggest keeping any commit messages relevant to the changes in the PR.

for ex.

PP-11601 improve rejected authorisation handling for wallets

- updated apple wallet pact test wrt new error handling behaviour in connector

referencing 'the review' is meaningless for anyone looking back on these changes in the future, please can you tidy up the commit history prior to merging

otherwise looks fine 👍🏻 , I see the google pay side of this work has been moved into a separate story
Thanks, I would normally add a comprehensive comment on-squashing the commits via the UI post approval, but points well noted.

@olatomgds olatomgds closed this Nov 7, 2023
@olatomgds olatomgds reopened this Nov 7, 2023
- updated apple wallet pact test wrt new error handling behaviour in connector
@olatomgds olatomgds requested a review from nlsteers November 7, 2023 16:04
Copy link
Collaborator

@nlsteers nlsteers left a comment

Choose a reason for hiding this comment

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

🔥

@olatomgds olatomgds merged commit e30fbde into master Nov 7, 2023
5 checks passed
@olatomgds olatomgds deleted the PP-11601-Handle-rejected-authorisation-Tests branch November 7, 2023 17:57
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.

2 participants