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.

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

[Product][Shop] Buy product in correct prices for channel and currency #6666

Conversation

Zales0123
Copy link
Member

@Zales0123 Zales0123 commented Nov 8, 2016

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

@Zales0123 Zales0123 force-pushed the buy-product-in-correct-price-for-channel-and-currency branch from 3dd1812 to cbd7fc0 Compare November 8, 2016 21:32
@Zales0123 Zales0123 closed this Nov 8, 2016
@Zales0123 Zales0123 reopened this Nov 8, 2016
@Zales0123 Zales0123 changed the title [WIP] Buy product in correct price for channel and currency [WIP][Product][Shop] Buy product in correct prices for channel and currency Nov 8, 2016
@Zales0123
Copy link
Member Author

I've opened this PR accidentally (I didn't want to open it yet) - but then I've just realized I can't lose such a beautiful PR number as #6666 😄

@Zales0123 Zales0123 force-pushed the buy-product-in-correct-price-for-channel-and-currency branch 2 times, most recently from dab967c to 203d11b Compare November 9, 2016 12:40
@Zales0123
Copy link
Member Author

@pjedrzejewski @michalmarcinkowski do we want also to implement displaying product price based on pricing calculator on product's details page? Or is it the case for the separate PR?

@Zales0123 Zales0123 changed the title [WIP][Product][Shop] Buy product in correct prices for channel and currency [Product][Shop] Buy product in correct prices for channel and currency Nov 9, 2016
@Zales0123 Zales0123 force-pushed the buy-product-in-correct-price-for-channel-and-currency branch from 203d11b to 55bf26b Compare November 9, 2016 13:58
Copy link
Member

@pjedrzejewski pjedrzejewski left a comment

Choose a reason for hiding this comment

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

  1. The amounts in the configuration array will be stored in the specific currency, not base amount. You should take exchange rates into account in scenarios and in the calculator, convert the specified amount to the base amount before returning it.
  2. It should not use currency context - this makes it stateful. It should get the currency as part of the "context" array passed to the calculator.

@Zales0123 Zales0123 force-pushed the buy-product-in-correct-price-for-channel-and-currency branch 2 times, most recently from 0061b0a to 21bbb2c Compare November 10, 2016 12:02
@@ -150,9 +150,6 @@ public function thatChannelAllowsToShopUsingAndCurrencies(ChannelInterface $chan
public function itUsesTheCurrencyByDefault(ChannelInterface $channel, $currencyCode)
{
$currency = $this->provideCurrency($currencyCode);
$currency->setExchangeRate(1.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This step is that channel uses the currency by default -> it should not set this currency's exchange rate to 1.0, as exchange rate is not configured per channel, but for whole system. In my case, it caused a mistake when one channel has base currency EUR and the other one GBP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so it means channel, not store. Totally valid fix, good catch! 👍

*/
public function itHasDifferentPricesForDifferentChannelsAndCurrencies(ProductInterface $product)
{
$product->getFirstVariant()->setPricingCalculator(Calculators::CHANNEL_AND_CURRENCY_BASED);
Copy link
Contributor

Choose a reason for hiding this comment

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

getFirstVariant method should be removed. defaultVariantResolver should be used instead.

ChannelInterface $channel,
$currency
) {
$variant = $product->getFirstVariant();
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.

@@ -20,6 +20,7 @@
<argument type="service" id="__symfony__.sylius.factory.channel" />
<argument type="service" id="__symfony__.sylius.repository.channel" />
<argument type="service" id="__symfony__.sylius.manager.channel" />
<argument>%__behat__.mink.parameters%</argument>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No 👍 😄

@Zales0123 Zales0123 force-pushed the buy-product-in-correct-price-for-channel-and-currency branch 2 times, most recently from 238d561 to 8322b7a Compare November 10, 2016 12:23
Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

Contains redundant commits (and "Merge branch 'master' of git://github.com/Sylius/Sylius" should be removed as well).

@Zales0123 Zales0123 force-pushed the buy-product-in-correct-price-for-channel-and-currency branch from 8322b7a to e9ba0f9 Compare November 10, 2016 13:09
@Zales0123
Copy link
Member Author

@pamil Not anymore 🐫

@michalmarcinkowski michalmarcinkowski merged commit 84521dc into Sylius:master Nov 11, 2016
@michalmarcinkowski
Copy link
Contributor

Thank you Mateusz! Please apply all comments in a separate PR.

@Zales0123 Zales0123 deleted the buy-product-in-correct-price-for-channel-and-currency branch March 29, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants