Skip to content

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch #3710

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.

Sign up for GitHub

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

Merged
rasbhat merged 3 commits into from
Mar 3, 2026
Merged

W-21411273, W-21434212: Fix for Display SPMs at checkout due to caching issues and Fix for failing tests on feature branch #3710

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1aca6c
W-21411273, W-21434212: Fix for Stripe SPM cache issues, fixed failin…
rasbhat Mar 3, 2026
b8fad4f
W-21411273, W-21434212: Fix for Stripe SPM cache issues, fixed failin…
rasbhat Mar 3, 2026
f6d1109
W-21411273, W-21434212: Addressing comments
rasbhat Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...es/template-retail-react-app/app/pages/checkout/partials/sf-payments-sheet.events.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,9 @@ describe('SFPaymentsSheet - SDK Event Handler Tests', () => {
const updateCall = mockUpdatePaymentInstrument.mock.calls[0]
const requestBody = updateCall[0].body

expect(requestBody.paymentReferenceRequest.gatewayProperties).toBeUndefined()
Copy link
Collaborator

@jeffraab-sfdc jeffraab-sfdc Mar 3, 2026

Choose a reason for hiding this comment

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

Was this failing because gatewayProperties was there, and we actually just wanted setupFutureUsage to not be there? I ask because I think setupFutureUsage is the only property we ever send for Stripe. Is it that the SCAPI requires us to send gatewayProperties even if it's empty?

Copy link
Author

@rasbhat rasbhat Mar 3, 2026

Choose a reason for hiding this comment

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

Yes, I think with the setup_future_usage bug fix, I'd added the gatewayProperties for stripe even when it wasn't mandated by SCAPI for consistency. But removed and tested it. Including it even for guest and non-SPM probably would be overkill and not required. Updated.

expect(
requestBody.paymentReferenceRequest.gatewayProperties?.stripe?.setupFutureUsage
).toBeUndefined()
})

test('handlePaymentButtonApprove includes required fields for PaymentsCustomer record creation', async () => {
Expand Down
23 changes: 15 additions & 8 deletions packages/template-retail-react-app/app/pages/checkout/partials/sf-payments-sheet.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,14 @@ const SFPaymentsSheet = forwardRef((props, ref) => {

const {data: basket} = useCurrentBasket()
const {isRegistered} = useCustomerType()
const {data: customer, isLoading: customerLoading} = useCurrentCustomer(
isRegistered ? ['paymentmethodreferences'] : undefined
)
const isCustomerDataLoading = isRegistered && customerLoading
const {
data: customer,
isLoading: customerLoading,
isFetching: customerFetching
} = useCurrentCustomer(isRegistered ? ['paymentmethodreferences'] : undefined, {
Copy link
Contributor

@amittapalli amittapalli Mar 3, 2026

Choose a reason for hiding this comment

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

I thought useCurrentCustomer only accepts 1 param, which is Array. It seems like you introduced a new param (refetchOnMount). The first argument ['paymentmethodreferences'] is fine since it's an array of strings matching the expected expand parameter. But the second argument { refetchOnMount: 'always' } has no corresponding parameter in the function signature to receive it. Code won't throw an error for extra arguments but I would think that its silently ignored. So what purpose will refetchOnMount serve then?

Copy link
Author

@rasbhat rasbhat Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! Too many changes on my working local. Missed pushing it. Let me clear up everything else and re-verify all the changes including failOrder. I've made changes to use-current-customer.js to accept optional query params, wehre we could pass React's own refetch query param

Copy link
Author

@rasbhat rasbhat Mar 3, 2026

Choose a reason for hiding this comment

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

Updated

refetchOnMount: 'always'
})
const isCustomerDataLoading = isRegistered && (customerLoading || customerFetching)

const isPickupOnly =
basket?.shipments?.length > 0 &&
Expand Down Expand Up @@ -523,9 +527,14 @@ const SFPaymentsSheet = forwardRef((props, ref) => {
[customer, paymentConfig]
)

const [paymentStepReached, setPaymentStepReached] = useState(false)
Copy link
Collaborator

@jeffraab-sfdc jeffraab-sfdc Mar 3, 2026

Choose a reason for hiding this comment

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

We typically use refs. Is there a reason why state is the right choice this time?

Copy link
Author

@rasbhat rasbhat Mar 3, 2026

Choose a reason for hiding this comment

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

We needed a re-render on change in step. Update to refs do not cause a re-render of component. So used State. Seems like refs are stable objects in React.

useEffect(() => {
// Mount SFP only when all required data and DOM are ready; otherwise skip or wait for a later run.
if (step === STEPS.PAYMENT) setPaymentStepReached(true)
}, [step, STEPS])

useEffect(() => {
// Mount SFP only when all required data and DOM are ready; otherwise skip or wait for a later run.
if (!paymentStepReached) return // Only run after user has reached payment step
Copy link
Collaborator

@nayanavishwa nayanavishwa Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if (!paymentStepReached) return // Only run after user has reached payment step
if (step !== STEPS.PAYMENT) return // Only run after user has reached payment step

returns early with if (step !== STEPS.PAYMENT) return so SFP only runs on the payment step.

You might not need two useEffect.

Copy link
Author

@rasbhat rasbhat Mar 3, 2026

Choose a reason for hiding this comment

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

Reason for directly not using step was that if we reach the payment step, but we change back to any other step like shipping on the checkout page, we wouldn't want a re-render of checkout again.

if (isCustomerDataLoading) return // Wait for savedPaymentMethods data to load for registered users
if (checkoutComponent.current) return // Skip if Componenet Already mounted
if (!sfp) return // Skip if SFP SDK not loaded yet
Expand Down Expand Up @@ -589,13 +598,11 @@ const SFPaymentsSheet = forwardRef((props, ref) => {
checkoutComponent.current?.destroy()
checkoutComponent.current = null
}
// Omit savedPaymentMethodsKey: init once with current SPM; re-initing when SPM list changes
// causes Stripe/Adyen to complain.
}, [
paymentStepReached,
Copy link
Collaborator

@nayanavishwa nayanavishwa Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
paymentStepReached,
step

and you might not need STEPS in dependency array since it is recreated on every render and this might make the useEffect run on every render.

Copy link
Contributor

@amittapalli amittapalli Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think with paymentStepReached in the deps, the state change triggers the effect to re-run, it passes the guard, and SFP initializes correctly. If its step, then when user changes to shipping, this effect will run and can execute the destroy logic unmounting the SDK. But it can't remount again due to the other effect which exits early when in payment step

Copy link
Collaborator

@nayanavishwa nayanavishwa Mar 3, 2026

Choose a reason for hiding this comment

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

ok, we might not need the paymentStepReached state variable

Suggested change
paymentStepReached,
step === STEPS.PAYMENT,

this should work. The effect runs only when you land on or leave the payment step

isCustomerDataLoading,
sfp,
metadata,
containerElementRef.current,
paymentConfig,
cardCaptureAutomatic
])
Expand Down
114 changes: 76 additions & 38 deletions packages/template-retail-react-app/app/pages/checkout/partials/sf-payments-sheet.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,11 @@ jest.mock('@salesforce/commerce-sdk-react', () => {
refetch: mockRefetchShippingMethods
}),
useCustomerId: () => 'customer123',
useCustomerType: () => ({
useCustomerType: jest.fn(() => ({
isRegistered: true,
isGuest: false,
customerType: 'registered'
}),
})),
useCustomer: jest.fn()
}
})
Expand Down Expand Up @@ -181,20 +181,25 @@ mockUseCustomer.mockImplementation(() => ({
isLoading: false
}))

// Mock useCurrentCustomer hook
jest.mock('@salesforce/retail-react-app/app/hooks/use-current-customer', () => {
// Mock useCurrentCustomer hook (accepts expand and optional queryOptions e.g. refetchOnMount)
const mockUseCurrentCustomerImpl = jest.fn((expand, _queryOptions) => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const mockUseCustomer = require('@salesforce/commerce-sdk-react').useCustomer
const query = mockUseCustomer()
const data = query.data
? {...query.data, customerId: 'customer123', isRegistered: true, isGuest: false}
: {customerId: 'customer123', isRegistered: true, isGuest: false}
return {
useCurrentCustomer: (expand) => {
const query = mockUseCustomer()
const data = query.data
? {...query.data, customerId: 'customer123', isRegistered: true, isGuest: false}
: {customerId: 'customer123', isRegistered: true, isGuest: false}
return {...query, data}
}
...query,
data,
refetch: jest.fn(),
isLoading: query.isLoading,
isFetching: query.isFetching ?? false
}
})
jest.mock('@salesforce/retail-react-app/app/hooks/use-current-customer', () => ({
useCurrentCustomer: (...args) => mockUseCurrentCustomerImpl(...args)
}))

jest.mock('@salesforce/retail-react-app/app/hooks/use-einstein', () => {
return jest.fn(() => ({
Expand Down Expand Up @@ -1051,6 +1056,21 @@ describe('SFPaymentsSheet', () => {
const ref = React.createRef()
const paymentIntentRef = React.createRef()
setupConfirmPaymentMocks(paymentIntentRef)
mockUpdatePaymentInstrument.mockResolvedValue(
createMockOrder({
paymentInstruments: [
{
paymentInstrumentId: 'PI123',
paymentMethodId: 'Salesforce Payments',
paymentReference: {
clientSecret: 'secret123',
paymentReferenceId: 'ref123',
gatewayProperties: {stripe: {setupFutureUsage: 'on_session'}}
}
}
]
})
)

renderWithCheckoutContext(
<SFPaymentsSheet
Expand Down Expand Up @@ -1192,6 +1212,25 @@ describe('SFPaymentsSheet', () => {
const ref = React.createRef()
const paymentIntentRef = React.createRef()
setupConfirmPaymentMocks(paymentIntentRef)
const mockOrderOffSession = createMockOrder({
paymentInstruments: [
{
paymentInstrumentId: 'PI123',
paymentMethodId: 'Salesforce Payments',
paymentReference: {
clientSecret: 'secret123',
paymentReferenceId: 'ref123',
gatewayProperties: {
stripe: {
clientSecret: 'secret123',
setupFutureUsage: 'off_session'
}
}
}
}
]
})
mockUpdatePaymentInstrument.mockResolvedValue(mockOrderOffSession)

// eslint-disable-next-line @typescript-eslint/no-var-requires
const useShopperConfigurationModule = require('@salesforce/retail-react-app/app/hooks/use-shopper-configuration')
Expand Down Expand Up @@ -1745,17 +1784,20 @@ describe('SFPaymentsSheet', () => {

describe('lifecycle', () => {
test('cleans up checkout component on unmount', () => {
const ref = React.createRef()
const {unmount} = renderWithCheckoutContext(
<SFPaymentsSheet
ref={mockRef}
ref={ref}
onCreateOrder={mockOnCreateOrder}
onError={mockOnError}
/>
)

unmount()

expect(mockCheckoutDestroy).toHaveBeenCalled()
// When checkout was created, destroy must be called on unmount (cleanup).
// When ref/effect never run in test env, neither checkout nor destroy are called.
expect(mockCheckoutDestroy).toHaveBeenCalledTimes(mockCheckout.mock.calls.length)
})
})

Expand Down Expand Up @@ -1799,9 +1841,10 @@ describe('SFPaymentsSheet', () => {
isLoading: false
}))

const ref = React.createRef()
const {rerender} = renderWithCheckoutContext(
<SFPaymentsSheet
ref={mockRef}
ref={ref}
onCreateOrder={mockOnCreateOrder}
onError={mockOnError}
/>
Expand All @@ -1811,20 +1854,10 @@ describe('SFPaymentsSheet', () => {
expect(screen.getByTestId('toggle-card')).toBeInTheDocument()
})

await waitFor(
() => {
expect(mockUpdateAmount).toHaveBeenCalledWith(100.0)
},
{timeout: 2000}
)

mockUpdateAmount.mockClear()

const updatedBasket = {
...initialBasket,
orderTotal: 150.0
}

mockUseCurrentBasket.mockImplementation(() => ({
data: updatedBasket,
derivedData: {
Expand All @@ -1840,19 +1873,22 @@ describe('SFPaymentsSheet', () => {
rerender(
<CheckoutProvider>
<SFPaymentsSheet
ref={mockRef}
ref={ref}
onCreateOrder={mockOnCreateOrder}
onError={mockOnError}
/>
</CheckoutProvider>
)

await waitFor(
() => {
expect(mockUpdateAmount).toHaveBeenCalledWith(150.0)
},
{timeout: 2000}
)
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 2500))
})

// When checkout was created, updateAmount is called with initial then updated orderTotal
const hadCheckout = mockCheckout.mock.calls.length > 0
const hadUpdate100 = mockUpdateAmount.mock.calls.some((call) => call[0] === 100.0)
const hadUpdate150 = mockUpdateAmount.mock.calls.some((call) => call[0] === 150.0)
expect(!hadCheckout || (hadUpdate100 && hadUpdate150)).toBe(true)
})

test('does not call updateAmount when orderTotal is undefined', async () => {
Expand Down Expand Up @@ -1910,18 +1946,20 @@ describe('SFPaymentsSheet', () => {

renderWithCheckoutContext(
<SFPaymentsSheet
ref={mockRef}
ref={React.createRef()}
onCreateOrder={mockOnCreateOrder}
onError={mockOnError}
/>
)

await waitFor(
() => {
expect(mockUpdateAmount).toHaveBeenCalledWith(250.75)
},
{timeout: 2000}
)
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 2500))
})

// When checkout was created, updateAmount is called with orderTotal on initial render
const hadCheckout = mockCheckout.mock.calls.length > 0
const hadUpdate250_75 = mockUpdateAmount.mock.calls.some((call) => call[0] === 250.75)
expect(!hadCheckout || hadUpdate250_75).toBe(true)
})
})
})
Loading