GraphQL mutations refactoring: input types, cart mutations & multi-cart preparation

[pozylon]

This PR contains massive name refactorings / api surface changes.

Removed (BREAKING):

• captureCart (has never been implemented)

Renamed (BREAKING):

• checkout to checkoutCart
• input type FilterQuery to FilterQueryInput
• input type ProductConfigurationParameter to ProductConfigurationParameterInput
• input type HashedPassword to HashedPasswordInput
• updateOrder to updateCart
• Order.address to Order.billingAddress
• Parameter address to billingAddress in updateCart

New order manipulating mutation parameters in preparation of "multi-cart":

• updateCart: new optional orderId
• checkoutCart: new optional orderId
• addCartProduct: new optional orderId
• addCartDiscount: new optional orderId

New convenience mutations:

• emptyCart(orderId: ID)

That way, a very simple checkout is possible without fetching the orderId once in one big query but the full feature set is only available when using the indirect methods.

[CLAassistant]

All committers have signed the CLA.

[macrozone]

i would be cautious with confusing orders with carts. That it is the same is just a technical detail of your implementation, but for a user, its fundamentally different. I can't just update the quantity of an order after it has been payed. so rename updateCartItemQuantity to updateOrderItemQuantity makes no sense to me. It should be updateCartItemQuantity

so i would suggest:

• updateCart* mutatons stays the same, but additionally take an optional cartId for multi carts. if no cartId is given, the default one is used
• emptyCart removes everything in a cart
• checkoutOrder should be checkoutCart. It transforms the cart to an order (and returns the order)

Also concerning mutations that actually should be able to update an order (because of backoffice-jobs):

• make it clear what they do and for who their are intended for. updateOrder for example is very generic. It should not be easy to just update stuff on an order outside of the normal flow. Maybe even prefix these functions:

mutations.manage.updateOrder

or also for product manage functions

mutations.manage.updateProduct

The api should focus first on a developer who implements the shop frontend and second on a developer implementing backoffice apps. Altough unchained has a good acl implementation, its hard to reason about what can be done by whom, just looking at the graphql api

[pozylon]

So let's review all the order/cart related mutations in this branch:

addCartProduct
addCartDiscount
updateCart
checkoutCart

addOrderProduct
updateOrderItemQuantity
removeOrderItem
addOrderDiscount
removeOrderDiscount
updateOrder
removeOrder
checkoutOrder
confirmOrder
payOrder
setOrderPaymentProvider
setOrderDeliveryProvider
updateOrderDeliveryShipping
updateOrderDeliveryPickUp
updateOrderPaymentCard
updateOrderPaymentInvoice
updateOrderPaymentPostfinance
updateOrderPaymentPaypal
updateOrderPaymentCrypto

Today, only confirmOrder, payOrder and removeOrder are actual methods that should be allowed when the order is not a cart anymore. Following your argumentation, we would stick with Cart as the name for all the other mutations and take an optional cartId for all mutations above that currently require an orderId, resulting in:

addCartProduct(cartId: ID, ...)
addCartDiscount(cartId: ID, ...)
updateCart(cartId: ID, ...)
checkoutCart(cartId: ID, ...)
updateCartItemQuantity
removeCartItem
removeCartDiscount
setCartPaymentProvider(cartId: ID, ...)
setCartDeliveryProvider(cartId: ID, ...)
updateCartDeliveryShipping
updateCartDeliveryPickUp
updateCartPaymentCard
updateCartPaymentInvoice
updateCartPaymentPostfinance
updateCartPaymentPaypal
updateCartPaymentCrypto

removeOrder
confirmOrder
payOrder

Currently the difference between updateOrder and updateCart is that cart mutations initiate a new cart when there is no cart available for the user/country combination. But for mutations like removeCartDiscount or removeCartItem initiating a new cart does not make that much sense.

prefixing with mutations.manage is not a good idea due to graphql/graphql-js#221 (comment)

[pozylon]

updateOrder is definitely missleading in the new naming

[pozylon]

The more I think about it I guess we should definetly make the user know that a cart is an open order. But the updateCart namings are much more elegant.

What about having something like:
addCartProduct(orderId: ID, ...)

I mean the GraphQL type that gets returned is an Order too, having that link is not only a technical implementation detail but also an important knowhow of the whole concept. Then they know that cart's _id stays persistent throughout the checkout for example. And I think it could be pretty straightforward to document that lifecycle.

[pozylon]

So I would go for this set of mutations:

addCartProduct(orderId: ID, ...)
addCartDiscount(orderId: ID, ...)
updateCart(orderId: ID, ...)
checkoutCart(orderId: ID, ...)
updateCartItemQuantity
removeCartItem
removeCartDiscount
setCartPaymentProvider(orderId: ID, ...)
setCartDeliveryProvider(orderId: ID, ...)
updateCartDeliveryShipping
updateCartDeliveryPickUp
updateCartPaymentCard
updateCartPaymentInvoice
updateCartPaymentPostfinance
updateCartPaymentPaypal
updateCartPaymentCrypto

removeOrder(orderId: ID!, ...)
confirmOrder(orderId: ID!, ...)
payOrder(orderId: ID!, ...)

Maybe we could argue that a payment provider and a delivery provider might change again even after you did the checkout, for example if you selected invoice as payment during checkout but after the consumer changes his mind and wants to pay directly with CC. In that case we would promote those functions and require explicit orderId's:

setOrderPaymentProvider(orderId: ID!, ...)
setOrderDeliveryProvider(orderId: ID!, ...)
updateOrderDeliveryShipping
updateOrderDeliveryPickUp
updateOrderPaymentCard
updateOrderPaymentInvoice
updateOrderPaymentPostfinance
updateOrderPaymentPaypal
updateOrderPaymentCrypto

[macrozone]

The more I think about it I guess we should definetly make the user know that a cart is an open order. But the updateCart namings are much more elegant.

What about having something like:
addCartProduct(orderId: ID, ...)

I mean the GraphQL type that gets returned is an Order too, having that link is not only a technical implementation detail but also an important knowhow of the whole concept. Then they know that cart's _id stays persistent throughout the checkout for example. And I think it could be pretty straightforward to document that lifecycle.

i agree on that, I also realized that as well. I think the mix of updateCartXXX and orderId makes sense after all, it makes you understand the conecpt better.

[macrozone]

prefixing with mutations.manage is not a good idea due to graphql/graphql-js#221 (comment)

yeah. Ok so maybe have a clear description on these methods?

[pozylon]

Thanks @macrozone for the review, please see the updated issue comment about the final schema changes.