Skip to content

GraphQL cart error messages #291

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

GraphQL cart error messages #291

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions design-documents/graph-ql/coverage/Cart.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ type Cart {

is_multishipping: Boolean!
is_virtual: Boolean!

errors: [CartError]
}

type CheckoutCustomer {
Expand Down Expand Up @@ -57,3 +59,8 @@ type CheckoutPaymentMethod {
type CartGiftCard {
code: String!
}

type CartError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain in proposal why it is not possible to use built-in mechanism for GraphQL errors along with \Magento\Framework\Validation\ValidationException::getErrors

Copy link
Member

Choose a reason for hiding this comment

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

The main reason that resolvers have conditions to prevent the execution of add product to cart and show items in the cart(example app/code/Magento/QuoteGraphQl/Model/Cart/AddProductsToCart.php). That behavior difference with frontend where the customer could see products in cart with messages on frontend but in graphql query he got the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain in the proposal why we should ignore other message types like success, notice, warning

Copy link
Member

Choose a reason for hiding this comment

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

In base PR magento/graphql-ce#475 we expected all kind of messages. Looks like we need rename CartError to CartMessages.

identifier: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide a use case when identifier is needed on storefront.

It seems like it should not be exposed to storefront. If so than this CartError type is excessive and its usages can be replaced with String

Copy link
Member

Choose a reason for hiding this comment

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

The main reason to add the identifier is localizing part of process where errors were thrown. It should fetch profit for developers who extend base functionality and implements new features.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
identifier: String
identifier: String!

text: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
text: String
text: String!

}