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

Rewrite placeOrder and support multiple payments for an order #4908

Merged
merged 53 commits into from
Jan 18, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Jan 9, 2019

Resolves #4860
Impact: breaking
Type: feature

Changes

The primary change is that when placing an order, you can now pay with multiple payment methods. A variety of other changes needed to be made as a result.

MongoDB schema changes:

  • There is no longer a payment property on each order fulfillment group. Rather there is a payments array on the order itself, and payments are not associated with any particular fulfillment group.
  • currencyCode property is added to invoice for use by that resolver
  • captureErrorCode and captureErrorMessage properties are added to payment so that the operator UI can display capture error messages
  • invoice was previously on each fulfillment group and also on each payment. It's no longer on payment because payments no longer correspond to just one group.
  • removed payment.data.billingAddress because it's already on payment.address

GraphQL schema changes:

  • A new placeOrder GraphQL mutation supports placing an order with multiple payments for it.
  • _id is now optional on Address type
  • shippingAddress and shop fields added to FulfillmentGroup type
  • payments field added to Order type
  • status field is added to Payment type and is defined by new PaymentStatus enum
  • [BREAKING] placeOrderWithExampleIOUPayment mutation is removed
  • [BREAKING] placeOrderWithStripeCardPayment mutation is removed
  • [BREAKING] Created captureOrderPayments mutation to replace the "orders/capturePayments" Meteor method. It has a similar API but allows you to pass an array of payment IDs to capture only certain payments.
  • Moved some IOU Example types from core payments schema to the included plugin's schema

Meteor method changes:

  • [BREAKING] orders/approvePayment method signature changed from (order) to (orderId, paymentId)

Refactoring:

  • registration of payment methods is now handled within the payments plugin rather than in the core plugin's startup code.
  • The "Currency exchange rates..." message that appears a bunch of times each time the operator UI loads a catalog listing page is change from "warn" level to "debug" level
  • [BREAKING] The createOrder internal mutation function is removed
  • Duplicate stripe functions into the marketplace plugin. That plugin should not require the stripe plugin.
  • Improved the Stripe payment capture function to better handle errors. The "charge_already_captured" will now mark the charge as captured in the system rather than resulting in an error, and other errors appear in the operator UI so that you know why the capture didn't work.

Operator UI:

  • The payment method settings for "supports" (capture, de-auth, etc.) are no longer respected and the check boxes have been removed from the operator UI
  • The order summary view is rearranged to accommodate showing multiple payments and to keep similar information together.
    • Created a new OrderPayment React component
    • Order cancel button remains with the invoice UI, but approving, capturing, and refunding UI now appears once for each payment. Canceling the order still refunds all of the captured payments.
    • Some of the order info that was at the top has been moved to the shipping or payment sections based on what it is related to.
    • The Print button is now always visible and isn't coupled with payment buttons.

Bug fixes:

  • One appEvent consumer throwing an error no longer prevents the remaining consumers from running
  • Fixed the permission check in "orders/refund/list" Meteor method

Breaking changes

All of the individual placeOrder* GraphQL mutations provided by the built-in payment plugins are removed and replaced with a single placeOrder mutation that supports multiple payments. The createOrder internal mutation that these used is also gone, so this will break any custom payment method plugins.

See also any with BREAKING above.

Testing

It's easiest to test this along with the starter kit changes. See the testing instructions in reactioncommerce/example-storefront#477

Switch from placeOrderWithStripeCardPayment mutation
to placeOrder mutation
to captureOrderPayments GraphQL mutation
And use captureOrderPayments GraphQL mutation
supported methods" check boxes
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

Order emails only display one paymenr, they should display all payments (this order had IOU and Credit Card on it):

airmail

@@ -97,7 +98,8 @@ class OrderTable extends Component {

renderOrderInfo(order) {
const { displayMedia, moment } = this.props;
const { invoice } = getPaymentForCurrentShop(order);
const [payment] = order.payments || [];
const { amount } = payment || {};
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how this is working to me? I think it's just a syntax I haven't seen before, but if this is a payment in an array, how are we extracting amount directly from payment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const [payment] takes the first payment and saves it in the payment const (array destructuring). Then I destructure the amount property off payment.

Ideally we shouldn't be using only the first payment anywhere, but I was trying to avoid doing too many changes in this one PR.

@spencern spencern modified the milestones: 🏔 Shavano, 🏔 Torreys Jan 15, 2019
@aldeed
Copy link
Contributor Author

aldeed commented Jan 15, 2019

Migrations 24 and 32 both throw this error on startup

That is something unrelated that I fixed in RC8 branch. I'll merge the fix into develop and this branch.

Order cancellation is not available until all payments are approved and captured.

I thought this was always the case but maybe not. Either way, what you say makes sense. I should be able to update the cancellation method to skip refunding if the payment isn't captured yet.

Order emails only display one payment

Good catch. I'll fix.

Inventory is not being correctly tracked

I'll investigate

@kieckhafer
Copy link
Member

I thought this was always the case but maybe not. Either way, what you say makes sense. I should be able to update the cancellation method to skip refunding if the payment isn't captured yet.

The cancellation button was hidden - you needed to click the down arrow next to the Approve button, but it was still available before the approval.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 15, 2019

The cancellation button was hidden

Right, but I seem to remember it didn't work for me until after capturing. It just did nothing when I clicked it.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 16, 2019

@kieckhafer All of the bugs you found should be fixed now.

@dancastellon I fixed sending the order emails when payments don't have a billing address, and I added a way for payment methods to indicate they don't support refunding. Add canRefund: false in the paymentMethods object in your register.js. The refund UI then will not appear. I think this was everything you had mentioned to me.

Copy link
Contributor

@dancastellon dancastellon left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good to me. I did a quick bit of testing with my rewards payment method prototype and it still works.

…order-rewrite

# Conflicts:
#	imports/plugins/core/versions/server/migrations/index.js
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

On first start up, I'm seeing this message:

Exception from sub OrderImages id 9pSNCnMxnwNZXcecT TypeError: Cannot read property 'shipping' of undefined
    at Subscription.Meteor.publish.orderId [as _handler] (imports/plugins/core/core/server/publications/collections/orders.js:223:28)
    at currentArgumentChecker.withValue (packages/check/match.js:118:15)
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1304:12)
    at Object._failIfArgumentsAreNotAllChecked (packages/check/match.js:116:43)
    at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1764:18)
    at DDP._CurrentPublicationInvocation.withValue (packages/ddp-server/livedata_server.js:1043:15)
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1304:12)
    at Subscription._runHandler (packages/ddp-server/livedata_server.js:1041:51)
    at Session._startSubscription (packages/ddp-server/livedata_server.js:859:9)
    at Session.sub (packages/ddp-server/livedata_server.js:625:12)
    at packages/ddp-server/livedata_server.js:559:43

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

All previously mentioned issues have been fixed.

Two new things:

  • The error message above on startup, I have not been able to reproduce though, I only saw it one time.

  • When I cancel an order via the operator UI, I get a rounding issue with the total (just a UI issue):
    basic_reaction_product

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

All changes look good! Merging it! 👍

@kieckhafer kieckhafer merged commit 9f4bc33 into develop Jan 18, 2019
@kieckhafer kieckhafer deleted the feat-aldeed-place-order-rewrite branch January 18, 2019 05:22
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.

4 participants