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 updateOrder mutation #5019

Merged
merged 5 commits into from
Mar 7, 2019
Merged

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Mar 6, 2019

Part of #4999
Impact: minor
Type: feature

Changes

A new updateOrder mutation allows you to change the email, status, or customFields of an order after it has been placed. (Saving customFields is not enabled by default. See https://docs.reactioncommerce.com/docs/how-to-store-custom-order-fields)

The operator UI does not yet implement this.

  • updateOrder can be called internally to synchronize from an external system by setting context.isInternalCall to true
  • updateOrder can be called through GraphQL by any user with "orders" permission for the shop that owns the order

Breaking changes

None

Testing

Note: Do not worry about testing the customFields update since it requires customization to opt in.

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

@aldeed aldeed self-assigned this Mar 6, 2019
@aldeed aldeed added this to the 🏔 Uncompahgre milestone Mar 6, 2019
@aldeed aldeed requested a review from kieckhafer March 6, 2019 02:06
aldeed and others added 4 commits March 6, 2019 14:56
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer force-pushed the feat-aldeed-update-order-mutation branch from e564101 to d961a13 Compare March 6, 2019 22:57
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.

Made two commits with what I think are "fixes" to some commenting, looks like it just wasn't updated from being copied over from the move mutation, but please confirm my changes are OK.

Also fixed conflicts in a few files, most were straightforward but inside imports/plugins/core/orders/server/no-meteor/schemas/schema.graphql just want to make sure it looks ok.

Also a couple questions...

* @param {Object} parentResult - unused
* @param {Object} args.input - an object of all mutation arguments that were sent by the client
* @param {Object} [args.input.customFields] - Updated custom fields
* @param {String} [args.input.email] - Set this as the order email
Copy link
Member

Choose a reason for hiding this comment

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

Should any of these, aside from customFields, be inside an array in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the [] around them? That means optional in jsdoc and they are in fact all optional except orderId. Ideally you would provide as least one thing to update, but if you don't, I just have it return the un-updated order without throwing an error.

Copy link
Member

Choose a reason for hiding this comment

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

Got it... Yes that is what I was referring to.

@@ -0,0 +1,12 @@
mutation ($email: String, $orderId: ID!, $status: String) {
Copy link
Member

Choose a reason for hiding this comment

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

Does customFields need to be added in the mutation here, or is that going to ben an extension in its' own package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will need to be added if used because we don't know what the schema should be.

@@ -401,6 +406,30 @@ input PlaceOrderInput {
payments: [PaymentInput]
}

"Input for the updateOrder mutation"
input UpdateOrderInput {
Copy link
Member

Choose a reason for hiding this comment

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

Does customFields need to be added in the mutation here, or is that going to ben an extension in its' own package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will need to be added if used because we don't know what the schema should be.

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.

Do you think there should be some sort of list somewhere that is an array of allowed workflow statuses? Maybe something that goes along with the orderStatusLabels that I was working on.

I see the argument that it's on the user / the client / somewhere else, outside the API, to be checking this data, but I can add anything I want to the workflow and there isn't really any consequence to it:

banners_and_alerts_and_robo_3t_-_1_1

@kieckhafer
Copy link
Member

Works as described.

Can update order only from orders permission user. Cannot update even by the user that placed the order.

A few comments, but overall good to go.

…-order-mutation

# Conflicts:
#	imports/plugins/core/orders/server/no-meteor/mutations/index.js
#	imports/plugins/core/orders/server/no-meteor/resolvers/Mutation/index.js
#	imports/plugins/core/orders/server/no-meteor/schemas/schema.graphql
@aldeed
Copy link
Contributor Author

aldeed commented Mar 6, 2019

@kieckhafer I do think eventually we should check against a status list, but we'll soon have to do some work to make that list configurable. I thought about adding a check now with a hard-coded array but it seems low risk to leave it wide open for now, and I don't know for sure that people aren't already storing custom statuses that would not be valid according to our list.

@aldeed
Copy link
Contributor Author

aldeed commented Mar 7, 2019

Also, @kieckhafer all of your commits and conflict fixes LGTM. Thanks!

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.

Added a comment in #5000, which deals with the translations UI, about adding a list of allowed statues. Looks good to go after your comments 👍

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