Skip to content

Conversation

@swnsma
Copy link
Contributor

@swnsma swnsma commented Jul 28, 2019

Description

Extension of Quote Graph Ql endpoint to become able to pass pickup location code.
Issue: #2382

al.kravchuk added 3 commits July 27, 2019 10:54
@swnsma swnsma requested a review from naydav July 28, 2019 08:25
@swnsma
Copy link
Contributor Author

swnsma commented Jul 28, 2019

Not sure about copy-paste detector.
For me, it looks that it will be easier to maintain, if the code will be the same.

@ishakhsuvarov
Copy link
Contributor

@swnsma CPD is something we can ignore if there is a valid reason. Shouldn't be a problem here.

al.kravchuk added 2 commits August 2, 2019 13:50
Fix bug with deletion of address pickup locatio .
Add Pickup Location to Quote Shipping Address Graph Ql Schema.
Fix adding of Pickup Location Code to Shipping Address.
$cartRepository->save($cart);

/** @var \Magento\Quote\Model\QuoteIdMask $quoteIdMask */
$quoteIdMask = Bootstrap::getObjectManager()->create(QuoteIdMaskFactory::class)->create();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify in the fixture name that this is a guest quote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I think there need to be done arrangement of fixtures.
I think we can rename it to 'guest' but during API tests coverage, fixtures need to be refactored.

$cartRepository->save($cart);

/** @var \Magento\Quote\Model\QuoteIdMask $quoteIdMask */
$quoteIdMask = Bootstrap::getObjectManager()->create(QuoteIdMaskFactory::class)->create();
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixture name states that it's intended for customer, but this seems to be another guest quote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point that masked_quote_id is required in any case.
Please see.
\Magento\QuoteGraphQl\Model\Resolver\SetShippingAddressesOnCart::resolve
\Magento\QuoteGraphQl\Model\Cart\GetCartForUser::execute

/**
* Set shipping address to the cart. Proceed with passed Pickup Location code.
*/
class SetShippingAddressesOnCart implements SetShippingAddressesOnCartInterface
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
class SetShippingAddressesOnCart implements SetShippingAddressesOnCartInterface
class SetShippingAddressesToCart implements SetShippingAddressesToCartInterface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original interface has name 'SetShippingAddressesOnCartInterface'.
I think that this is better to keep them similar.

…2382-GraphQl-support-for-In-Store-Pickup-Mutation

Conflicts:
	composer.lock
* @throws GraphQlInputException
* @throws GraphQlNoSuchEntityException
*/
private function getShippingAddress(array $shippingAddressInput, ContextInterface $context): Address
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't it possible just to implement a plugin for the class where this logic originally came from?

Copy link
Contributor Author

@swnsma swnsma Aug 29, 2019

Choose a reason for hiding this comment

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

Do you mean around plugin without calling the proceed?

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov left a comment

Choose a reason for hiding this comment

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

Please take a look at the review comments

al.kravchuk added 2 commits August 29, 2019 10:58
…of github.com:magento-engcom/msi into MSI-2382-GraphQl-support-for-In-Store-Pickup-Mutation
@swnsma
Copy link
Contributor Author

swnsma commented Aug 29, 2019

Hi guys, @ishakhsuvarov @lenaorobei,

Thank you for your review.
I have reacted on code review notices.

…2382-GraphQl-support-for-In-Store-Pickup-Mutation
@ishakhsuvarov ishakhsuvarov removed this from the Store Pickup Support milestone Sep 10, 2019
al.kravchuk added 3 commits September 11, 2019 19:08
Fix Pickup Location missed for created order.
…of github.com:magento-engcom/msi into MSI-2382-GraphQl-support-for-In-Store-Pickup-Mutation
@lenaorobei
Copy link
Contributor

No other comments from my side. Please make sure that schema is submitted and approved here https://github.com/magento/architecture/tree/master/design-documents/graph-ql/coverage

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov left a comment

Choose a reason for hiding this comment

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

I've filed an issue #2557 to take one more look at the copy-pasted code in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants