Skip to content
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

Removed shipping address & customer email from payment intent metadata #2366

Closed
wants to merge 2 commits into from

Conversation

yasserlens
Copy link
Contributor

This should solve #2364

Removed shipping address and customer email from payment intent request metadata. This is because Stripe express payments require a payment intent, but at the point of doing an express payment we may not have customer details (they are fetched from the payment resolver, such as Google/Apple pay.

…ed with the stripe.paymentIntent.create() request. This is because Stripe express payments require a payment intent, but at the point of doing an express payment we may not have customer details (they are fetched from the payment resolver, such as Google/Apple pay.
@netlify
Copy link

netlify bot commented Aug 27, 2023

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit a4f77f0
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/64eba1a5f7d2710007257e36

@michaelbromley
Copy link
Member

Hi, thanks for the detailed investigation and PR.

I'm not understanding something about this though:

Removed shipping address and customer email from payment intent request metadata

I don't see this in the changes to the PR. All I see is that you are removing the call to options.metadata().

@yasserlens
Copy link
Contributor Author

Hey Michael

options.metadata() is deconstructed, generating the following pairs when an order has no customerEmail or shippingAddress attached:

{
"customerEmail": "unknown",
"shippingAddress": "undefined undefined, undefined, undefined undefined, undefined",
}

Removing the call you referred to should omit the key-value pairs above. The rest are hard-coded:

{ 
channelToken: ctx.channel.token,
orderId: order.id,
orderCode: order.code,
}

The unit tests don't check for shippingAddress, but they check for the customerEmail field, which is why I removed it from the checks to make the tests pass.

@michaelbromley
Copy link
Member

options.metadata() is deconstructed, generating the following pairs when an order has no customerEmail or shippingAddress attached:

What I'm not understanding is that by default, the options.metadata function is undefined. This means that unless you yourself are defining a metadata function which returns the customerEmail and shippingAddress keys, how is this being included in the metadata that gets sent to Stripe?

@yasserlens
Copy link
Contributor Author

yasserlens commented Aug 29, 2023

Great Catch - Michael. I actually do define this metadata in Stripe's config when setting up (in vendure-config.ts).
The reason why they're their is to make sure customer details are attached to the payment, so we could find the customer payment in Stripe if needed given shipping info.

We can remove this and this should solve the issue without going with this change.
However, keeping such metadata which can change from one call to another for the same order could result in this happening again to other Stripe plugin users. Do we want to keep this metadata in the paymentIntent.create() call?

Happy to discard the change if you think it's necessary to keep it.

@michaelbromley
Copy link
Member

Thanks for the clarification. I'm gonna close this because I don't think it's the right direction for a solution. I've made another suggestion in the original issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants