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

Permalinks for taxons #2431

Merged
merged 1 commit into from
Feb 14, 2015
Merged

Permalinks for taxons #2431

merged 1 commit into from
Feb 14, 2015

Conversation

gonzalovilaseca
Copy link
Contributor

This fixes the permalink generation TODO in taxons.
I've basically added some customization to the TreeHander. I couldn't extend that class as it has private attributes, so I had to copy the whole class.

The handler is located in src/Sylius/Bundle/TranslationBundle/GedmoHandler, if there is a better place to put it just let me know.

@gonzalovilaseca gonzalovilaseca changed the title Permalinks for translation entities Permalinks for taxons Feb 4, 2015
// Comprobar que el parent del relation tb es single?
// if (!$meta->isSingleValuedAssociation($options['relationParentRelationField'])) {
// throw new InvalidMappingException("Unable to find tree parent slug relation through field - [{$options['relationParentRelationField']}] in class - {$meta->name}");
// }
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry..I didn't push the latest changes where I adressed this... :-)

@gonzalovilaseca
Copy link
Contributor Author

I'm going to have a look at the failing tests. Will update ASAP.

@gonzalovilaseca
Copy link
Contributor Author

@pjedrzejewski The failing test is this one:

    Scenario: Creating new taxon with existing name under given taxonomy
        Given I am on the page of taxonomy "Category"
          And I follow "Create taxon"
         When I fill in "Name" with "Electronics"
          And I press "Create"
         Then I should be on the page of taxonomy "Category"
          And I should see "Taxon has been successfully created."
          And I should see 9 taxons in the list

This test assumes we can have the same taxon name for two different taxons under the same taxonomy, that would mean two identic permalinks pointing to different taxons, and that is not correct, isn't it?

@michalmarcinkowski
Copy link
Contributor

@pjedrzejewski @gonzalovilaseca
I think we should allow for Taxons with the same name, but not under the same parent.

Valid scenario

Electronics > PC
Electronics > Featured
Clothes > T-Shirts
Clothes > Featured

Invalid scenario

Electronics > PC
Electronics > Featured
Clothes > T-Shirts
Clothes > T-Shirts*
Clothes > Featured

*should not be allowed

If we go this way, the failed scenario is invalid and should be replaced with similar examples as these above. In that case the permalink will do the validation job and we should only check for its uniqueness.

@gonzalovilaseca
Copy link
Contributor Author

That is how it's set up right now, that's why the test fails. I agree with you, let's see what pawel says.

@gonzalovilaseca
Copy link
Contributor Author

PR updated with tests passing. I added a test that needs to be implemented (I don't have the time right now).

pjedrzejewski pushed a commit that referenced this pull request Feb 14, 2015
@pjedrzejewski pjedrzejewski merged commit 9c792b2 into Sylius:master Feb 14, 2015
@pjedrzejewski
Copy link
Member

Thank you Gonzalo! 👍

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

3 participants