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

feat(medusa-payment-stripe): Enhance stripe payment to handle description and improve status resolution #1404

Merged
merged 6 commits into from
May 10, 2022

Conversation

adrien2p
Copy link
Member

@adrien2p adrien2p commented Apr 21, 2022

Allow the user to either set a static description through configuration such as:

  {
    resolve: `medusa-payment-stripe`,
    options: {
      api_key: STRIPE_API_KEY,
      webhook_secret: STRIPE_WEBHOOK_SECRET,
      payment_description: "Digital arts/services",
    },
  },

Or eventually override the static description through the cart metadata.

@adrien2p adrien2p requested a review from a team as a code owner April 21, 2022 13:37
@ShivamJoker
Copy link

Way to go 🎉

@adrien2p
Copy link
Member Author

Fixes #1401

@srindom
Copy link
Collaborator

srindom commented Apr 21, 2022

@ShivamJoker @adrien2p I actually think that this is already possible if you call:

POST /carts/:id/payment-sessions/stripe
{ description: ..., [potentially more fields here]}

This call would result in:

cartService.updatePaymentSession -> paymentProviderService.updateSessionData -> StripeProvider.updatePaymentData

Where this method is implemented like this:

// [medusa-payment-stripe/.../stripe-provider.js
  async updatePaymentData(sessionData, update) {
    try {
      return this.stripe_.paymentIntents.update(sessionData.id, {
        ...update.data,
      })
    } catch (error) {
      throw error
    }
  }

@adrien2p
Copy link
Member Author

adrien2p commented Apr 21, 2022

@srindom if I did properly understood, the problem is that you need to create the payment intent with the description, otherwise you get an error and therefore there is nothing to update ☺️

Plus from a DX point of view, having to create and then immediately update without any changes in between I dont think it is very nice nor performant 😅 it also increase the complexity when thing can be easier to do for the consumer

Wdyt?

https://stripe.com/docs/india-accept-international-payments#paiements-internationaux-de-services

@ShivamJoker
Copy link

Ah! But still wouldn't it be easier to have a global config at least instead of updating it on cart?

@adrien2p
Copy link
Member Author

@srindom any suggestion regarding the doc I linked in my comment?

@olivermrbl
Copy link
Contributor

Given that descriptions are required on payments in some countries, I would argue that adding it as a configurable prop to the plugin options is reasonable.

You wrote "& other params" in the feature description @ShivamJoker - do you know of any other properties that might be worth looking into before we proceed with this one?

@ShivamJoker
Copy link

I thought for other countries we may need some more params, so I thought but for now this would do the job.

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

Should we change the plugin property to payment_description to make it clear, what is actually configured?

@@ -141,6 +125,7 @@ class StripeProviderService extends PaymentService {
const amount = await this.totalsService_.getTotal(cart)

const intentRequest = {
description: cart?.context?.description ?? this.options?.description,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would argue, that plugin options should take precedence over cart context

Copy link
Member Author

@adrien2p adrien2p Apr 24, 2022

Choose a reason for hiding this comment

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

I wasn't sure, i was thinking that if you want to set a special description depending on the provenance of the order etc if the configuration is taking precedence it means that you can't. Wdty? The description could integrate some interesting information about the context of the payment

Copy link
Member Author

Choose a reason for hiding this comment

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

@olivermrbl wdty?

@ShivamJoker
Copy link

I think yeah we can do that.

@adrien2p
Copy link
Member Author

Should we change the plugin property to payment_description to make it clear, what is actually configured?

Yeah agree 👍 ill do the changes asap

@adrien2p
Copy link
Member Author

@olivermrbl could you point me to where I should update the doc. Only if you want it to be part of the pr

@adrien2p adrien2p added this to In review in medusajs May 10, 2022
@adrien2p adrien2p moved this from In review to Approved in medusajs May 10, 2022
@adrien2p adrien2p moved this from Approved to In review in medusajs May 10, 2022
@olivermrbl
Copy link
Contributor

olivermrbl commented May 10, 2022

@adrien2p did you test against the real Stripe API, that an undefined description is allowed when creating the payment intent?

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM, but please address comment before merging :)

@olivermrbl olivermrbl moved this from In review to Approved in medusajs May 10, 2022
@adrien2p adrien2p force-pushed the feat-stipePluginEnhancement branch from 211a9ae to 6cc66e8 Compare May 10, 2022 15:06
@olivermrbl olivermrbl merged commit 327614e into medusajs:develop May 10, 2022
medusajs automation moved this from Approved to Done May 10, 2022
SGFGOV pushed a commit to SGFGOV/medusa that referenced this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants