[Feature] Toggleable taxon #11287

mmenozzi · 2020-03-25T08:36:20Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no?
Deprecations?	no
Related tickets	implement #8785
License	MIT

Enabled property
Adjustment of repository methods to filter for enabled / maybe new methods to get all (also disabled)
Showing in admin that a taxon is disabled in taxon create/update page
Showing in admin that a taxon is disabled in products list page

mmenozzi · 2020-05-05T06:50:25Z

Hi @pamil and @lchrusciel, I don't know why but this PR won't trigger the Travis CI build. Do you know why? Anyway it's ready for review.

LucaGallinari · 2020-05-15T08:07:08Z

Hi @pamil and @lchrusciel this PR is ready for review

Zales0123

Sorry for the long response time @mmenozzi, it's a nice piece of code 👌

Zales0123 · 2020-05-22T10:44:55Z

features/homepage/viewing_only_enabled_taxons_in_taxon_menu.feature

@@ -0,0 +1,29 @@
+@homepage
+Feature: Viewing only enabled taxons in taxon menu
+    In order to not access disabled taxons


In order to browse only available categories? We should think about the benefit or the Visitor/Customer. They do no think about disabled/enabled entities, but about the merch, they see in the shop :)

Ok, good point. I am ok in changing this only in the that specific line of the feature definition. Because i have some doubts in rewording this in the other parts of this feature: the taxon availability is not only related to the fact that it is enabled but even to the fact that it should be associated to the current channel. This features only test enabled/disabled taxon and not their belonging to a specific channel.
I don't know if i expressed correctly what i am thinking =)

Zales0123 · 2020-05-22T10:47:08Z

features/homepage/viewing_only_enabled_taxons_in_taxon_menu.feature

+        Then I should see "Accessories Caps" and "Caps" in the menu
+        And I should not see "Clothes", "T-Shirts", "Coats" and "Belts" in the menu
+
+    @api


Move this tag to the previous step (@ui @api) and we save some lines 💃

One of the step is sligthly different. Before i pushed the changes i had put them together in a single scenario but test failed at step 'Then I should see "Accessories Caps" and "Caps" in the menu' cause @ui and @api seems to fill the test attributes of the menu differently. For a parent category the @ui prints "[Parent name] [List of children names]", instead @api prints "[Parent name]". I don't think this should be solved with this PR.

Zales0123 · 2020-05-22T10:49:41Z

features/homepage/viewing_only_enabled_taxons_in_taxon_menu.feature

+        Given the "Clothes" taxon is disabled
+        And the "Belts" taxon is disabled
+        When I check available taxons
+        Then I should see "Accessories Caps" and "Caps" in the menu


We should use as little context-related words as possible (especially as we want to use this scenario both for UI and API). So maybe something like Then taxons "Accessories Caps" and "Caps" should be available?

I followed this file features/homepage/viewing_only_taxons_from_menu_taxon.feature and it uses "i should see"

Zales0123 · 2020-05-22T10:50:32Z

features/product/viewing_products/accessing_disabled_taxon.feature

+    Background:
+        Given the store operates on a single channel in "United States"
+
+    @ui


We could also make it in API - check do we get 404 on some specific request

A disabled taxon should still be available through API. Can you show some example for this case? Only a not existent taxon should lead to a 404 on API calls

From the shop perspective disabled taxon should not be available I suppose. However, the admin user should still have access to it

Yeah, you are totally right, dunno why i thought of it as an admin API

Hi @lchrusciel, did you mean that we should open a separate PR for the Sylius/ShopApiPlugin? Am I right?

Basically a PR to change here:

https://github.com/Sylius/ShopApiPlugin/blob/master/src/Controller/Taxon/ShowTaxonDetailsAction.php#L47

with something like:

$taxon = $this->taxonRepository->findOneBy(['code' => $code, 'enabled' => true]);

Let's split the problem into two parts.

First of all this repo: I just meant, that the current scenario makes sense and should be covered with UI. What (I think) @Zales0123 meant was about our new implementation of integration with Api Platform.

Secondly, Shop API is a totally different topic and can be covered after 1.8 release :)

features/product/viewing_products/accessing_disabled_taxon.feature

src/Sylius/Behat/Page/Admin/Taxon/UpdatePage.php

src/Sylius/Bundle/ShopBundle/Resources/views/Taxon/_horizontalMenu.html.twig

Zales0123 · 2020-05-22T11:18:42Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonType.php

@@ -32,6 +33,10 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
                'entry_type' => TaxonTranslationType::class,
                'label' => 'sylius.form.taxon.name',
            ])
+            ->add('enabled', CheckboxType::class, [


Shouldn't it be true as default?

I followed what has been done for the enabled field in other entities and it is as this: no default value (as it true/checked as default) and required=>false. I am ok in changing this btw

Zales0123 · 2020-05-22T11:19:01Z

src/Sylius/Bundle/TaxonomyBundle/Resources/config/doctrine/model/Taxon.orm.xml

@@ -56,6 +56,8 @@
        </field>

        <gedmo:tree type="nested" />
+
+        <field name="enabled" column="enabled" type="boolean" />


redundant column attribute

But we have the practice to leave them in Core

src/Sylius/Bundle/TaxonomyBundle/Resources/translations/messages.en.yml

LucaGallinari · 2020-05-27T07:41:16Z

Hi @lchrusciel the build does not trigger continuous-integration/travis-ci/pr. It happened even few weeks ago and you resolved by closing and reopening, can you do the same?

lchrusciel · 2020-05-27T09:17:59Z

of course ;)

LucaGallinari · 2020-06-08T08:46:54Z

I made the requested changes and commented others. Ready to be reviewed again

LucaGallinari · 2020-06-18T10:17:55Z

Hi guys @lchrusciel @pamil @Zales0123 , I made the requested changes and commented others. This PR is ready to be reviewed again.
The build is failing on some features but it doesn't seem to be related to my changes. It's strange that it's green for 7.4 and not for 7.3

…sabled

…ferently

mmenozzi · 2020-07-06T09:43:00Z

Hi @lchrusciel @pamil @Zales0123, I resolved a conflict in this PR related to the inheritdoc removal.
I think that this PR is ready to merge.
It would be great if we could have this feature in Sylius 1.8.

Let me know if there's something I can do to help.

lchrusciel · 2020-07-09T12:46:31Z

features/product/viewing_products/accessing_disabled_taxon.feature

+    Background:
+        Given the store operates on a single channel in "United States"
+
+    @ui


From the shop perspective disabled taxon should not be available I suppose. However, the admin user should still have access to it

lchrusciel · 2020-07-09T13:24:23Z

Thanks a lot, Manuele and Luca! 🎉 It was a lot of work but also quite a lot of waiting for us. Sorry about that. However, it is finally there and the whole community will take advantage of it after the next release ;)

Also, can you apply changes from #11287 (comment) in separate PR?

mmenozzi · 2020-07-10T06:07:34Z

Thanks a lot, Manuele and Luca! 🎉 It was a lot of work but also quite a lot of waiting for us. Sorry about that. However, it is finally there and the whole community will take advantage of it after the next release ;)

Thanks to you and the whole Sylius team! I'm happy that this feature will be available in Sylius 1.8. 🎉

Also, can you apply changes from #11287 (comment) in separate PR?

Sure, will do it ASAP!

LucaGallinari · 2020-07-10T10:33:49Z

Thank you guys!

loevgaard · 2020-08-18T19:46:59Z

Great work, guys! Thank you!

probot-autolabeler bot added Admin AdminBundle related issues and PRs. Shop ShopBundle related issues and PRs. labels May 4, 2020

mmenozzi marked this pull request as ready for review May 4, 2020 20:07

mmenozzi requested a review from a team as a code owner May 4, 2020 20:07

lchrusciel closed this May 5, 2020

lchrusciel reopened this May 5, 2020

LucaGallinari force-pushed the toggleable-taxon branch from c776328 to 79d0a9d Compare May 12, 2020 08:37

Zales0123 reviewed May 22, 2020

View reviewed changes

pamil mentioned this pull request May 24, 2020

Toggleable product variant #11241

Merged

LucaGallinari force-pushed the toggleable-taxon branch from 4896ed6 to 0c76f62 Compare May 26, 2020 15:29

lchrusciel closed this May 27, 2020

lchrusciel reopened this May 27, 2020

LucaGallinari force-pushed the toggleable-taxon branch 2 times, most recently from 8e2f97e to a2e5ab2 Compare June 3, 2020 07:28

LucaGallinari force-pushed the toggleable-taxon branch 2 times, most recently from 3f6d255 to 12b3e88 Compare June 18, 2020 09:39

mmenozzi and others added 8 commits July 6, 2020 11:25

Add toggleable to taxon and first behat scenario

19e7e26

Fix TaxonApiTest to include new enabled field

6ac8751

Enable all taxons upon migration

acb183a

Improves managing taxons disable/enable feature

3bd9b1c

Show taxon is disabled in manage taxon pages and product list page

829e294

Do not show taxon page if it is disabled

7052da5

WIP do not show disabled categories on horizontal menu

45adc66

Properly test that menu do not show disabled categories

e2f1e7a

LucaGallinari and others added 16 commits July 6, 2020 11:25

Do not show disabled taxons in vertical menu

0764475

Moves vertical menu element to proper context namespace

46ae778

Do not show go level up link on taxon vertical menu when parent is di…

4f16b51

…sabled

Do not link to disabled taxons on taxon breadcrumb

7d06a61

Fixes renamed step for homepage taxons

8fe0878

Rename feature and fix typo

105f7ed

Add missing steps for homepage api context

512ad55

Check all existing and missing taxons in menu

fe45cd1

Use 2 different scenario for api and ui as they check menu taxons dif…

a6b1dc6

…ferently

Refactor for entries alphabetical order

8b73b71

Features rewording

15468c7

Adds void return type to behat contexts methods

d7801d4

Remove not required persists

4bc00f9

Same instead of equal in behat ProductContext

13b3c03

Can retrieve enabled children taxons from taxon

036c7b7

Remove inheritdoc directive from Taxon::getEnabledChildren method

c51226e

mmenozzi force-pushed the toggleable-taxon branch from 12b3e88 to c51226e Compare July 6, 2020 09:36

lchrusciel approved these changes Jul 9, 2020

View reviewed changes

lchrusciel merged commit b643117 into Sylius:master Jul 9, 2020

mmenozzi mentioned this pull request Aug 18, 2020

New Feature - toggleable taxons #8785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Toggleable taxon #11287

[Feature] Toggleable taxon #11287

mmenozzi commented Mar 25, 2020 •

edited

Loading

mmenozzi commented May 5, 2020

LucaGallinari commented May 15, 2020

Zales0123 left a comment

Zales0123 May 22, 2020

LucaGallinari May 25, 2020

Zales0123 May 22, 2020

LucaGallinari May 25, 2020

Zales0123 May 22, 2020

LucaGallinari May 25, 2020

Zales0123 May 22, 2020

LucaGallinari Jun 8, 2020

lchrusciel Jul 9, 2020

LucaGallinari Jul 10, 2020

mmenozzi Jul 11, 2020

lchrusciel Jul 18, 2020

Zales0123 May 22, 2020

LucaGallinari May 26, 2020

Zales0123 May 22, 2020

lchrusciel May 26, 2020

LucaGallinari commented May 27, 2020

lchrusciel commented May 27, 2020

LucaGallinari commented Jun 8, 2020

LucaGallinari commented Jun 18, 2020

mmenozzi commented Jul 6, 2020

lchrusciel Jul 9, 2020

lchrusciel commented Jul 9, 2020 •

edited

Loading

mmenozzi commented Jul 10, 2020

LucaGallinari commented Jul 10, 2020

loevgaard commented Aug 18, 2020

[Feature] Toggleable taxon #11287

[Feature] Toggleable taxon #11287

Conversation

mmenozzi commented Mar 25, 2020 • edited Loading

mmenozzi commented May 5, 2020

LucaGallinari commented May 15, 2020

Zales0123 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaGallinari commented May 27, 2020

lchrusciel commented May 27, 2020

LucaGallinari commented Jun 8, 2020

LucaGallinari commented Jun 18, 2020

mmenozzi commented Jul 6, 2020

Choose a reason for hiding this comment

lchrusciel commented Jul 9, 2020 • edited Loading

mmenozzi commented Jul 10, 2020

LucaGallinari commented Jul 10, 2020

loevgaard commented Aug 18, 2020

mmenozzi commented Mar 25, 2020 •

edited

Loading

lchrusciel commented Jul 9, 2020 •

edited

Loading