-
Notifications
You must be signed in to change notification settings - Fork 154
Conversation
|
||
$customerId = $quote->getCustomerId(); | ||
|
||
if (!$customerId) { |
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.
Please add comments to explain allowed use cases. It might be not obvious why we have these checks.
* Service for checking that the shopping cart operations | ||
* are allowed for current user | ||
*/ | ||
interface CartMutationsAllowedInterface |
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.
All classes/interfaces should be named using nouns. Please consider making changes across the review. In this specific case something like Authorization\CartMutation::isAllowed
sounds better
$couponCode = $args['input']['coupon_code']; | ||
|
||
if (!$maskedQuoteId || !$couponCode) { | ||
throw new GraphQlInputException(__('Required parameter is missing')); |
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.
It is better to specify which parameter is missing (have two separate exceptions)
|
||
if (!$this->cartMutationsAllowed->execute($cartId)) { | ||
throw new GraphQlAuthorizationException( | ||
__('Operations with selected cart is not permitted for current user') |
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.
'cart is not permitted' => 'cart are not permitted'
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.
If you can include the cart ID, change exception text to
'The current user cannot perform operations on cart <cart_id>'
Otherwise, change the text to
The current user cannot perform operations on the selected cart'
try { | ||
$this->couponManagement->set($cartId, $couponCode); | ||
} catch (\Exception $exception) { | ||
throw new GraphQlInputException(__($exception->getMessage())); |
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.
We must not display exception messages from \Exception
to the clients, they may contain sensitive information. Please log the original exception message and display some generic one.
Please make sure to fix similar issues across the PR if any.
'code' => $couponCode | ||
]; | ||
|
||
$result = function () use ($data) { |
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.
How is it possible to get the full cart object if needed?
} | ||
|
||
type AppliedCoupon { | ||
# Wrapper allows for future extension of coupon info |
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.
We probably don't need to leave such comments in the schema.
try { | ||
$cartId = $this->maskedQuoteIdToQuoteId->execute($maskedQuoteId); | ||
} catch (NoSuchEntityException $exception) { | ||
throw new GraphQlNoSuchEntityException(__('No cart with provided ID found')); |
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.
If you can include the ID, then change the exception text to
'Could not find a cart with ID <ID_number>'
Otherwise, change exception text to 'Could not find a cart with the provided ID'
|
||
if (!$this->cartMutationsAllowed->execute($cartId)) { | ||
throw new GraphQlAuthorizationException( | ||
__('Operations with selected cart is not permitted for current user') |
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.
If you can include the cart ID, change exception text to
'The current user cannot perform operations on cart <cart_id>'
Otherwise, change the text to
The current user cannot perform operations on the selected cart'
{ | ||
$maskedCartId = $args['input']['cart_id']; | ||
|
||
if (!$maskedCartId) { |
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.
Apply the same edits to the exception text as with ApplyCouponToCart.php
# QuoteGraphQl | ||
|
||
**QuoteGraphQl** provides type and resolver information for the GraphQl module | ||
to generate quote (cart) information endpoints. Also provides endpoints for modifying a quote. |
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.
Change second sentence to
It also provides...
# See COPYING.txt for license details. | ||
|
||
type Mutation { | ||
createEmptyCart: String @resolver(class: "\\Magento\\QuoteGraphQl\\Model\\Resolver\\Cart\\CreateEmptyCart") @doc(description:"Creates empty shopping cart for guest or logged in user") |
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.
Change description to
Creates an empty shopping cart for a guest or a logged in user
use Magento\TestFramework\TestCase\GraphQlAbstract; | ||
|
||
/** | ||
* Test for empty cart creation mutation |
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.
Please fix the comment
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.
Great contribution! Please address several minor comments and we should be good to merge.
-- fix static tests
Description
The PR introduces logic for working with shopping cart coupons. It adds two operations
applyCouponToCart
andremoveCouponFromCart
.Also, within a scope of this PR an additional interface for checking cart operations permissions is added. By using this class we can restrict a customer cart from editing by a guest or another customer.
Please note, changes from #124 are present in this branch
Fixed Issues (if relevant)
Manual testing scenarios
Apply coupon
Remove coupon