[Admin][Province] Managing country with provinces #4689

GSadee · 2016-04-04T22:43:29Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets
License	MIT

Based on #4499

CoderMaggie · 2016-04-05T05:27:52Z

features/addressing/managing_countries/editing_country_with_provinces.feature

+
+    @ui @javascript
+    Scenario: Adding province to existing country
+        Given I want to edit this country


Given I want to edit the country "United Kingdom"

Why cannot I use this country in this step?

Because regarding the scope of the scenario we do not know which country it is. Even though there is only one defined in the shop by that I am forcing the reader to go back and read the background.
IMHO using the country's name is a whole lot more readable and we are really taking care of it in the new Behats. :)

I do not agree, IMO this is readable to the same extent as using country's name. This feature is using in every scenarios only one country and writing this country is more natural for me than using in every step country's name.

Please have a look at for instance the already merged editing_tax_category.feature, editing_tax_rate.feature or editing_locale.feature. All of the use the convention I've suggested.
@pjedrzejewski @michalmarcinkowski ?

CoderMaggie · 2016-04-05T05:34:00Z

You can tag the scenarios that are not yet implemented with a @todo tag instead of suites tags, they will be skipped in tests then. :)

lchrusciel · 2016-04-05T07:04:09Z

features/addressing/managing_countries/editing_country_with_provinces.feature

+    I want to be able to edit a country and its provinces
+
+    Background:
+        Given I am logged in as an administrator


This steps should be swapped. I am logged in as an administrator should be always written as a last one.

lchrusciel · 2016-04-05T07:07:57Z

features/addressing/managing_countries/editing_country_with_provinces.feature

@@ -0,0 +1,26 @@
+@addressing
+Feature: Editing country with provinces


I think it is misleading to name it Editing country with provinces. IMHO it should something like Managing provinces of a country? Or if you are convinced, that it should be a part of editing it should be placed under editing_country.feature

pamil · 2016-04-05T14:35:22Z

.travis.yml

@@ -60,6 +60,8 @@ before_script:
    - app/console assets:install --env=test_cached --no-debug
    - app/console assetic:dump --env=test_cached --no-debug

+    - npm install


Should be executed in install section (just as Composer install).

Also node_modules should be able to be added to cached directories.

Changes from #4499 on which this PR based on

lchrusciel · 2016-04-06T11:18:26Z

etc/behat/services/contexts/setup.xml

            <argument type="service" id="sylius.factory.country" container="symfony" />
            <argument type="service" id="sylius.repository.country" container="symfony" />
            <argument type="service" id="sylius.converter.country_name" container="symfony" />
-            <argument type="service" id="sylius.behat.shared_storage" container="symfony" />
+            <argument type="service" id="sylius.factory.province" container="symfony" />


I would suggest to put factories together

michalmarcinkowski · 2016-04-12T12:56:19Z

src/Sylius/Behat/Context/Ui/Admin/ManagingCountriesContext.php

-            $this->countryIndexPage->isResourceOnPage(['code' => $country->getCode()]),
-            sprintf('Country %s should exist but it does not', $country->getCode())
+            $this->countryUpdatePage->isOpen(['id' => $country->getId()]),
+            sprintf('Country %s does not appear in the store.', $country->getCode())


What was wrong with previous implementation? We agreed to check existence on index page.

…in existing country

…rovince

michalmarcinkowski · 2016-04-14T05:45:54Z

Thanks Grzesiu! 👍

CoderMaggie reviewed Apr 5, 2016
View reviewed changes

lchrusciel reviewed Apr 5, 2016
View reviewed changes

GSadee force-pushed the editing-country-with-provinces branch from 5281de3 to 727e2d8 Compare April 5, 2016 07:05

lchrusciel reviewed Apr 5, 2016
View reviewed changes

GSadee force-pushed the editing-country-with-provinces branch 4 times, most recently from 82090dd to 65c3b1f Compare April 5, 2016 13:54

pamil reviewed Apr 5, 2016
View reviewed changes

GSadee force-pushed the editing-country-with-provinces branch from 65c3b1f to 4bc9fa6 Compare April 6, 2016 10:34

michalmarcinkowski added Behat Issues and PRs aimed at improving Behat usage. Awaiting Review Admin AdminBundle related issues and PRs. labels Apr 6, 2016

lchrusciel reviewed Apr 6, 2016
View reviewed changes

GSadee force-pushed the editing-country-with-provinces branch 2 times, most recently from ae54a99 to f836b9b Compare April 6, 2016 13:24

michalmarcinkowski reviewed Apr 12, 2016
View reviewed changes

GSadee force-pushed the editing-country-with-provinces branch 3 times, most recently from ce63581 to f426ded Compare April 13, 2016 14:09

CoderMaggie mentioned this pull request Apr 13, 2016

[Admin] Adding countries with provinces #4499

Closed

Magdalena Banasiak and others added 15 commits April 13, 2016 16:27

Adding countries with provinces scenarios

cc4df7e

Adding provinces with Country

7bf189b

Selecting countries and entering province name in Selenium fix

3e0c26c

Adding two provinces with country

12d062b

Review fixes

799cbe2

[Admin][Province] Added scenarios for addding and removing provinces …

c8b6f2f

…in existing country

[Admin][Province] Behat implementation of adding and removing provinces

e4c1eaf

[Admin][Province] Fixes after PR review

d5c4b52

[Admin][Province] Added scenarios for validation required fields in p…

c7e484d

…rovince

[Admin][Province] Implemented scenarios for validation required fields

4635a20

Added missing edit country message

8827ada

[Admin][Province] Added province unique code validation scenario

53340f1

[Admin][Province] Implemented province unique code validation scenario

745d52f

[Admin][Province] Some fixes to country and province in behat

fb3d984

[Admin][Province] Fixes after PR review

8beb0e3

GSadee force-pushed the editing-country-with-provinces branch from f426ded to 8beb0e3 Compare April 13, 2016 14:27

GSadee changed the title ~~[Admin][Province] Editing country with provinces~~ [Admin][Province] Managing country with provinces Apr 13, 2016

michalmarcinkowski merged commit f4c4ac9 into Sylius:master Apr 14, 2016

GSadee deleted the editing-country-with-provinces branch September 22, 2017 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Admin][Province] Managing country with provinces #4689

[Admin][Province] Managing country with provinces #4689

GSadee commented Apr 4, 2016

CoderMaggie Apr 5, 2016

GSadee Apr 5, 2016

CoderMaggie Apr 5, 2016

GSadee Apr 5, 2016

CoderMaggie Apr 5, 2016

CoderMaggie commented Apr 5, 2016

lchrusciel Apr 5, 2016

lchrusciel Apr 5, 2016

pamil Apr 5, 2016

pamil Apr 5, 2016

GSadee Apr 6, 2016

lchrusciel Apr 6, 2016

michalmarcinkowski Apr 12, 2016

michalmarcinkowski commented Apr 14, 2016

		@@ -0,0 +1,26 @@
		@addressing
		Feature: Editing country with provinces

[Admin][Province] Managing country with provinces #4689

[Admin][Province] Managing country with provinces #4689

Conversation

GSadee commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoderMaggie commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented Apr 14, 2016