[Api][Address] Browsing AddressBook #11914

Tomanhez · 2020-10-07T06:35:08Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
License	MIT

AdamKasp

generally I have problem with scenario tagged @javascript, it looks like UI feature, and IMO we shouldn't cover it with API especially when you added step And my billing address is fulfilled automatically after filling field which is matching to field from my address book's address in API we don't have behaviour like automatically do sth it should be call to addresses/index and the UI should fill address with data from addressbook, But of course i am open to discussion.

AdamKasp · 2020-10-07T07:38:17Z

src/Sylius/Behat/Context/Api/Shop/AddressContext.php

+    public function thisAddressShouldBeAssignedTo(string $fullName): void
+    {
+        /** @var AddressInterface $address */
+        $address = $this->sharedStorage->get('address_assigned_to_' . $fullName);


fullName has space between first and last name IMO you shouldn't have the key in shared storage with spaces,
address_assigned_to_Joe Doe ith looks bad :(

Yes you aright, but $fullName is behind the scene, I can convert it to loverCase and add padding here and in places when I must get it by customer name, but for me its unnecessary confusion.

You can use StringInflector class, just add there an method

public static function nameToSnakeCase(string $value): string { return (string) u($value)->snake(); } ```

src/Sylius/Behat/Context/Ui/Shop/AddressBookContext.php

src/Sylius/Behat/Resources/config/services/api.xml

src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/Address.xml

GSadee · 2020-10-07T07:41:57Z

features/account/customer_account/address_book/viewing_addresses.feature

@@ -6,15 +6,15 @@ Feature: Viewing my address book

    Background:
        Given the store operates on a single channel in "United States"
-        And I am a logged in customer
+        And I am a logged in customer with name "Lucifer Morningstar"


Why did you need to change that?

I changed it, because IMO scenario Inability to view the addresses of other customers is not clearly and which is customer about in step this address should be assigned to "Lucifer Morningstar", we have added address with this name for this customer, but this customer is not "Lucifer Morningstar" but random named

...es/checkout/addressing_order/choosing_billing_and_shipping_address_from_address_book.feature

src/Sylius/Behat/Context/Api/Shop/CheckoutContext.php

src/Sylius/Component/Core/Repository/AddressRepositoryInterface.php

Tomanhez · 2020-10-07T08:32:10Z

generally I have problem with scenario tagged @javascript, it looks like UI feature, and IMO we shouldn't cover it with API especially when you added step And my billing address is fulfilled automatically after filling field which is matching to field from my address book's address in API we don't have behaviour like automatically do sth it should be call to addresses/index and the UI should fill address with data from addressbook, But of course i am open to discussion.

I think that few tests for addressBook like choosing_billing_and_shipping_address_from_address_book.feature are wrote not perfect for UI, because lots is happening automatically and is based on autocomplete from web browser, I add few new steps for greater clarity and of course for improve to work with @api tests. I know that many of these behawior is hard to recode on api cases but, IMO its only what i can do. Other way is write new tests for API behawior instead of present tests.

GSadee · 2020-10-07T08:54:51Z

generally I have problem with scenario tagged @javascript, it looks like UI feature, and IMO we shouldn't cover it with API especially when you added step And my billing address is fulfilled automatically after filling field which is matching to field from my address book's address in API we don't have behaviour like automatically do sth it should be call to addresses/index and the UI should fill address with data from addressbook, But of course i am open to discussion.

I think that few tests for addressBook like choosing_billing_and_shipping_address_from_address_book.feature are wrote not perfect for UI, because lots is happening automatically and is based on autocomplete from web browser, I add few new steps for greater clarity and of course for improve to work with @api tests. I know that many of these behawior is hard to recode on api cases but, IMO its only what i can do. Other way is write new tests for API behawior instead of present tests.

I'm not fully convinced but we could implement the existing scenarios without adding these unneeded (unclear for me) new steps. The proper behaviour for getting address from customer's address book could be implemented in the existing steps in API context.

AdamKasp · 2020-10-08T11:26:25Z

Thanks, Tomasz! 🎉

GSadee · 2020-10-08T12:41:54Z

src/Sylius/Behat/Context/Api/Shop/CheckoutContext.php

+        if (
+            $address['firstName'] === $addressToCompare->getFirstName() &&
+            $address['lastName'] === $addressToCompare->getLastName() &&
+            $address['countryCode'] === $addressToCompare->getCountryCode() &&
+            $address['street'] === $addressToCompare->getStreet() &&
+            $address['city'] === $addressToCompare->getCity() &&
+            $address['postcode'] === $addressToCompare->getPostcode() &&
+            ($addressToCompare->getProvinceName() !== null && isset($address['provinceName'])) ?
+                $address['provinceName'] === $addressToCompare->getProvinceName() : true
+        ) {
+            return true;
+        }
+
+        return false;


Could be simplified to one return

This PR was merged into the 1.9-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Related tickets | partially #11914 ( this needs to be merged first) | License | MIT Commits ------- b566cd5 add api delete address book d0d71cc fixed small issues in PR ef7b2fd merged scenarios in one

Tomanhez requested a review from a team as a code owner October 7, 2020 06:35

AdamKasp reviewed Oct 7, 2020

View reviewed changes

GSadee reviewed Oct 7, 2020

View reviewed changes

Tomanhez added 4 commits October 7, 2020 10:42

Modify behats

a5d1437

Add contexts implementation

062eec4

Add resource config for AddressBook

fc722c2

Add data providers for Address

67df566

Tomanhez force-pushed the address-book-api branch from ed13b6d to 83872a4 Compare October 7, 2020 09:02

Add fixes for tests

8b4b882

Tomanhez force-pushed the address-book-api branch from 83872a4 to 8b4b882 Compare October 7, 2020 09:13

arti0090 approved these changes Oct 8, 2020

View reviewed changes

arti0090 mentioned this pull request Oct 8, 2020

[API]delete address book #11915

Merged

AdamKasp merged commit 91f26cd into Sylius:master Oct 8, 2020

GSadee reviewed Oct 8, 2020

View reviewed changes

dependabot bot mentioned this pull request Mar 16, 2021

composer(deps-dev): update sylius/sylius requirement from ~1.8.9 to ~1.8.9 || ~1.9.0 Setono/SyliusTikTokPlugin#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Api][Address] Browsing AddressBook #11914

[Api][Address] Browsing AddressBook #11914

Tomanhez commented Oct 7, 2020

AdamKasp left a comment

AdamKasp Oct 7, 2020

Tomanhez Oct 7, 2020

arti0090 Oct 8, 2020

GSadee Oct 7, 2020

Tomanhez Oct 7, 2020

Tomanhez commented Oct 7, 2020

GSadee commented Oct 7, 2020

AdamKasp commented Oct 8, 2020

GSadee Oct 8, 2020

[Api][Address] Browsing AddressBook #11914

[Api][Address] Browsing AddressBook #11914

Conversation

Tomanhez commented Oct 7, 2020

AdamKasp left a comment

Choose a reason for hiding this comment

AdamKasp Oct 7, 2020

Choose a reason for hiding this comment

Tomanhez Oct 7, 2020

Choose a reason for hiding this comment

arti0090 Oct 8, 2020

Choose a reason for hiding this comment

GSadee Oct 7, 2020

Choose a reason for hiding this comment

Tomanhez Oct 7, 2020

Choose a reason for hiding this comment

Tomanhez commented Oct 7, 2020

GSadee commented Oct 7, 2020

AdamKasp commented Oct 8, 2020

GSadee Oct 8, 2020

Choose a reason for hiding this comment