Skip to content
This repository was archived by the owner on Dec 19, 2019. It is now read-only.

Conversation

@sergiy-v
Copy link

@sergiy-v sergiy-v commented Sep 4, 2019

Description (*)

Improved strict typing in SelectedShippingMethod.

Fixed Issues (if relevant)

#893: [Checkout] Improve strict typing in SelectedShippingMethod

Copy link
Contributor

@lenaorobei lenaorobei left a comment

Choose a reason for hiding this comment

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

The expected behaviour here is when method is selected return array with data, if not, return null. We should not return array keys with null values.

@sergiy-v sergiy-v requested a review from paliarush as a code owner September 6, 2019 05:55
@sergiy-v
Copy link
Author

sergiy-v commented Sep 6, 2019

The expected behaviour here is when method is selected return array with data, if not, return null. We should not return array keys with null values.

Thank you for the clarification, added the changes.

@sergiy-v sergiy-v requested a review from lenaorobei September 6, 2019 05:57
…ShippingMethod

# Conflicts:
#	app/code/Magento/QuoteGraphQl/Model/Resolver/ShippingAddress/SelectedShippingMethod.php
#	app/code/Magento/QuoteGraphQl/etc/schema.graphqls
#	dev/tests/api-functional/testsuite/Magento/GraphQl/Quote/Customer/GetSelectedShippingMethodTest.php
#	dev/tests/api-functional/testsuite/Magento/GraphQl/Quote/Guest/GetSelectedShippingMethodTest.php
@lenaorobei
Copy link
Contributor

@sergiy-v could you please pull latest changes from 2.3-develop. This issue is partially resolved by #884 and #906

…ShippingMethod

# Conflicts:
#	app/code/Magento/QuoteGraphQl/Model/Resolver/ShippingAddress/SelectedShippingMethod.php
@sergiy-v
Copy link
Author

@lenaorobei pulled, thank you.

$rates = $address->getAllShippingRates();
$carrierTitle = null;
$methodTitle = null;
$carrierTitle = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove redundant lines.

Copy link
Author

@sergiy-v sergiy-v Sep 20, 2019

Choose a reason for hiding this comment

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

Please remove redundant lines.

In case I'll remove these lines the $carrierTitle and the $methodTitle variables might not be defined (please check lines 46, 47).
Should I add any changes here?
Thank you.

@lenaorobei
Copy link
Contributor

@magento run Integration Tests build

@magento-engcom-team
Copy link
Contributor

Hi @lenaorobei, thank you for the review.
ENGCOM-5913 has been created to process this Pull Request
✳️ @lenaorobei, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@ghost
Copy link

ghost commented Sep 25, 2019

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants