-
Notifications
You must be signed in to change notification settings - Fork 13
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
One time checkout - get user type #6560
base: main
Are you sure you want to change the base?
Conversation
Size Change: +87 B (0%) Total Size: 2.22 MB ℹ️ View Unchanged
|
@@ -147,7 +148,10 @@ function paymentResultFromObject( | |||
}); | |||
} | |||
|
|||
return Promise.resolve(PaymentSuccess); | |||
const response = json as { data?: { userType: UserType } }; |
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.
Admittedly a bit of a bodge 🤷♂️
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.
Would it be worth writing a zod schema to validate this, rather than casting?
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.
Yeah that would be ideal, there's another cast in this function so definitely a worthwhile refactor
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 looks great! 👍
I left a few minor comments/questions but they aren't blockers.
@@ -147,7 +148,10 @@ function paymentResultFromObject( | |||
}); | |||
} | |||
|
|||
return Promise.resolve(PaymentSuccess); | |||
const response = json as { data?: { userType: UserType } }; |
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.
Would it be worth writing a zod schema to validate this, rather than casting?
@@ -463,6 +463,9 @@ export function OneTimeCheckoutComponent({ | |||
}); | |||
const thankYouUrlSearchParams = new URLSearchParams(); | |||
thankYouUrlSearchParams.set('contribution', finalAmount.toString()); | |||
'paymentStatus' in paymentResult && |
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.
Why do we need to check this?
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.
paymentResult could be PaymentResult
| CreatePayPalPaymentResponse
. userType
isn't on the payPal type (although possibly could be). So it's a way of type switching that we use further down too
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.
(Really sorry if I'm missing something obvious!) Should it not be 'userType' in paymentResult
in that case?
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.
Yep that makes more sense, it was a copy paste from below - my initial intent was to add it to PayPalResponse
What are you doing in this PR?
Returning the identity user type for one time payments through the API, and using this information on the client to set a URL param on the thank you page.
This client side solution does not work for PayPal, as we redirect them to the site and can't set the URL params
How to test
Use the new one time checkout and see "userType=x" when redirected to the thank you page.
I used the always authenticate card to simulate 3DS, as that makes an additional API call
Screenshots