-
Notifications
You must be signed in to change notification settings - Fork 153
graphQl-509: save_in_address_book
has no impact on Address Book
#873
graphQl-509: save_in_address_book
has no impact on Address Book
#873
Conversation
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 address static build failure.
|
||
$customerId = $context->getUserId(); | ||
// need to save address only for registered user and if save_in_address_book = true | ||
if (0 !== $customerId && !empty($addressInput['save_in_address_book'])) { |
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 throw an exception for guests.
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.
Seems like we don't need it in this place. We have this exception in case $customerAddressId !== null
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.
I believe we can move this code to the top of the method.
if (false === $context->getExtensionAttributes()->getIsCustomer()) {
throw new GraphQlAuthorizationException(__('The current customer isn\'t authorized.'));
}
This will simplify the complexity.
And if value is expected to be boolean
, please use strict comparison.
(bool)$addressInput['save_in_address_book'] === false
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.
@lenaorobei, Seems like we can't as with if (null === $customerAddressId)
we are creating billing address for guest.
Array may don't have "save_in_address_book". So we will need double check: for isset and (bool)$addressInput['save_in_address_book'] === true
. Can't we just leave only check for empty()
?
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.
Because of PHP type casting please use isset
+ === true
.
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.
Done
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 consider refactoring createBillingAddress
and createShippingAddress
methods.
# Conflicts: # app/code/Magento/QuoteGraphQl/Model/Cart/SetShippingAddressesOnCart.php
Hi @lenaorobei, thank you for the review. |
Hi @kisroman, thank you for your contribution! |
#509