Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Country] Adding a province to a country manually #6002

Merged
merged 6 commits into from
Sep 22, 2016
Merged

[Country] Adding a province to a country manually #6002

merged 6 commits into from
Sep 22, 2016

Conversation

tuka217
Copy link
Contributor

@tuka217 tuka217 commented Sep 7, 2016 •
edited
Loading

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

@tuka217 tuka217 force-pushed the country-province branch 3 times, most recently from 5089a2a to 1edb2f6 Compare September 7, 2016 09:51
@tuka217 tuka217 force-pushed the country-province branch 5 times, most recently from e1234b1 to 4d367a2 Compare September 12, 2016 09:41
@tuka217 tuka217 changed the title [WIP][Country] Adding a province to a country manually [Country] Adding a province to a country manually Sep 12, 2016
@tuka217 tuka217 force-pushed the country-province branch 8 times, most recently from 3a55532 to ac986e0 Compare September 13, 2016 08:00
Zales0123
Zales0123 reviewed Sep 13, 2016
View reviewed changes
src/Sylius/Behat/Context/Ui/Admin/ManagingCustomersContext.php
/**
* @Then the province in the shipping address should be :provinceName
*/
public function hisProvinceInTheShippingAddressShouldBe($provinceName)
Copy link
Member

Choose a reason for hiding this comment

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

theProvinceInTheShippingAddressShouldBe

Zales0123
Zales0123 reviewed Sep 13, 2016
View reviewed changes
src/Sylius/Component/Addressing/Provider/ProvinceNamingProvider.php
Assert::notNull($province,sprintf('Province with code "%s" not found.', $address->getProvinceCode()));

if (null !== $provinceAbbreviation = $province->getAbbreviation()) {

Copy link
Member

Choose a reason for hiding this comment

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

Unneeded blank line, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we always put blank line before return?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line before return statements, unless the return is alone inside a statement-group (like an if statement);

Btw. we can remove this if and the next return statement by:

return $province->getAbbreviation() ?: $province->getName();

@tuka217 tuka217 force-pushed the country-province branch 4 times, most recently from 4f48377 to c9a8b56 Compare September 14, 2016 09:04
NoResponseMate
NoResponseMate reviewed Sep 14, 2016
View reviewed changes
...mer_account/viewing_orders_history/seeing_province_created_manually_in_order_history.feature
Scenario: Seeing a province manually defined in a order history
When I view the summary of the order "#00000666"
Then I should see "East of England" as province in the shipping address
And I should see "East of England" ad province in the billing address
Copy link
Contributor

Choose a reason for hiding this comment

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

as province

michalmarcinkowski
michalmarcinkowski suggested changes Sep 15, 2016
View reviewed changes
...mer_account/viewing_orders_history/seeing_province_created_manually_in_order_history.feature
And the store has "DHL" shipping method with "$20.00" fee within "EN" zone
And the store has a product "Angel T-Shirt" priced at "$39.00"
And I am a logged in customer
And I placed an order "#00000666"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have to be logged in customer, We can use step "And customer "x" placed order ...`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I want to see my order in the history.

...mer_account/viewing_orders_history/seeing_province_created_manually_in_order_history.feature
And the store has a zone "English" with code "EN"
And this zone has the "United Kingdom" country member
And the store operates on a channel named "Web"
And that channel allows to shop using the "GBP" currency
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

...mer_account/viewing_orders_history/seeing_province_created_manually_in_order_history.feature
Given the store ships to "United Kingdom"
And the store has a zone "English" with code "EN"
And this zone has the "United Kingdom" country member
And the store operates on a channel named "Web"
Copy link
Contributor

Choose a reason for hiding this comment

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

Channel should be specified in the first step.

...mer_account/viewing_orders_history/seeing_province_created_manually_in_order_history.feature
I want to be able to see province in the order history

Background:
Given the store ships to "United Kingdom"
Copy link
Contributor

Choose a reason for hiding this comment

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

the store operates in "United Kingdom"

...checkout/seeing_order_summary/seeing_province_created_manually_on_order_summary_page.feature

@ui @javascript
Scenario: Seeing manually defined province on order summary page
Given I had product "PHP T-Shirt" in the cart
Copy link
Contributor

Choose a reason for hiding this comment

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

I added - IMO is more appropriate

src/Sylius/Bundle/AdminBundle/Resources/views/Common/_address.html.twig
{{ address.provinceCode|sylius_province_abbreviation }}<br/>
{{ address|sylius_province_name }}
{% if address|sylius_province_name is not null %}
<br/>
Copy link
Contributor

@michalmarcinkowski michalmarcinkowski Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think the following will be more readable:

{% if address|sylius_province_name is not null %}
    {{ address|sylius_province_name }}<br/>
{% endif %}

src/Sylius/Bundle/ShopBundle/Resources/views/Common/_address.html.twig
{{ address.provinceCode|sylius_province_abbreviation }}<br/>
{{ address|sylius_province_name }}
{% if address|sylius_province_name is not null %}
<br/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

src/Sylius/Component/Addressing/Provider/ProvinceNamingProvider.php
}

return $provinceCode;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

       if (null !== $address->getProvinceName()) {
            return $address->getProvinceName();
        }

        if (null === $address->getProvinceCode()) {
             return null;
        }

        $province = $this->provinceRepository->findOneBy(['code' => $address->getProvinceCode()]);
        Assert::notNull($province,sprintf('Province with code "%s" not found.', $address->getProvinceCode()));

        return $province->getName();

IMO looks more readable, WDYT?

src/Sylius/Component/Addressing/Provider/ProvinceNamingProvider.php
}

return $province;
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

src/Sylius/Component/Addressing/Provider/ProvinceNamingProviderInterface.php
*
* @return string
* @return string|null
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 return null or empty string, WDYT?

Copy link
Contributor Author

@tuka217 tuka217 Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Not big difference, because in twig we do not render this field if it is null. But using string only will be more coherent.

@tuka217 tuka217 force-pushed the country-province branch 4 times, most recently from 1644fa2 to c6f7c48 Compare September 16, 2016 10:54
michalmarcinkowski
michalmarcinkowski reviewed Sep 21, 2016
View reviewed changes
...es/user/managing_customers/seeing_province_created_manually_on_customer_details_page.feature
I want to be able to see specific customer's page with provinces in the addresses

Background:
Given I am logged in as an administrator
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have only one scenario here the background in not necessary.

michalmarcinkowski
michalmarcinkowski reviewed Sep 21, 2016
View reviewed changes
src/Sylius/Behat/Page/Shop/Checkout/AddressPage.php
return $this->hasElement('login_button');
});
}

private function waitForLoginAction()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be replaced with waitForElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot. It waits until an element disappear.

src/Sylius/Bundle/ShopBundle/Resources/views/Common/_address.html.twig
@@ -1,4 +1,4 @@
<address>
\<address>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be reverted.

src/Sylius/Bundle/AddressingBundle/Twig/ProvinceNamingExtension.php
*
* @return string
* @return string|null
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not return null anymore.

src/Sylius/Bundle/AddressingBundle/Twig/ProvinceNamingExtension.php
*
* @return string
* @return string|null
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not return null anymore.

@michalmarcinkowski
Copy link
Contributor

@tuka217 please rebase. Last comments can be applied in a separate PR.

@tuka217 tuka217 force-pushed the country-province branch 2 times, most recently from be195ed to a8f2e19 Compare September 22, 2016 08:37
@michalmarcinkowski michalmarcinkowski merged commit 6bbea4c into Sylius:master Sep 22, 2016
@michalmarcinkowski
Copy link
Contributor

Nice work Ania! 👍

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

@michalmarcinkowski michalmarcinkowski michalmarcinkowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tuka217 @michalmarcinkowski @pamil @Zales0123 @NoResponseMate @pjedrzejewski