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 GiftMessageGraphQl add coverage for cart #27956

Merged
merged 8 commits into from
Jun 5, 2020

Conversation

Usik2203
Copy link
Contributor

@Usik2203 Usik2203 commented Apr 23, 2020

Description (*)

This PR extends the existing GraphQl coverage by adding information about gift message to different types of cart items.

Please note, this PR does not cover the entire gift wrapping functionality but resolvers for the gift message. The rest of the git wrapping functionality will be delivered with the others PR

Note: These types were not extended because these types are not declared

 
type GiftCardCartItem {
    gift_message: GiftMessage @doc(description: "The entered gift message for the cart item")
}

type SalesItemInterface {
    gift_wrapping: GiftWrapping @doc(description: "The selected gift wrapping for the order item")
    gift_message: GiftMessage @doc(description: "The entered gift message for the order item")
}

type SalesTotalsInterface {
    gift_options: GiftOptionsPrices @doc(description: "The list of prices for the selected gift options")
}

This PR was mentioned for fixing failed static test related with composer.json file
#253

Related Pull Requests

#28072
#28105

Fixed Issues (if relevant)

  1. GraphQL Checkout :: Add support for Gift Wrapping / Gift Message during checkout.

Manual testing scenarios (*)

You can use this query for manual testing. You should take maskedQuoteId from quote_id_mask table

{
    cart(cart_id: "maskedQuoteId") {
           gift_message {
               to
               from
               message
           }
      }
}

@m2-assistant
Copy link

m2-assistant bot commented Apr 23, 2020

Hi @Usik2203. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@ghost ghost added this to Pending Review in Pull Requests Dashboard Apr 23, 2020
@magento-engcom-team magento-engcom-team added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Apr 23, 2020
@rogyar rogyar self-assigned this Apr 23, 2020
@rogyar rogyar marked this pull request as draft April 23, 2020 17:25
@rogyar rogyar self-requested a review April 28, 2020 07:57
Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @Usik2203. Thank you for your collaboration. Please, check my comments, adjust the code style and you may proceed with the test coverage.

Thank you!

$giftCartMessage = $this->cartRepository->get($cart->getId());

return [
'to' => $giftCartMessage->getRecipient(),
Copy link
Contributor

Choose a reason for hiding this comment

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

The recipient, sender and message may have null value. In this case, we need to make sure that we have no exception because all 3 values are required (and have Sring type) in the schema. I would recommend using the null coalesce operator and return empty string in case of no value. i.e.

$giftCartMessage->getSender() ?? ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"magento/module-backend": "*",
"magento/module-catalog": "*",
"magento/module-checkout": "*",
"magento/module-customer": "*",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make sure that we need all these dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/**
* Class GiftMessageItem
*/
class GiftMessageItem implements ResolverInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should extend this PR with more functionality. Let's leave the gift messages for different cart item types in this PR and cover all other functionality (mutations, gift wrapping) in the following PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed it from this PR, will do it in other PR

@ghost ghost moved this from Pending Review to Changes Requested in Pull Requests Dashboard Apr 28, 2020
@rogyar rogyar added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Apr 28, 2020
@Usik2203
Copy link
Contributor Author

@rogyar api-functional test was added for this PR.
Thank you for review.

@rogyar rogyar added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels May 4, 2020
@rogyar rogyar marked this pull request as ready for review May 4, 2020 06:53
@rogyar rogyar changed the title [WIP] GraphQL Checkout Add support for Gift Message during checkout GraphQL Checkout Add support for Gift Message during checkout May 4, 2020
@Usik2203
Copy link
Contributor Author

@magento run Static Tests

@Usik2203
Copy link
Contributor Author

@magento run Magento Health Index

@Usik2203
Copy link
Contributor Author

@magento run Static Tests

1 similar comment
@Usik2203
Copy link
Contributor Author

@magento run Static Tests

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-7587 has been created to process this Pull Request

@paliarush
Copy link
Contributor

Verified that no new @api or interfaces were introduced in scope of this PR, so approval by the architecture team is not required.

@engcom-Bravo engcom-Bravo self-assigned this Jun 2, 2020
@engcom-Bravo engcom-Bravo moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Jun 2, 2020
@engcom-Bravo
Copy link
Contributor

✔️ QA Passed

The GraphQL query

query {
  cart(
    cart_id: "8T3x445Ys2Scga6yWHAGVP9GYUPR8ua2"
  ) {
    gift_message {
      from
      message
      to
    }
  }
}

returns

{
  "data": {
    "cart": {
      "gift_message": {
        "from": "From",
        "message": "Message",
        "to": "To"
      }
    }
  }
}

@engcom-Bravo engcom-Bravo moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Jun 2, 2020
@engcom-Kilo engcom-Kilo self-assigned this Jun 2, 2020
@lenaorobei lenaorobei moved this from Extended Testing (optional) to Merge in Progress in Pull Requests Dashboard Jun 2, 2020
@slavvka slavvka added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Jun 5, 2020
@slavvka slavvka added this to the 2.4.1 milestone Jun 5, 2020
@magento-engcom-team magento-engcom-team merged commit ee50e37 into magento:2.4-develop Jun 5, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jun 5, 2020

Hi @Usik2203, 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.

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Jun 5, 2020
@lenaorobei lenaorobei added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: category of expertise Award: test coverage Component: GiftMessageGraphQl Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner Priority: P3 May be fixed according to the position in the backlog. Progress: accept Project: GraphQL QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
Pull Requests Dashboard
  
Recently Merged
Development

Successfully merging this pull request may close these issues.

GraphQL Checkout :: Add support for Gift Wrapping / Gift Message during checkout.