W-20394105: Save payment method for future use#3570
W-20394105: Save payment method for future use#3570rasbhat merged 12 commits intot/team404/sfp-on-pwafrom
Conversation
|
Thanks for the contribution! It looks like @rvishwanathbhat is an internal user so signing the CLA is not required. However, we need to confirm this. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
packages/template-retail-react-app/app/pages/checkout/partials/sf-payments-sheet.jsx
Show resolved
Hide resolved
| statusCode, | ||
| errorMessage, | ||
| errorDetails, | ||
| requestBody: JSON.parse(JSON.stringify(requestBody)), |
There was a problem hiding this comment.
How can we be sure it's safe to log the entire request body? If we keep this, let's please not do parse(stringify()).
There was a problem hiding this comment.
Removed the Request body. Modified to log only error details.
|
|
||
| const account = findPaymentAccount(paymentMethodSetAccounts, paymentMethodType) | ||
| if (!account) { | ||
| return paymentMethodType === 'card' ? 'stripe' : null |
There was a problem hiding this comment.
I don't understand the need for these fallbacks. Also, can we put constants in a file to use here instead of repeating strings?
There was a problem hiding this comment.
Removed these, now just returning null if an account isn't found.
|
@rasbhat I saw the save as default checkbox in the screenshot. Since we don't support default SPM yet, should we suppress that checkbox? |
Makes sense, I've disabled the save as default checkbox. |
| */ | ||
| export const PARTIAL_HYDRATION_ENABLED = false | ||
|
|
||
| // Constants for Salesforce Payments |
There was a problem hiding this comment.
Would these be better added to use-sf-payments? I honestly don't know.
There was a problem hiding this comment.
Ah, that seems to be a hooks specific. Let me see if I can move it to payment specific file
| const handlePaymentButtonApprove = async (event) => { | ||
| try { | ||
| const updatedOrder = await createAndUpdateOrder() | ||
| if (event?.detail?.savePaymentMethod !== undefined) { |
There was a problem hiding this comment.
Is the idea here only to overwrite savePaymentMethodRef.current if we get event.detail.savePaymentMethod and the value is a boolean? If there are reasons to sometimes not get a savePaymentMethod value in this event, or not even be passed an event at all, it would be good to memorialize those reasons in a comment here.
There was a problem hiding this comment.
Added a comment regarding the same
| paymentMethodType.current, | ||
| zoneId | ||
| zoneId, | ||
| undefined, |
There was a problem hiding this comment.
It's unusual to pass undefined like this. Would null suffice?
There was a problem hiding this comment.
Yeah null should do. Updated it accordingly
| paymentElement.addEventListener('sfp:paymentapprove', handlePaymentButtonApprove) | ||
| paymentElement.addEventListener('sfp:paymentcancel', handlePaymentButtonCancel) | ||
| paymentElement.addEventListener('sfp:savepaymentmethodchange', (event) => { | ||
| savePaymentMethodRef.current = event.detail?.savePaymentMethod === true |
There was a problem hiding this comment.
This will set savePaymentMethodRef.current to false if the event has no detail or detail has no savePaymentMethod. Is that intentional? Are there sfp.savepaymentmethodchange events which will be missing those data?
There was a problem hiding this comment.
BTW I didn't recognize sfp:savepaymentmethodchange from the ecom SDK, and when I searched I didn't find it. Where did you see an event being fired with this name?
There was a problem hiding this comment.
I was trying to have a listener on any changes in the payment method, buthandlePaymentMethodSelected is already doing the job by updating savePaymentMethodRef.current from evt.detail.savePaymentMethodForFutureUse, removed this redundant one. In handlePaymentMethodSelected, we only update when value is explicitly provided or preserve the previous value if not.
|
|
||
| try { | ||
| // Update order payment instrument to create payment | ||
| const paymentInstrumentBody = createPaymentInstrumentBody( |
There was a problem hiding this comment.
We've added quite a bit to this component why do we not have test coverage? use-f-payments.js should also probably have unit test. Have you ran the necessary CI commands like test coverage and linting? Since we are working on feature branch I'm afraid that we are pushing stuff that would normally be gated.
There was a problem hiding this comment.
All the changes we have are in event handlers that are triggered by SDK events (eg: sfp:paymentapprove). Testing this might require dispatching SDK events, and I wasn't able to come across any tests in codebase that did it. The existing tests in sf-payment-sheet.test.js as well didn't cover any of these functions.
I've covered all utility function tests.
I've run lint on all files. Attaching it in the description.
There was a problem hiding this comment.
This is concerning. We have changes here regarding confirming a payment yet existing tests aren't updated and no new tests are added. If what you say is true then our feature branch likely doesn't have sufficient coverage to be merged. CC @amittapalli
There was a problem hiding this comment.
:( We are behind in coverage for sure. Limited tests were added for things during the development phase. Let me create a WI to track this. We start slow and fix the low hanging fruits first and increase coverage.
Thing is the PWA team has also suggested refactoring some of these components that have grown over time and hard to understand. No decisions have been made since refactoring can also create further instability. If any refactoring is done, the tests need to be rewritten anyway.
And due to the heavy dependency on SDK, events etc. testing some of the scenarios will require more time, hence we need separate WIs to go over this carefully
There was a problem hiding this comment.
Since SF Next is the path forward I recommend against heavy refactoring. Whether or not we choose to refactor, we need test coverage for many reasons. IMO it can come soon after feature delivery since it's a form of tech debt to pay down, but since we could find subtle bugs in the process it's not something we should delay for very long. We just have to balance our priorities.
There was a problem hiding this comment.
Added test file to test SDK events sf-payments-sheet.events.test.js
- Increased sf-payments-sheet.js coverage to 75%
- Increased the
sf-payments-utils.test.jscoverage to 100%. - Increased
use-sf-payments.jscoverage to 95%.
|
|
||
| await waitFor(() => { | ||
| const checkbox = screen.queryByRole('checkbox', {name: /same as shipping/i}) | ||
| if (checkbox) { |
There was a problem hiding this comment.
so if checkbox is falsy for some reason wouldn't we get a false positive?
There was a problem hiding this comment.
Makes sense, Changed from conditional assertion to explicit assertion, so the test will fail if the checkbox exists unexpectedly.
| const updateCall = mockUpdatePaymentInstrument.mock.calls[0] | ||
| const requestBody = updateCall[0].body | ||
| expect(requestBody.paymentReferenceRequest.gatewayProperties.stripe.setupFutureUsage).toBe('on_session') | ||
| expect(querySelectorSpy).toHaveBeenCalledWith('.sfpp-save-payment-method-for-future-use input[type="checkbox"]') |
There was a problem hiding this comment.
While I like the effort to push this test to more of an integration style test I think we should keep these more as unit tests. IMO this test gives us the assurance we need by firing the event, calling confirm and validating the API request. If you feel this adds more reliable assurance then feel free to keep it but I think the checkbox is the responsibility of the SDK and not this component.
There was a problem hiding this comment.
Refactored tests : removed DOM checks, focused assertions on behavior rather than implementation details.
There was a problem hiding this comment.
I see you removed some tests such as confirmPayment reads checkbox state for shouldSavePaymentMethod. To be clear I think the tests I was referring to were necessary and good tests. I was just saying that we didn't need the part regarding the SFP controlled checkbox. That is reaching outside the boundary of this component and complicates the test. However, it make perfect sense to make sure that confirm is being called with the correct arguments particularly after these events have been fired.
There was a problem hiding this comment.
Makes sense. I took out the test previously as they were emulating integration tests and having to read state of checkbox as you mentioned. Added back the confirmpayment behavior tests for when the events are fired.
| describe('onRequiresPayButtonChange callback', () => { | ||
| test('renders successfully with callback provided', () => { | ||
| const mockOnRequiresPayButtonChange = jest.fn() | ||
| test('confirmPayment sets setup_future_usage in paymentIntent when shouldSavePaymentMethod is true', async () => { |
There was a problem hiding this comment.
This test and previous one appear to have a lot of duplicated code. Opportunity to DRY it up and make it more readable
There was a problem hiding this comment.
Refactored to remove duplication
| futureUsageOffSession = false, | ||
| paymentMethods = null, | ||
| paymentMethodSetAccounts = null, | ||
| isPostRequest = false |
There was a problem hiding this comment.
There are a lot of arguments to this function. Would it be more consumable to accept an object instead?
There was a problem hiding this comment.
Makes sense, refactored to accept a single object parameter.
sf-mkosak
left a comment
There was a problem hiding this comment.
Thanks for adding tests. Have a couple comments you should consider.
| // Update order payment instrument to create payment | ||
| updatedOrder = await createAndUpdateOrder() | ||
| const checkbox = containerElementRef.current?.querySelector( | ||
| '.sfpp-save-payment-method-for-future-use input[type="checkbox"]' |
There was a problem hiding this comment.
If I'm reading this right, this is reaching into the DOM rendered by the SDK to pull out the value. This code must not depend on the DOM in that way and will need to change. In SFRA we were able to implement all necessary work for SPMs using events and promises etc., so there should be no reason PWA needs to depend on internals in this way. Have a look at plugin_salesforcepayments client side JS if you need examples of that.
There was a problem hiding this comment.
Removed the DOM dependency and used savePaymentMethodRef.current to get the value.
There was a problem hiding this comment.
I have to head OOO so I don't have time to review the changes, but I re-requested myself to remove the request changes blocker. @sf-mkosak you had made a similar comment so please have a look at the latest when you get a chance. Thanks!
d73d5bb to
3bdc9f2
Compare
…/save-payment-method-for-future-use W-20394105: Save payment method for future use
…/save-payment-method-for-future-use W-20394105: Save payment method for future use
…/save-payment-method-for-future-use W-20394105: Save payment method for future use
A registered customer will be able to save payment method.
Description
GET customer API call response after payment method is saved:
Tests covered:
Lint:
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization