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

[taxon_fixtures] Fix child taxon slug generation #10191

Merged
merged 3 commits into from
Jun 13, 2019
Merged

[taxon_fixtures] Fix child taxon slug generation #10191

merged 3 commits into from
Jun 13, 2019

Conversation

tannyl
Copy link
Contributor

@tannyl tannyl commented Feb 22, 2019

Q A
Branch? 1.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets none
License MIT

@loevgaard suggested I made this PR for this problem we ran into with using the Taxon fixture to add many nested categories into our dev server.

When using the TaxonFixture to generate nested categories, with slugs being automatically generated based on name, the slugs generated for the child categories, will not inherit their parents slugs.

This is due to the way child categories are added in the TaxonExampleFactory. A child category is added to the parent category after the child category has had it's slug generated via the taxonSlugGenerator. Thus the generator does not know about the parent, when trying to generate a slug for the child category.

An side effect of this is that, you can't have child categories with the same name in different parent categories.

Take this configuration as an example

sylius_fixtures:
  suites:
    default:
      fixtures:
        taxon:
          options:
            custom:
              - code: 'category'
                name: 'Category'
                children:
                  - code: 'child_1'
                    name: 'ChildOne'
                    children:
                      - code: 'subchild_1'
                        name: 'SubChildOne'
                  - code: 'child_2'
                    name: 'ChildTwo'
                    children:
                      - code: 'subchild_2'
                        name: 'SubChildOne'

You would expect the following slugs:

category
category/childone
category/childone/subchildone
category/childtwo/
category/childtwo/subchildone

But instead you get this when using the above configuration:

category
childone
subchildone
childtwo
subchildone

This results in the Taxon fixture just silently failing, because two of the taxons/categories have the same slug (subchildone) = no taxons are added to the database.

My suggestion for a fix, is to change when the child is added to the parent category, so it happens before the slug for the child is generated.

Moved the code that generates the taxons into createTaxon, so we can supply the parentTaxon as an argument, and add it to the child taxon right after creation. Otherwise $this->taxonSlugGenerator->generate() won't know that this taxon is the child of a parent, and slugs wont be generatored correctly (you get "final_category" instead of "category/middle_category/final_category")
@tannyl tannyl requested a review from a team as a code owner February 22, 2019 11:11
@Zales0123 Zales0123 added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Apr 3, 2019
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

Seems like a good change 👍 We definitely need some tests for that. First of all, I would add some spec in TaxonExampleFactorySpec. I would also squash commits, as 3 of them for one file changed is a little bit too much :)

Second, maybe we should extend our basic fixtures to have such a multi-level taxons structure? They're loaded before running tests on Travis to make sure they do not fail with some 500 error. Right now taxon fixtures look like this:

CATEGORY
    MUGS
    STICKERS
    BOOKS
    T-SHIRTS
        MAN
        WOMAN

They for sure could be a little bit more sophisticated, maybe something like:

CATEGORY
    MUGS
    STICKERS
    BOOKS
    T-SHIRTS
        MAN
            SYLIUS
            PHP
            OTHERS
        WOMAN
            SYLIUS
            PHP
            OTHERS

??? I don't know if it's a change for this or for a sperate PR, I would really like to see the opinion of the rest of the @Sylius/core-team 🏇 If no, then maybe some PHPUnit test for the TaxonExampleFactory would prevent us sufficiently from the regression?

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

I would say, that we should do it in one shot, as it will be a test for this change. So Tanny, could you customize our fixtures, to use your new feature?

Anyway, welcome to our community :)

@Zales0123
Copy link
Member

Hi @tannyl, will you be to continue work on this PR? It would be great to have it merged :)

@tannyl
Copy link
Contributor Author

tannyl commented May 16, 2019

@Zales0123 Yes, I will look at it again. Just been very busy at work (where we use Sylius ;) ) so I haven't had time to look at it again.

@pamil pamil merged commit 7c4f629 into Sylius:1.4 Jun 13, 2019
@pamil
Copy link
Contributor

pamil commented Jun 13, 2019

Thank you, Tanny! 🥇

@tannyl tannyl deleted the tannyl-sylius-taxonsfixtures-slug-fix branch June 15, 2019 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants