[ProductAttribute] Fix displaying attributes depending on locales and channels #8775

GSadee · 2017-10-05T07:49:50Z

Q	A
Branch?	1.0
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	partially fixes #8220
License	MIT

pamil

Can you add some unit tests for getAttributesByLocale method? It feels complicated enough not to rely only on Behat.

GSadee · 2017-10-10T10:35:32Z

@pamil I've added some specs for test this behaviour

pamil · 2017-10-10T12:51:35Z

src/Sylius/Component/Product/Model/Product.php

+            return false;
+        }
+
+        if (empty($attributeValue->getValue())) {


I guess we do not want it to be an empty call.

The following things are considered to be empty:

"" (an empty string)

0 (0 as an integer)

0.0 (0 as a float)

"0" (0 as a string)

NULL

FALSE

array() (an empty array)

$var; (a variable declared, but without a value)

I'm afraid we're missing the tests for attributes that have values existing in base locale, but these values are considered empty (especially for booleans or integers).

Like with base locale fr_FR and current locale pl_PL and boolean attribute, what happens if:

boolean attribute has false value in fr_FR but true in pl_PL

integer attribute has 0 value in fr_FR but 1 in pl_PL

pamil · 2017-10-10T12:55:58Z

src/Sylius/Component/Product/spec/Model/ProductSpec.php

+        $this
+            ->getAttributesByLocale('pl_PL', 'en_US')
+            ->toArray()
+            ->shouldReturn([$attributeValuePL->getWrappedObject()])


Let's add another one which is testing the same call but returning the attribute from the fallback locale.

pamil · 2017-10-10T12:58:00Z

src/Sylius/Component/Product/spec/Model/ProductSpec.php

+        $this
+            ->getAttributesByLocale('pl_PL', 'en_US', 'fr_FR')
+            ->toArray()
+            ->shouldReturn([$attributeValueEN->getWrappedObject()])


Let's add another ones which are testing the same call but:

returning the attribute from $localeCode locale

returning the attribute from $baseLocaleCode locale

pamil · 2017-10-10T13:00:05Z

src/Sylius/Component/Product/spec/Model/ProductSpec.php

+        $this
+            ->getAttributesByLocale('pl_PL', 'en_US')
+            ->toArray()
+            ->shouldReturn([$attributeValuePL->getWrappedObject()])


Btw. instead of $this->getAttributesByLocale(...)->toArray()->shouldReturn([$mock->getWrappedObject()]); you can use $this->getAttributesByLocale(...)->shouldIterateAs([$mock]);

pamil · 2017-10-16T11:55:13Z

UPGRADE-1.0.md

@@ -1,3 +1,11 @@
+# UPGRADE FROM 1.0.0 to 1.0.1


There's no additional action needed, so I think it's not needed to mention that (and it's 1.0.1 -> 1.0.2 now anyway)

…ales and channels

… channels

pamil · 2017-10-17T08:54:34Z

I've been thinking for some time whether to classify this as a bug fix or a new feature, but it looks like it is the latter. getAttributeByCodeAndLocale behaviour stays the same if the third parameter is not passed.

What do you think about targeting master branch with this PR? Some parts can be still merged to 1.0, like the first two Behat scenarios and specs that does not call that method with the thrid parameter, but it's not necessary.

GSadee · 2017-10-17T09:14:27Z

@pamil IMO this is a bug fix, because even if the behaviour of getAttributeByCodeAndLocale has not been changed, adding the third parameter in twig template on shop fixes the behaviour of displaying attributes there.

And these behat scenarios cannot be merge to 1.0 without changing the getAttributeByCodeAndLocale, because they will fail 💥

pamil · 2017-10-18T08:59:16Z

From my POV the current attributes behaviour is like for any other translatable resources: try to fetch a translation for current locale, if not found - try fallback locale. This PR adds a new feature, which is including also attributes in base locale.

As for Behat, the first two scenarios should pass, the other two are describing new features.

Anyway, it might be helpful to hear some other opinions than ours :)

GSadee · 2017-10-18T10:47:44Z

I can't 100% agree because this PR fixes the scenario:

the base locale is en_US
the product has attribute with values in en_US and pl_PL locales
this product is in channel with default locale fr_FR
in the shop, product is displaying in locale pl_PL

Before: the attribute woldn't be shown at all and it isn't a standard behaviour for translatable entities for me :)
After: the attribute would be shown in pl_PL

lchrusciel

It seems for me to be rather a bug fix, but as @pjedrzejewski said in #8220 (comment) we should probably merge it to the master

lchrusciel · 2017-10-18T10:30:00Z

...roduct/viewing_products/viewing_product_attributes_in_different_locales_and_channels.feature

+        And I should also see the product attribute "T-shirt details" with value "Banana is a very good material."
+
+    @ui
+    Scenario: Viewing a detailed page with product's attribute for current channel in different locale


The name of this scenario should have it is name changed with the scenario above. This one is using a default locale

lchrusciel · 2017-10-18T10:34:41Z

src/Sylius/Component/Attribute/Model/AttributeSubjectInterface.php

+    public function getAttributesByLocale(
+        string $localeCode,
+        string $fallbackLocaleCode,
+        ?string $baseLocaleCode = null


Maybe we could start adding TODO messages with the information, that from 2.0 the third parameter would be mandatory?

If we intend to make it required then E_USER_DEPRECATED error should be triggered if value was not passed. To be honest, I'd rather make fallback locale code optional as well (there's no method to get product attributes values in given locale only now).

pamil · 2017-10-18T12:06:11Z

Thanks @GSadee for your patience! ☮️

GSadee added the Bug Fix label Oct 5, 2017

GSadee added this to the 1.0 milestone Oct 5, 2017

GSadee mentioned this pull request Oct 5, 2017

RFC - Translatable atribute values behavior #8220

Closed

GSadee force-pushed the diplaying-attributes-in-channels branch from d222d21 to 8c45747 Compare October 6, 2017 10:42

pamil suggested changes Oct 10, 2017

View reviewed changes

GSadee force-pushed the diplaying-attributes-in-channels branch from 8c45747 to e552c6b Compare October 10, 2017 10:34

pamil suggested changes Oct 10, 2017

View reviewed changes

GSadee force-pushed the diplaying-attributes-in-channels branch from e552c6b to 29c6dd0 Compare October 10, 2017 20:43

pamil approved these changes Oct 16, 2017

View reviewed changes

pamil reviewed Oct 16, 2017

View reviewed changes

GSadee added 3 commits October 16, 2017 15:13

[Behat] Add scenarios for viewing product attributes in different loc…

ed48e56

…ales and channels

[ProductAttribute] Fix displaying attributes depending on locales and…

7210861

… channels

[Product] Add specs for getting attributes by locale

cb931cd

GSadee force-pushed the diplaying-attributes-in-channels branch from 29c6dd0 to cb931cd Compare October 16, 2017 13:13

GSadee requested review from Zales0123 and lchrusciel October 18, 2017 10:49

lchrusciel approved these changes Oct 18, 2017

View reviewed changes

pamil merged commit 1bbf5e0 into Sylius:1.0 Oct 18, 2017

GSadee deleted the diplaying-attributes-in-channels branch October 18, 2017 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ProductAttribute] Fix displaying attributes depending on locales and channels #8775

[ProductAttribute] Fix displaying attributes depending on locales and channels #8775

GSadee commented Oct 5, 2017 •

edited

Loading

pamil left a comment

GSadee commented Oct 10, 2017

pamil Oct 10, 2017

pamil Oct 10, 2017

pamil Oct 10, 2017

pamil Oct 10, 2017

pamil Oct 10, 2017

pamil Oct 16, 2017

pamil commented Oct 17, 2017

GSadee commented Oct 17, 2017

pamil commented Oct 18, 2017 •

edited

Loading

GSadee commented Oct 18, 2017 •

edited

Loading

lchrusciel left a comment

lchrusciel Oct 18, 2017

lchrusciel Oct 18, 2017

pamil Oct 18, 2017

pamil commented Oct 18, 2017

[ProductAttribute] Fix displaying attributes depending on locales and channels #8775

[ProductAttribute] Fix displaying attributes depending on locales and channels #8775

Conversation

GSadee commented Oct 5, 2017 • edited Loading

pamil left a comment

Choose a reason for hiding this comment

GSadee commented Oct 10, 2017

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

pamil commented Oct 17, 2017

GSadee commented Oct 17, 2017

pamil commented Oct 18, 2017 • edited Loading

GSadee commented Oct 18, 2017 • edited Loading

lchrusciel left a 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

pamil commented Oct 18, 2017

GSadee commented Oct 5, 2017 •

edited

Loading

pamil commented Oct 18, 2017 •

edited

Loading

GSadee commented Oct 18, 2017 •

edited

Loading