@W-21012995, W-21103673: Adyen support for SPM

[rasbhat]

Provides Adyen support for SPMs. This PR also prevents re-render of checkout component with SPM changes.

Types of Changes

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Documentation update
• Breaking change (could cause existing functionality to not work as expected)
• Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

• Removing a public function or component or prop
• Adding a required argument to a function
• Changing the data type of a function parameter or return value
• Adding a new peer dependency to package.json

Changes

• (change1)

How to Test-Drive This PR

• (step1)

Checklists

General

• Changes are covered by test cases
• CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

• There are no changes to UI

or...

• Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
• Changes comply with WCAG 2.0 guidelines levels A and AA
• Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

• Changes include a UI text update in the Retail React App (which requires translation)

[cc-prodsec]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[rasbhat]

The displayed name for Mastercard is not getting handled in the SDK for Adyen. Will be rasing a PR for the same.

[amittapalli]

If you want to clean it up, you can also try this before the effect:

const canSaveForFutureUsage =
!!(customer?.isRegistered && customer?.customerId)

Then use it this way inside the effect:
showSaveForFutureUsageCheckbox: canSaveForFutureUsage,

And just pass canSaveForFutureUsage in the dependency if that is the only purpose of checking for isRegistered

[rasbhat]

Makes sense. Updated

[amittapalli]

Or shorten it to
...(storePaymentMethod && {
storePaymentMethod: true
})

[rasbhat]

Updated

[jeffraab-sfdc]

The displayed name for Mastercard is not getting handled in the SDK for Adyen. Will be rasing a PR for the same.

I thought we had normalized the SDK brand constants based on the Stripe constants. Where do you plan to make the change? It sounds like you mean in the SDK but it might be ecom that's at fault.

[rasbhat]

I thought we had normalized the SDK brand constants based on the Stripe constants. Where do you plan to make the change? It sounds like you mean in the SDK but it might be ecom that's at fault.

I'm currently getting 'mc' as the brand from the backend for the mastercard. Should we be normalizing in PWA before sending to SDK? @jeffraab-sfdc

[jeffraab-sfdc]

Should we be normalizing in PWA before sending to SDK?

Yes, I think we should normalize but in ecom. Can you file a bug for that? As part of the description please indicate if this only affects PWA (i.e. SCAPI) or if it also affects SFRA (i.e. Script API).

[rasbhat]

Should we be normalizing in PWA before sending to SDK?

Yes, I think we should normalize but in ecom. Can you file a bug for that? As part of the description please indicate if this only affects PWA (i.e. SCAPI) or if it also affects SFRA (i.e. Script API).

Sure got it. I'll file a bug for the same.

[sf-mkosak]

Nit: We have these individual checks that short circuit the effect but then we have below a conditional gating the effect's implementation. Any reason to not add to the below conditional? The way I read this effect is basically like below. Wondering if these conditions just need to be part of readyToMountSFP in the below pseudo.

useEffect(() => {
    if (readyToMountSFP) {
          mountSFP();
    }
}, [dependencies]);

[amittapalli]

Not sure if I follow but isDataLoading (or whatever semantics you choose) needs to be a condition as well as a dependency in this example if I understood how React works. So dependencies control when the effect runs, while the condition controls what happens when it runs. So in this case, anytime the value of isDataLoading changes, the effect runs and when it runs, you want to make sure the logic is not executed if data is still loading, something like that.

[sf-mkosak]

I'm just saying we should be consistent. We were short circuiting(added in this pr) and gating the execution based on a condition(already implemented). Both things effectively accomplish the same thing but doing it differently affects readability in a negative way.

[sf-mkosak]

Nit: naming. Is what data loading? isRegistered is not about data loading.

[amittapalli]

I think she is tracking the 'isLoading' property that comes with any of the query hooks. She just aliased it to 'customerLoading'. She is then setting that flag based on whether customer is registered and if so, is the data (ex: SPM) is still loading. Maybe rename it to 'isCustomerDataLoading'?

[sf-mkosak]

I get the condition its just a nitpick on the name. If your not in view of where we assign the value you dont have much context into what it actually means. isCustomerDataLoading is good for the loading part but we have isRegistered embedded in it which is an entirely different check unrelated to loading of the data.

[rasbhat]

Yeah, we only wait on the data to load for a Registered user, so needed both of the checks as a single const to be present in the dependency array. I've renamed it to isCustomerDataLoading as Anitha suggested. Not sure how to give a combined name. Probably something along the lines of a descriptive isRegisteredAndSpmDataLoading?

[sf-mkosak]

Not to beat a dead horse (what an expression!) but I personally find all the negation hard to read. Why do some checks use !! while others !? I think we could either create variables that would make the main gating condition not have to negate all the things or we short circuit based on each individual condition.

// A nice semantic easy to understand variable name one can read without evaluating the underlying logic
const isCustomerDataReady = !isCustomerDataLoading
...same with rest of the variables

const readyToMountSFP = 
    isCustomerDataReady && ....the rest of the conditions

// or we could short circuit each check individually
// Allows you to add comments for each check if some conditions are less clear like checkoutComponent.current

if (isCustomerDataLoading) return
if (checkoutComponent.current) return
if (!sfp) return
...etc

[rasbhat]

Makes sense on the readability.. I was using the ! to negate, and !! to get the truthy values of the object. Modified it to descriptive variable names.

[jeffraab-sfdc]

IMO the hasXYZ constants which are just a restating of a negated variable - they're too much. Whatever the team is comfortable with is fine with me, but after reading the PR comments and this result the pendulum swung too far in the other direction.

Just some basic principles I've followed over the years:

• Negation causes additional cognitive load so avoid it when possible
• Consistency and symmetry are recognizable ways to reduce load, often more than the names themselves
• When you change the meaning of something, or combine meanings of things into a new thing, give it a new name
• If your condition is in that "in between" space where it's not incredibly simple, but it's not so hard as to warrant a function abstraction, use a human readable comment to describe your condition

My comment for this might be something like "We're ready to mount when sfp.js is loaded, we have the metadata, configuration, and container element needed to create the component."

I left out isCustomerDataReady because it's not clear why we're checking that condition. If it's because isCustomerDataReady being true means savedPaymentMethods is valid then it's more readable to check that.

I also left out checking if a component has already been created because it actually isn't a condition for when we're ready to mount. In that case we were already ready to mount before and did, so it's separate for me.

Use this info however you like.

[sf-mkosak]

Those are all good principles that I would agree with. Since I started this conversation I'll just say that I think the readability here is important because mounting SFP correctly is clearly very complex and getting it wrong results in high priority bugs. This is also reference code so it's going to end up the hands of other external developers that may want to customize it. Hopefully it doesn't feel like unnecessary churn on something trivial.

[jeffraab-sfdc]

+1000 that the readability is important for the reasons you mentioned.

[rasbhat]

Yeah absolutely, makes sense. In that case, instead of overloading, how about checking them individually as Matt previously suggested , with comments in line, so that they're descriptive as well:

   useEffect(() => {
        // Mount SFP only when all required data and DOM are ready; otherwise skip or wait for a later run.

        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
        if (!metadata) return // Skip if SFP metadata not available yet
        if (!containerElementRef.current) return // Skip if Payment container ref not attached to DOM yet
        if (!paymentConfig) return // Skip if Payment config not loaded yet

[jeffraab-sfdc]

Should we be using isRegistered here rather than customer?.isRegistered?

[jeffraab-sfdc]

Also - what does customer?.customerId actually check? If it's a way to check if customer is an object, that will already be checked by the customer?.isRegistered because it will be falsy if customer is null/undefined as well as if customer.isRegistered is false.

[rasbhat]

I was checking if the customerID from the GET Customer is valid. So the SPM can be tied to a customer with valid ID

[jeffraab-sfdc]

I don't see how isRegistered could be true without a customer ID. This may end up being a proxy for isCustomerDataReady below, since customer won't be available until loading is complete. I'm aiming for as much clarity and simplicity as we can get, given the complexity of the conditions involved in the mounting. IMO react is just not good at expressing this.

[rasbhat]

Good call out, Seems like customerID was just a proxy of isCustomerDataLoading, where it just waited for customer data to load. I took out the canSaveForFutureUsage and just directly used isRegistered to display the checkbox keeping it simple and removed it from the dependency array as isCustomerDataLoading already checks if the customer is registered.

[jeffraab-sfdc]

Nice!