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

GraphQL cart error messages #291

Closed
wants to merge 1 commit into from

Conversation

lenaorobei
Copy link
Contributor

Problem

Now in GraphQL when multiple errors occur with cart there is no way to read all cart messages in order to understand the whole context.
53808295-8c011b00-3f5a-11e9-9372-d84fb4464e89

Solution

@XxXgeoXxX proposes to add additional filed to the Cart type magento/graphql-ce#475
Names changed by me but, PR will be adjusted once approved here.

Requested Reviewers

@paliarush

@paliarush paliarush self-assigned this Sep 25, 2019
@@ -57,3 +59,8 @@ type CheckoutPaymentMethod {
type CartGiftCard {
code: String!
}

type CartError {
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.

@@ -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.

@@ -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 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.


type CartError {
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!

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

type CartError {
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.

Suggested change
identifier: String
identifier: String!

@lenaorobei
Copy link
Contributor Author

@paliarush @XxXgeoXxX we have an alternative proposal here magento/graphql-ce#970

It will look like this:

{
  "errors": [
    {
      "message": "First error.",
      "extensions": {
        "category": "graphql"
      },
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ]
    },
    {
      "message": "Second error.",
      "extensions": {
        "category": "graphql"
      },
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ]
    }
  ]
}

Please let me know what do you think.

@lenaorobei
Copy link
Contributor Author

Closing in favor of magento/graphql-ce#970

@lenaorobei lenaorobei closed this Oct 15, 2019
@paliarush
Copy link
Contributor

@lenaorobei it looks to me that there is a bug in the native error handling functionality which should be fixed. We should not introduce custom error messages.

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.

3 participants