Skip to content

[GRAPHQL] Made the attribute destination_cart_id for mergeCarts mutation not required. #30633

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

Merged
merged 4 commits into from
Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 74 additions & 16 deletions app/code/Magento/QuoteGraphQl/Model/Resolver/MergeCarts.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,24 @@

namespace Magento\QuoteGraphQl\Model\Resolver;

use Magento\Framework\App\ObjectManager;
use Magento\Framework\Exception\CouldNotSaveException;
use Magento\Framework\GraphQl\Config\Element\Field;
use Magento\Framework\GraphQl\Exception\GraphQlAuthorizationException;
use Magento\Framework\GraphQl\Exception\GraphQlInputException;
use Magento\Framework\GraphQl\Exception\GraphQlNoSuchEntityException;
use Magento\Framework\GraphQl\Query\ResolverInterface;
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;
use Magento\QuoteGraphQl\Model\Cart\GetCartForUser;
use Magento\Quote\Api\CartRepositoryInterface;
use Magento\GraphQl\Model\Query\ContextInterface;
use Magento\Framework\GraphQl\Exception\GraphQlAuthorizationException;
use Magento\Quote\Api\CartRepositoryInterface;
use Magento\Quote\Model\Cart\CustomerCartResolver;
use Magento\Quote\Model\QuoteIdToMaskedQuoteIdInterface;
use Magento\QuoteGraphQl\Model\Cart\GetCartForUser;

/**
* Merge Carts Resolver
*
* @SuppressWarnings(PHPMD.LongVariable)
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewbess why do we need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rest looks good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @prabhuram93
Because we have a long property in this class $quoteIdToMaskedQuoteId

*/
class MergeCarts implements ResolverInterface
{
Expand All @@ -31,44 +38,95 @@ class MergeCarts implements ResolverInterface
*/
private $cartRepository;

/**
* @var CustomerCartResolver
*/
private $customerCartResolver;

/**
* @var QuoteIdToMaskedQuoteIdInterface
*/
private $quoteIdToMaskedQuoteId;

/**
* @param GetCartForUser $getCartForUser
* @param CartRepositoryInterface $cartRepository
* @param CustomerCartResolver|null $customerCartResolver
* @param QuoteIdToMaskedQuoteIdInterface|null $quoteIdToMaskedQuoteId
*/
public function __construct(
GetCartForUser $getCartForUser,
CartRepositoryInterface $cartRepository
CartRepositoryInterface $cartRepository,
CustomerCartResolver $customerCartResolver = null,
QuoteIdToMaskedQuoteIdInterface $quoteIdToMaskedQuoteId = null
) {
$this->getCartForUser = $getCartForUser;
$this->cartRepository = $cartRepository;
$this->customerCartResolver = $customerCartResolver
?: ObjectManager::getInstance()->get(CustomerCartResolver::class);
$this->quoteIdToMaskedQuoteId = $quoteIdToMaskedQuoteId
?: ObjectManager::getInstance()->get(QuoteIdToMaskedQuoteIdInterface::class);
}

/**
* @inheritdoc
*/
public function resolve(Field $field, $context, ResolveInfo $info, array $value = null, array $args = null)
{
public function resolve(
Field $field,
$context,
ResolveInfo $info,
array $value = null,
array $args = null
) {
if (empty($args['source_cart_id'])) {
throw new GraphQlInputException(__('Required parameter "source_cart_id" is missing'));
}

if (empty($args['destination_cart_id'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion. If the destination_cart_id is not set, most likely we will have the "Undefined index" notice. We may check if it is set, and then check it is empty.

throw new GraphQlInputException(__('Required parameter "destination_cart_id" is missing'));
throw new GraphQlInputException(__(
'Required parameter "source_cart_id" is missing'
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
'Required parameter "source_cart_id" is missing'
'Required parameter "destination_cart_id" is missing'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rogyar
Thank you for your review.
This block of the code is needed to check the parameter source_cart_id
The function empty is suitable for this checking.
Please take a look at the result code by this link.

Copy link
Contributor

Choose a reason for hiding this comment

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

But you are checking for destination_cart_id above, or I miss something?

if (empty($args['destination_cart_id'])) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I got your point. Looked at the wrong line of code ;)

));
}

/** @var ContextInterface $context */
if (false === $context->getExtensionAttributes()->getIsCustomer()) {
throw new GraphQlAuthorizationException(__('The current customer isn\'t authorized.'));
throw new GraphQlAuthorizationException(__(
'The current customer isn\'t authorized.'
));
}
$currentUserId = $context->getUserId();

if (!isset($args['destination_cart_id'])) {
try {
$cart = $this->customerCartResolver->resolve($currentUserId);
} catch (CouldNotSaveException $exception) {
throw new GraphQlNoSuchEntityException(
__('Could not create empty cart for customer'),
$exception
);
}
$customerMaskedCartId = $this->quoteIdToMaskedQuoteId->execute(
(int) $cart->getId()
);
} else {
if (empty($args['destination_cart_id'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we already have this check at the very top of the resolver. I believe we may optimize these conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rogyar
Thank you for your review.
The block of code is needed for checking the parameter destination_cart_id.
We check if the parameter is missing then get the customer cart id from our logic, but this parameter cannot be empty. So, we throw an exception about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, wrong line of code reviewed previously

throw new GraphQlInputException(__(
'The parameter "destination_cart_id" cannot be empty'
));
}
}

$guestMaskedCartId = $args['source_cart_id'];
$customerMaskedCartId = $args['destination_cart_id'];
$customerMaskedCartId = $customerMaskedCartId ?? $args['destination_cart_id'];
Copy link
Contributor

Choose a reason for hiding this comment

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

There's magic in 7.4 that allows us to turn it into

Suggested change
$customerMaskedCartId = $customerMaskedCartId ?? $args['destination_cart_id'];
$customerMaskedCartId ??= $args['destination_cart_id'];

But let's leave it as it is to preserve compatibility with 7.3


$currentUserId = $context->getUserId();
$storeId = (int)$context->getExtensionAttributes()->getStore()->getId();
// passing customerId as null enforces source cart should always be a guestcart
$guestCart = $this->getCartForUser->execute($guestMaskedCartId, null, $storeId);
$customerCart = $this->getCartForUser->execute($customerMaskedCartId, $currentUserId, $storeId);
$guestCart = $this->getCartForUser->execute(
$guestMaskedCartId,
null,
$storeId
);
$customerCart = $this->getCartForUser->execute(
$customerMaskedCartId,
$currentUserId,
$storeId
);
$customerCart->merge($guestCart);
$guestCart->setIsActive(false);
$this->cartRepository->save($customerCart);
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/QuoteGraphQl/etc/schema.graphqls
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type Mutation {
setPaymentMethodOnCart(input: SetPaymentMethodOnCartInput): SetPaymentMethodOnCartOutput @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\SetPaymentMethodOnCart")
setGuestEmailOnCart(input: SetGuestEmailOnCartInput): SetGuestEmailOnCartOutput @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\SetGuestEmailOnCart")
setPaymentMethodAndPlaceOrder(input: SetPaymentMethodAndPlaceOrderInput): PlaceOrderOutput @deprecated(reason: "Should use setPaymentMethodOnCart and placeOrder mutations in single request.") @resolver(class: "\\Magento\\QuoteGraphQl\\Model\\Resolver\\SetPaymentAndPlaceOrder")
mergeCarts(source_cart_id: String!, destination_cart_id: String!): Cart! @doc(description:"Merges the source cart into the destination cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\MergeCarts")
mergeCarts(source_cart_id: String!, destination_cart_id: String): Cart! @doc(description:"Merges the source cart into the destination cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\MergeCarts")
placeOrder(input: PlaceOrderInput): PlaceOrderOutput @resolver(class: "\\Magento\\QuoteGraphQl\\Model\\Resolver\\PlaceOrder")
addProductsToCart(cartId: String!, cartItems: [CartItemInput!]!): AddProductsToCartOutput @doc(description:"Add any type of product to the cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\AddProductsToCart")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public function testMergeCartsWithEmptySourceCartId()
public function testMergeCartsWithEmptyDestinationCartId()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would ask you to cover this case with an additional test. The test will assert that without providing the destination cart id, the current cart is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rogyar
Thank you for your review.
I extended the tests.
Please check.

Thank you in advance.

{
$this->expectException(\Exception::class);
$this->expectExceptionMessage('Required parameter "destination_cart_id" is missing');
$this->expectExceptionMessage('The parameter "destination_cart_id" cannot be empty');

$guestQuote = $this->quoteFactory->create();
$this->quoteResource->load(
Expand All @@ -209,6 +209,54 @@ public function testMergeCartsWithEmptyDestinationCartId()
$this->graphQlMutation($query, [], '', $this->getHeaderMap());
}

/**
* @magentoApiDataFixture Magento/Checkout/_files/quote_with_virtual_product_saved.php
* @magentoApiDataFixture Magento/Customer/_files/customer.php
* @magentoApiDataFixture Magento/GraphQl/Catalog/_files/simple_product.php
* @magentoApiDataFixture Magento/GraphQl/Quote/_files/customer/create_empty_cart.php
* @magentoApiDataFixture Magento/GraphQl/Quote/_files/add_simple_product.php
*/
public function testMergeCartsWithoutDestinationCartId()
{
$guestQuote = $this->quoteFactory->create();
$this->quoteResource->load(
$guestQuote,
'test_order_with_virtual_product_without_address',
'reserved_order_id'
);
$guestQuoteMaskedId = $this->quoteIdToMaskedId->execute((int)$guestQuote->getId());
$query = $this->getCartMergeMutationWithoutDestinationCartId(
$guestQuoteMaskedId
);
$mergeResponse = $this->graphQlMutation($query, [], '', $this->getHeaderMap());

self::assertArrayHasKey('mergeCarts', $mergeResponse);
$cartResponse = $mergeResponse['mergeCarts'];
self::assertArrayHasKey('items', $cartResponse);
self::assertCount(2, $cartResponse['items']);

$customerQuote = $this->quoteFactory->create();
$this->quoteResource->load($customerQuote, 'test_quote', 'reserved_order_id');
$customerQuoteMaskedId = $this->quoteIdToMaskedId->execute((int)$customerQuote->getId());

$cartResponse = $this->graphQlMutation(
$this->getCartQuery($customerQuoteMaskedId),
[],
'',
$this->getHeaderMap()
);

self::assertArrayHasKey('cart', $cartResponse);
self::assertArrayHasKey('items', $cartResponse['cart']);
self::assertCount(2, $cartResponse['cart']['items']);
$item1 = $cartResponse['cart']['items'][0];
self::assertArrayHasKey('quantity', $item1);
self::assertEquals(2, $item1['quantity']);
$item2 = $cartResponse['cart']['items'][1];
self::assertArrayHasKey('quantity', $item2);
self::assertEquals(1, $item2['quantity']);
}

/**
* Add simple product to cart
*
Expand Down Expand Up @@ -256,6 +304,31 @@ private function getCartMergeMutation(string $guestQuoteMaskedId, string $custom
QUERY;
}

/**
* Create the mergeCart mutation
*
* @param string $guestQuoteMaskedId
* @return string
*/
private function getCartMergeMutationWithoutDestinationCartId(
string $guestQuoteMaskedId
): string {
return <<<QUERY
mutation {
mergeCarts(
source_cart_id: "{$guestQuoteMaskedId}"
){
items {
quantity
product {
sku
}
}
}
}
QUERY;
}

/**
* Get cart query
*
Expand Down