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

Add moveOrderItems mutation #5018

Merged
merged 14 commits into from
Mar 14, 2019
Merged

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Mar 5, 2019

Part of #4999
Impact: minor
Type: feature

Changes

A new moveOrderItems mutation allows you to move one or more order items from one fulfillment group to another existing fulfillment group on the same order.

The operator UI does not yet implement this.

  • moveOrderItems can be called internally to synchronize from an external system by setting context.isInternalCall to true
  • moveOrderItems can be called through GraphQL by any user with "orders" permission for the shop that owns the order
  • moveOrderItems can be called through GraphQL by the user who placed the order. In this case, the move will only succeed if both the item status and the order status are "new".

Breaking changes

None

Testing

  1. Place an order.
  2. Verify that the mutation works as described.
  3. Verify that you can move order items when authenticated as a user with "orders" role or as the user who placed the order, but not when authenticated as someone else or when not authenticated.

@aldeed aldeed self-assigned this Mar 5, 2019
@aldeed aldeed added this to the 🏔 Uncompahgre milestone Mar 5, 2019
@aldeed aldeed requested a review from kieckhafer March 5, 2019 23:27
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.

When I move an item between fulfillmentGroups, the itemIds array on each fulfillmentGroup is not updated, it keeps the original itemIds.
robo_3t_-_1_1

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.

Not sure if this is done on purpose or not, but I cannot move ALL items from one fulfillment group to another, and "close" the fulfillment group. I get an error You must specify at least 1 values, which doesn't completely make sense, but I believe based on not allowing me to move a single remaining item to another fulfillmentGroup, it means that each fulfillmentGroup must have at least one item in it.

@aldeed
Copy link
Contributor Author

aldeed commented Mar 7, 2019

@kieckhafer I did intentionally want an error if you try to have no items in a group. It's something we could consider supporting in the future, but I think it leads to a lot of unanswered questions and potential confusion as far as what to do about group status, totals, etc.

I will fix the itemIds issue. Beyond that, I think there is a whole can of worms here that I hadn't realized until now. We probably need to recalculate totals after moving, or they won't make sense. The group totals will change, which means discounts, shipping, taxes, and surcharges could all change, too.

I think I'll need to pull out the placeOrder calculations to a util function, so we can redo them as the order changes. (The payments will then not necessarily match the total, but that can be handled by refunds or adding additional payments, something we'll add support for in a future PR.)

@aldeed aldeed requested a review from kieckhafer March 12, 2019 21:05
@aldeed
Copy link
Contributor Author

aldeed commented Mar 12, 2019

@kieckhafer This should be ready for another look

Signed-off-by: Erik Kieckhafer <[email protected]>
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.

I'm having issues with what seems like Taxes not being calculated / added into the total, causing an mismatch error in total where I can't check out:

Checkout___Reaction

This mutation works when taxes aren't active, so it's possible this was something overlooked during my testing in #5027 when all those files were moved around, and not necessarily the fault of this PR.

after select fulfillment option

Signed-off-by: Eric Dobbertin <[email protected]>
@aldeed aldeed requested a review from kieckhafer March 14, 2019 22:30
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.

👍 good to go with updates

@aldeed aldeed merged commit bfd6c69 into develop Mar 14, 2019
@aldeed aldeed deleted the feat-aldeed-move-order-items-mutation branch March 14, 2019 22:55
@jeffcorpuz jeffcorpuz mentioned this pull request Mar 19, 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.

2 participants