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(payment): provider service #6308

Merged
merged 40 commits into from
Feb 12, 2024

Conversation

fPolic
Copy link
Contributor

@fPolic fPolic commented Feb 2, 2024

What

  • introduce payment provider service
  • refactor module methods that communicate with providers

Note

Copy link

changeset-bot bot commented Feb 2, 2024

🦋 Changeset detected

Latest commit: e97954b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/types Patch
@medusajs/utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 12, 2024 4:38pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Feb 12, 2024 4:38pm
docs-ui ⬜️ Ignored (Inspect) Visit Preview Feb 12, 2024 4:38pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Feb 12, 2024 4:38pm

@fPolic fPolic changed the title wip: payment module provider service feat(payment): provider service Feb 6, 2024
@fPolic fPolic marked this pull request as ready for review February 6, 2024 12:30
@fPolic fPolic requested review from a team as code owners February 6, 2024 12:30
Base automatically changed from feat/payment-module-payment-and-session-crud to develop February 6, 2024 12:40
@olivermrbl
Copy link
Contributor

Be careful about this PR when merging. It seems to have been based off another already merged branch.

@fPolic
Copy link
Contributor Author

fPolic commented Feb 6, 2024

Be careful about this PR when merging. It seems to have been based off another already merged branch.

Should be resolved now

@olivermrbl
Copy link
Contributor

olivermrbl commented Feb 6, 2024

As discussed previously, I think we should go with making all methods that interface with 3rd party providers singular and handle the bulk operations (and the errors that may occur) separately in the API or workflows. This should mitigate the risk of unexpected behavior or an inconsistent state of payments between systems. That way, we can handle an issue with one payment operation in isolation from the others instead of having to deal with scenarios like reconciling payments because 4 captures succeeded in Stripe but none in Medusa.

To illustrate, this would look something like this:

Payment Module

async capturePayment(paymentId) {
  const payment = await this.retrievePayment(paymentId)
  const provider = await this.retrieveProvider(payment.provider_id)

  await provider.capturePayment(payment)

  return await paymentService.update({ captured_at: new Date() })	
}

API

// POST /admin/payment-collection/:id/capture
const route = (req, res) => {
  const workflow = capturePaymentsWorkflow(req.scope)

  const paymentCollection = await paymentModule.retrieve(req.query.id)

  const result = await workflow.run({ input: paymentCollection }, { throwOnError: false })

  // handle errors … e.g. one out of 5 captures might have succeeded
}

Or alternatively, just do five requests to a single API endpoint

// POST /admin/payments/:id/capture
const route = (req, res) => {
  const workflow = capturePaymentWorkflow(req.scope)

  const payment = await paymentModule.retrievePayment(req.query.id)

  const result = workflow.run({ input: p })

  // handle errors
}

Aside from making it less error-prone, it would also simply the Payment Module business logic quite a bit.

cc @adrien2p @carlos-r-l-rodrigues

@fPolic
Copy link
Contributor Author

fPolic commented Feb 6, 2024

@olivermrbl I agree! Converting this temporarily to a draft while I work on a refactor.

@fPolic fPolic marked this pull request as draft February 6, 2024 16:12
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, think this is good to merge now. I imagine things will change a bit when we integrate Stripe and the BigNumber class, so let's push forward 💪

@olivermrbl
Copy link
Contributor

@adrien2p or @carlos-r-l-rodrigues – do any of you want to go through this before we merge?

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

Successfully merging this pull request may close these issues.

3 participants