Feature/graphql wishlist remove item

LucasCalazans commented 5 years ago

Description

Adding a new mutation to the WishlistGraphQl to remove multiple items

Fixed Issues

849: [Wish List] Remove products from the Wish List

Manual testing scenarios

Use the new mutation to remove a previously added item from your wishlist passing the products ids (It can be one or many)

mutation wishlistRemoveItem($wishlist_id: ID!, $wishlist_items_ids: [ID!]!) {
removeProductsFromWishlist(input: {
wishlist_id: $wishlist_id
wishlist_items_ids: $wishlist_items_ids
}) {
wishlist {
  id
  items {
    id
    product {
      id
      name
    }   
  }
  items_count
  sharing_code
  updated_at
}
}
}

Contribution checklist

[x] Pull request has a meaningful description of its purpose
[x] All commits are accompanied by meaningful commit messages
[ ] All new or changed code is covered with unit/integration tests (if applicable)
[ ] All automated tests passed successfully (all builds are green)

LucasCalazans commented 5 years ago

It's just missing the API functional tests. I'll try to do that this weekend.

lenaorobei commented 5 years ago

@magento run all tests

TomashKhamlai commented 5 years ago

Steps to reproduce:

Install Magento.
Don't register customer
Don't create any products
Don't add any items to wishlist

Send request:

Request:

mutation wishlistRemoveItem(
$wishlist_id: ID!
$wishlist_items_ids: [ID!]!
) {
removeProductsFromWishlist(
input: {
  wishlist_id: $wishlist_id
  wishlist_items_ids: $wishlist_items_ids
}
) {
wishlist {
  id
  items {
    id
    product {
      id
      name
    }
  }
  items_count
  sharing_code
  updated_at
}
}
}

Variables

{
"wishlist_id": "1",
"wishlist_items_ids": ["2", "3"]
}

Expected result:

Some of the errors:

Customer is not authorized to perform operations on wishlist with id=1

No such entity exception

Actual result:

{
"data": {
"removeProductsFromWishlist": {
"wishlist": {
"id": null,
"items": [],
"items_count": 0,
"sharing_code": null,
"updated_at": null
}
}
}
}

LucasCalazans commented 5 years ago

@TomashKhamlai Hey, thank you for your review. I've made some fixies, can you check again, please?

TomashKhamlai commented 5 years ago

@LucasCalazans, please pull the latest changes on 2.3-develop, merge them into your branch and push the changes. I am blocked by:

main.CRITICAL: Class Magento\Catalog\Model\Product\Attribute\Source\LayoutUpdate does not exist {"report_id":"da73692f7a69afb4559ae3ee4b072292ba89f137f0cf4b249c23fdb39a0902a3","exception":"[object] (ReflectionException(code: -1): Class Magento\\Catalog\\Model\\Product\\Attribute\\Source\\LayoutUpdate does not exist at /var/www/html/graphqlce/lib/internal/Magento/Framework/Code/Reader/ClassReader.php:26)"} []
main.CRITICAL: Unable to serialize value. Error: Malformed UTF-8 characters, possibly incorrectly encoded {"exception":"[object] (InvalidArgumentException(code: 0): Unable to serialize value. Error: Malformed UTF-8 characters, possibly incorrectly encoded at /var/www/html/graphqlce/lib/internal/Magento/Framework/Serialize/Serializer/Json.php:26)"} []

Merging the latest mainline is a known and verified troubleshooting step for the problem with missing LayoutUpdate.php file

TomashKhamlai commented 5 years ago

There is another problem:

Steps to reproduce:

Create customer
Don't create products so there is nothing to add to wishlist
Generate Customer Token
Use query customer to get wishlist's id

Send a request with valid data(use any integer numbers that are greater than 0 for wishlist_items_ids):

Request:

mutation wishlistRemoveItem(
$wishlist_id: ID!
$wishlist_items_ids: [ID!]!
) {
removeProductsFromWishlist(
input: {
  wishlist_id: $wishlist_id
  wishlist_items_ids: $wishlist_items_ids
}
) {
wishlist {
  id
  items {
    id
    product {
      id
      name
    }
  }
  items_count
  sharing_code
  updated_at
}
}
}

Variables

{
"wishlist_id": "< valid_id >",
"wishlist_items_ids": ["2", "3"]
}

Variables

{
"Authorization": "Bearer < valid_token >"
}

Open another tab and generate the customer's token using the wrong password 10 times in a row.
Repeat step 5.

Expected result:

The locked customer is not able to perform this request and is notified with some message about it.

Actual result:

No difference between the locked and active customer

{
  "data": {
    "removeProductsFromWishlist": {
      "wishlist": {
        "id": "6",
        "items": [],
        "items_count": 0,
        "sharing_code": "yEbOzoRaadxxIn5YlofKYSUkUxufVGKD",
        "updated_at": "2019-11-15 17:33:28"
      }
    }
  }
}

TomashKhamlai commented 5 years ago

@chalov-anton, please continue testing this PR.

chalov-anton commented 5 years ago

Steps to reproduce:

Install Magento.

Don't register customer

Don't create any products

Don't add any items to wishlist

Send request:

Request:
mutation wishlistRemoveItem(
  $wishlist_id: ID!
  $wishlist_items_ids: [ID!]!
) {
  removeProductsFromWishlist(
    input: {
      wishlist_id: $wishlist_id
      wishlist_items_ids: $wishlist_items_ids
    }
  ) {
    wishlist {
      id
      items {
        id
        product {
          id
          name
        }
      }
      items_count
      sharing_code
      updated_at
    }
  }
}
Variables
{
  "wishlist_id": "1",
  "wishlist_items_ids": ["2", "3"]
}
Expected result:

Some of the errors:

Customer is not authorized to perform operations on wishlist with id=1

No such entity exception

Actual result:
{
  "data": {
    "removeProductsFromWishlist": {
      "wishlist": {
        "id": null,
        "items": [],
        "items_count": 0,
        "sharing_code": null,
        "updated_at": null
      }
    }
  }
}

If the Customer is not Authorized, for example the Authorization token is wrong(or not provided), an error message appears after performing mutation :thumbsup:

{
  "errors": [
    {
      "message": "The current user cannot perform operations on wishlist",
      "extensions": {
        "category": "graphql-authorization"
      },
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "removeProductsFromWishlist"
      ]
    }
  ],
  "data": {
    "removeProductsFromWishlist": null
  }
}

But still, if the customer is authorized and trying to perform operations on empty wishlists - a "success" message returns

{
  "data": {
    "removeProductsFromWishlist": {
      "wishlist": {
        "id": "1",
        "items": [],
        "items_count": 0,
        "sharing_code": "4zNIfYqgRRxpzTnDWEP2ZoPKmwbSuy15",
        "updated_at": "2019-11-18 08:10:16"
      }
    }
  }
}

Expected error message like There is no %s item in wishlist.

LucasCalazans commented 5 years ago

@chalov-anton I thought that it should return everything and ignore the ids.

Okay, when the user passes 2 ids. When the first exists and another one doesn't. What should I do in this case? Show an error or return the success object?

lenaorobei commented 5 years ago

@LucasCalazans the general rule for GraphQL functionality test coverage is to use api-functional tests. You can find some examples here /dev/tests/api-functional/testsuite/Magento/GraphQl. Unit tests do not cover actual use case, so please consider removing them and write api-functional tests. Thanks.

LucasCalazans commented 5 years ago

@lenaorobei I'll need more time to create the api-functional tests because I have never used it.

lenaorobei commented 5 years ago

@LucasCalazans please let me know if you need any help with this. Docs: https://devdocs.magento.com/guides/v2.3/graphql/functional-testing.html Example: Magento\GraphQl\Quote\Guest\RemoveItemFromCartTest.

LucasCalazans commented 5 years ago

Okay, I'll try to do that today.

@lenaorobei there are some errors in the Semantic Version Checker and Unit Tests. Do you know why? Nothing in this PR was changed related to this.

lenaorobei commented 5 years ago

@LucasCalazans, looks like an issue on our side. Let's ignore for now, I'll look into it.

chalov-anton commented 5 years ago

There is another issue with this PR If perform the mutation with correct Authorization Header but with wrong Variables, for example:

{
  "wishlist_id": "non_existing",
  "wishlist_items_ids": ["non_existing"]
}

The response message is

{
  "errors": [
    {
      "message": "The current customer isn't authorized.",
      "extensions": {
        "category": "graphql-authorization"
      },
...

In this case, we would expect an error message about wrong variables or _Non-existing wishlistid

lenaorobei commented 4 years ago

Hi @LucasCalazans! Unfortunately we need to close this PR in order to not to increase tech debt with new storefront approach. Hope you'll join us in upcoming project.

ghost commented 4 years ago

Hi @LucasCalazans, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento / graphql-ce