[Behat] Fix base currency step semantics #6791

Zales0123 · 2016-11-21T09:58:27Z

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

According to upcoming changes with currencies, it will be more appropriate to have base currency set in channel creation step (United States channel has obviously USD base currency).

lchrusciel · 2016-11-21T10:12:38Z

.../customer_account/viewing_orders_history/viewing_order_recalculated_by_exchange_rate.feature

@@ -11,7 +11,7 @@ Feature: Viewing details of an order
        And the store operates on a channel named "Web"
        And that channel allows to shop using the "USD" currency
        And that channel allows to shop using the "GBP" currency with exchange rate 3.0
-        And that channel uses the "USD" currency by default
+        And that channel uses the "USD" currency as base


I would change a whole sentence... And that channel operates on "USD" currency?

Not exactly, as channel can operate on many currencies, but it has only one base currency. Maybe sth like And that channel's base currency is "USD"?

Not exactly. Channel will operate only on a one currency, but prices can be shown in different for your convenience. I would stay with And that channel operates on "USD" currency

Not exactly. Channel will operate only on a one currency, but for now it can operate on many of them. So for now this sentence would be incorrect.

Not exactly 🌮

Not exactly. Channel can operate only on a one currency from the beginning. If we want to be so semantical strict, the sentence should be And that channel base display currency is "USD". Anyway, with your last argument there is no point in changing it now, because this sentence will be for 100% incorrect in one/two days.

Not exactly. There is a concept of baseCurrency on Channel entity and it will not be deleted in the nearest (or further) future. So the sentence And that channel's base currency is "USD" is 100% correct now and in the future. And that channel uses the "USD" currency by default is not 100% correct neither now nor in the future, IMO the same with And that channel operates on "USD" currency. channel's base currency is not open to interpretation, it's exactly what we do with channel (especially as it's the setup step).

OK, so after internal brainstorm we decided to merge this step with step that creates channel to have And the store operates on a channel named "Web" in "USD" currency - that will be suitable for further pricing changes (however destroys our beautiful argument 😢 ).

michalmarcinkowski · 2016-11-21T13:38:05Z

...t/shopping_cart/adding_product_to_cart_with_prices_dependent_on_channel_and_currency.feature

        And that channel allows to shop using "EUR" and "GBP" currencies
-        And that channel uses the "EUR" currency as base
        And the store operates on another channel named "Web-GB"


... another channel named "Web-GB" in currency "GBP"

michalmarcinkowski · 2016-11-21T13:39:06Z

...t/shopping_cart/adding_product_to_cart_with_prices_dependent_on_channel_and_currency.feature

        And that channel allows to shop using "EUR" and "GBP" currencies
-        And that channel uses the "EUR" currency by default
        And the store operates on another channel named "Web-GB"


... another channel named "Web-GB" **in "GBP" currency

michalmarcinkowski · 2016-11-21T13:40:05Z

src/Sylius/Behat/Context/Setup/CurrencyContext.php

     */
-    public function itUsesTheCurrencyByDefault(ChannelInterface $channel, $currencyCode)
+    public function itUsesTheCurrencyAsBase(ChannelInterface $channel, $currencyCode)


Should be removed.

pamil · 2016-11-21T13:38:51Z

.../customer_account/viewing_orders_history/viewing_order_recalculated_by_exchange_rate.feature

@@ -8,10 +8,9 @@ Feature: Viewing details of an order
        Given the store ships to "British Virgin Islands"
        And the store has a zone "English" with code "EN"
        And this zone has the "British Virgin Islands" country member
-        And the store operates on a channel named "Web"
+        And the store operates on a channel named "Web" in currency "USD"
        And that channel allows to shop using the "USD" currency


Is it still needed?

pamil · 2016-11-21T13:39:14Z

...ut/seeing_order_summary/seeing_prices_recalculated_by_exchange_rate_on_order_summary.feature

@@ -8,10 +8,9 @@ Feature: Seeing prices recalculated by exchange rate on order summary
        Given the store ships to "British Virgin Islands"
        And the store has a zone "English" with code "EN"
        And this zone has the "British Virgin Islands" country member
-        And the store operates on a channel named "Web"
+        And the store operates on a channel named "Web" in currency "USD"
        And that channel allows to shop using the "USD" currency


Same here etc.

pamil · 2016-11-21T13:39:54Z

src/Sylius/Component/Core/Test/Services/DefaultUnitedStatesChannelFactory.php

    {
+        if (null === $currencyCode) {


$currencyCode = $currencyCode ?: self::DEFAULT_CURRENCY_CODE;

pamil · 2016-11-21T13:40:32Z

src/Sylius/Component/Core/Test/Services/DefaultChannelFactory.php

    {
+        if (null === $currencyCode) {


$currencyCode = $currencyCode ?: $this->defaultCurrencyCode;

michalmarcinkowski · 2016-11-21T13:42:32Z

Which of these two is correct ... in currency "USD" or ... in "USD" currency? /cc @TheMadeleine

CoderMaggie · 2016-11-22T07:11:22Z

@michalmarcinkowski both are ok, but in "USD" currency sounds better imo.

michalmarcinkowski · 2016-11-22T07:15:41Z

I would vote for in "USD" currency also. @Zales0123 could you correct it? :)

michalmarcinkowski · 2016-11-22T13:31:24Z

src/Sylius/Component/Core/Test/Services/DefaultUnitedStatesChannelFactory.php

    {
+        $currencyCode = $currencyCode ?: self::DEFAULT_CURRENCY_CODE;


I think we can always use USD in this case, it's "The United States" channel, so the currency is pretty much known and immutable :) In Default channel it makes sense, but for this specific factory it doesn't IMO.

michalmarcinkowski · 2016-11-22T13:34:03Z

src/Sylius/Behat/Context/Setup/OrderContext.php

        $order->setLocaleCode((null !== $localeCode) ? $localeCode : $this->sharedStorage->get('locale')->getCode());

-        $currencyCode = $currencyCode ? $currencyCode : $this->sharedStorage->get('currency')->getCode();
+        $currencyCode = $currencyCode ? $currencyCode : $order->getChannel()->getBaseCurrency()->getCode();


The order should be always created with base currency of the channel in which it was placed. We should not allow to pass other value here.

michalmarcinkowski · 2016-11-22T13:36:07Z

src/Sylius/Behat/Context/Setup/ChannelContext.php


        $this->sharedStorage->setClipboard($defaultData);
        $this->sharedStorage->set('channel', $defaultData['channel']);
+        $this->currencyStorage->set($defaultData['channel'], $defaultData['currency']->getCode());


Do we need this?

michalmarcinkowski · 2016-11-22T13:36:49Z

Thanks Mateusz! Please apply my comments in a separate PR.

…cy-step [Behat] Fix base currency step semantics

lchrusciel requested changes Nov 21, 2016

View reviewed changes

pjedrzejewski added the Bug Fix label Nov 21, 2016

michalmarcinkowski reviewed Nov 21, 2016

View reviewed changes

pamil reviewed Nov 21, 2016

View reviewed changes

lchrusciel mentioned this pull request Nov 21, 2016

[WIP] Order placed in channel currency #6797

Closed

Zales0123 added 2 commits November 21, 2016 16:50

[Behat] Fix base currency step semantics

a3ae028

[Behat] Merge base currency step with channel creation step

597bb32

Zales0123 force-pushed the fix-base-channel-currency-step branch from 4a0272b to d5a7426 Compare November 21, 2016 15:50

Zales0123 force-pushed the fix-base-channel-currency-step branch from d5a7426 to 7981e8d Compare November 22, 2016 07:29

michalmarcinkowski approved these changes Nov 22, 2016

View reviewed changes

michalmarcinkowski added the Ready label Nov 22, 2016

lchrusciel approved these changes Nov 22, 2016

View reviewed changes

PR review fixes

7c20c56

Zales0123 force-pushed the fix-base-channel-currency-step branch from 7981e8d to 7c20c56 Compare November 22, 2016 09:31

michalmarcinkowski reviewed Nov 22, 2016

View reviewed changes

michalmarcinkowski merged commit 46f179e into Sylius:master Nov 22, 2016

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#6791 from Zales0123/fix-base-channel-curren…

313da66

…cy-step [Behat] Fix base currency step semantics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Behat] Fix base currency step semantics #6791

[Behat] Fix base currency step semantics #6791

Zales0123 commented Nov 21, 2016 •

edited

Loading

lchrusciel Nov 21, 2016

Zales0123 Nov 21, 2016

lchrusciel Nov 21, 2016

Zales0123 Nov 21, 2016

pamil Nov 21, 2016

lchrusciel Nov 21, 2016 •

edited

Loading

Zales0123 Nov 21, 2016

Zales0123 Nov 21, 2016

michalmarcinkowski Nov 21, 2016

michalmarcinkowski Nov 21, 2016

michalmarcinkowski Nov 21, 2016

pamil Nov 21, 2016

pamil Nov 21, 2016

pamil Nov 21, 2016

pamil Nov 21, 2016

michalmarcinkowski commented Nov 21, 2016

CoderMaggie commented Nov 22, 2016

michalmarcinkowski commented Nov 22, 2016

michalmarcinkowski Nov 22, 2016

michalmarcinkowski Nov 22, 2016

michalmarcinkowski Nov 22, 2016

michalmarcinkowski commented Nov 22, 2016

		{
		$currencyCode = $currencyCode ?: self::DEFAULT_CURRENCY_CODE;

[Behat] Fix base currency step semantics #6791

[Behat] Fix base currency step semantics #6791

Conversation

Zales0123 commented Nov 21, 2016 • edited Loading

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

lchrusciel Nov 21, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented Nov 21, 2016

CoderMaggie commented Nov 22, 2016

michalmarcinkowski commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented Nov 22, 2016

Zales0123 commented Nov 21, 2016 •

edited

Loading

lchrusciel Nov 21, 2016 •

edited

Loading