[ncp] handle create order errors #2320
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2320 +/- ##
==========================================
+ Coverage 51.30% 51.32% +0.02%
==========================================
Files 104 104
Lines 2037 2038 +1
Branches 604 604
==========================================
+ Hits 1045 1046 +1
Misses 889 889
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
jshawl
force-pushed
the
handle-ncp-errors
branch
from
c1fa20a to
fc488aa
Compare
src/hosted-buttons/utils.js
Outdated
| container?.appendChild(div); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I'm not sure I like this implementation. I did it so that I didn't have to pass popupFallback and selector down into onApprove. e.g. the alternative to this would be:
onApprove: buildHostedButtonOnApprove({
merchantId,
hostedButtonId,
selector,
popupFallback
})and then inside of buildHostedButtonOnApprove would be the contents of this function. I did it this way so that I could easily unit-test it without managing passing around props but now that I'm looking at it in the GitHub diff it looks a little confusing. wdyt?
There was a problem hiding this comment.
Ease of testing is a good reason in my book for going this route. Thanks for explaining this!
src/hosted-buttons/utils.js
Outdated
| @@ -149,19 +174,14 @@ export const buildHostedButtonOnApprove = ({ | |||
| context_id: data.orderID, | |||
| }), | |||
| }).then((response) => { | |||
| // remove the popup fallback message, if present | |||
| document.querySelector(`.${popupFallbackClassName}`)?.remove(); | |||
There was a problem hiding this comment.
this is another weird thing in this PR. I was really hoping to have all the dom manipulation contained within the popup handling code, but we need this here to handle a specific use case:
- buyer uses inline guest to checkout, has popup blocked, error message is rendered in the dom
- buyer uses a different payment method ("paypal") and when they click on the button, there should no longer be an error message.
src/hosted-buttons/utils.js
Outdated
| container?.appendChild(div); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Ease of testing is a good reason in my book for going this route. Thanks for explaining this!
Description
This PR shows a helpful error message to the buyer when create order responds without an order id. It also removes the custom popup for inline guest.
Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
If a merchant accidentally changes the client id or hosted button id, buyers will not be able to complete a checkout, and no error message is shown. This PR shows the error message above the buttons.
Reproduction Steps (if applicable)
Not yet applicable in a production/sandbox environment, but let me know if you want to try it out locally and I can share the details
❤️ Thank you!