-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
[GraphQl] Single mutation for adding different types of products to the shopping cart #27914
[GraphQl] Single mutation for adding different types of products to the shopping cart #27914
Conversation
Hi @rogyar. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphQL schema looks good and corresponds to the design https://github.com/magento/architecture/blob/master/design-documents/graph-ql/coverage/add-items-to-cart-single-mutation.md
@paliarush please confirm interfaces comply with requirements and standards. |
} | ||
|
||
if (is_string($result)) { | ||
$errors = array_unique(explode("\n", $result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For complex errors we should be able utilize composite GraphQlInputException
. Please see example \Magento\QuoteGraphQl\Model\Cart\AddSimpleProductToCart::execute
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lenaorobei. Thank you for your suggestion. In case of GraphQl modules, this approach will work. However, in this particular case, we work with the Quote
module that has no dependency to GraphQl modules.
We use a similar approach in the Reorder
functionality
if (is_string($addProductResult)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lenaorobei this ugly approach comes from \Magento\Quote\Model\Quote::addProduct and due to we need to provide error message(s) we need to "parse" it in that way, as was "designed" in Quote::addProduct
/**
* Error message
*/
if (is_string($cartCandidates) || $cartCandidates instanceof \Magento\Framework\Phrase) {
return (string)$cartCandidates;
}
/** @var AddProductsToCartOutput $addProductsToCartOutput */ | ||
$addProductsToCartOutput = $this->addProductsToCart->execute($maskedCartId, $cartItems); | ||
|
||
return [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With mentioned exception we should be able to return both - cart data and errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rogyar :
Below is a list of few of the use cases that are failing. Please take a look.
Case 1: add product to cart with a qty> the stock available
Actual result:
"errors": {
"debugMessage": "Expected a value of type \"CheckoutUserInputErrorCodes\" but received: (empty string)","message": "Internal server error",
Case 2 : Add product with qty=0 and a valid sku,
Expected: cart is empty and error message to enter a quantity >0
Actual: The product is still added to the cart with qty =1
Case 3: Add disabled product to cart
Expected: Cart is empty and message that "product is not available"
Actual result :
exception: {
"errors": [
{
"debugMessage": "Expected a value of type \"CheckoutUserInputErrorCodes\" but received: (empty string)",
"message": "Internal server error",
Now, if we omit "code" from userInputError{} and run the same query, below exception is returned
exception: ""errors": [
{
"debugMessage": "User Error: expected iterable, but did not find one for field CheckoutUserInputError.path.","
Case 4 : Same result as above when you try to add a product with "Out of Stock" status.
015fcf3
to
b9e28fd
Compare
@magento run all tests |
@magento run all tests |
@magento run all tests |
@magento run all tests |
…agento2 into 257-single-mutation-add-to-cart
Hi @rogyar, thank you for your contribution! |
Description (*)
This PR introduces a new approach that allows adding different types of products using the same mutation.
More details will be provided soon ...
Solution Architecture
Related Pull Requests
https://github.com/magento/partners-magento2ee/pull/260
https://github.com/magento/partners-magento2-infrastructure/pull/17
Fixed Issues (if relevant)
Manual testing scenarios (*)
TBP
Questions or comments
The data providers for different product types were decoupled as separate modules according to the suggestion from @mslabko.
Contribution checklist (*)