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

Use no-meteor functions for payment capture and refund methods #4803

Merged

Conversation

jamesporl
Copy link
Contributor

Impact: major
Type: refactor

Issue

Payment capture and refund are currently done by creating Meteor methods named using processorName + methodName within a payment plugin . As we intend to minimize using Meteor, we want to use no-meteor functions instead by registering them into the functionsByType parameter in a plugin's register.js file.

Solution

Instead of creating processor/method-name methods, payment plugins will need to register no-meteor functions in the register.js using the functionsByType parameter. In particular, payment plugins need to create the following functions:

${processor}CapturePayment
${processor}RefundPayment
${processor}ListRefund
${processor}DeAuthorizePayment

Breaking changes

Payment plugins that use Meteor methods for capture and refund will not be compatible with this PR.

Testing

As admin, enable example payment method in the admin dashboard.
As customer, create an order and pay with the example payment method (this can be done in the classic UI).
As admin, open the newly created order in the dashboard panel for Orders.
Click Approve, and Capture. See that Captured Total is updated.
In the same order, enter a refund amount and click Refund. Check that the order in the database has additional success transactions.

2
As admin, enable Stripe payment method in the admin dashboard and enter correct credentials.
As customer, create an order and pay with Stripe payment method (this can be done in the classic UI).
As admin, open the newly created order in the dashboard panel for Orders.
Click Approve, and Capture. See that Captured Total is updated.
In the same order, enter a refund amount and click Refund. See that the refunded amounts are displayed in the order.
For Stripe, you can verify the amounts captured and refunded in the Stripe merchant dashboard.

@brent-hoover
Copy link
Collaborator

brent-hoover commented Nov 14, 2018

@ajporlante I don't understand this part

need to create the following functions:

${processor}CapturePayment
${processor}RefundPayment
${processor}ListRefund
${processor}DeAuthorizePayment

Why do we care about what the methods are called? Couldn't they just be in a nested object that defines capture/refund/list etc. under a key for the payment provider? I would like to get away from using the names as keys because this has always brittle and causes problems with payment providers who have non-javascript safe characters in their names

@brent-hoover brent-hoover changed the base branch from master to release-2.0.0-rc.7 November 14, 2018 22:10
@jamesporl
Copy link
Contributor Author

@aldeed @zenweasel Reworked it as you requested. You can check, then I can proceed with removing the Meteor method files and rewriting the tests.

@aldeed
Copy link
Contributor

aldeed commented Nov 15, 2018

@ajporlante See my one minor comment but otherwise looking beautiful

@jamesporl jamesporl changed the title [WIP] Use no-meteor functions for payment capture and refund methods Use no-meteor functions for payment capture and refund methods Nov 19, 2018
@jamesporl
Copy link
Contributor Author

@zenweasel This is ready for another review. I have reworked as @aldeed suggested, removed the unnecessary Meteor methods and tests, and rewrote the unit tests for the new no-meteor functions.

@brent-hoover brent-hoover self-requested a review November 20, 2018 02:01
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Tested. Verified working. Great work. 🎉

@aldeed
Copy link
Contributor

aldeed commented Nov 20, 2018

@ajporlante Looks like one eslint error to resolve before we can merge this. https://circleci.com/gh/reactioncommerce/reaction/32460?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

@jamesporl
Copy link
Contributor Author

@aldeed Fixed.

@aldeed aldeed merged commit 1f6b5b3 into release-2.0.0-rc.7 Nov 26, 2018
@aldeed aldeed deleted the refactor-ajporlante-use-no-meteor-payment-methods branch November 26, 2018 18:41
@spencern spencern mentioned this pull request Jan 8, 2019
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.

3 participants