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

Fix taxon repository #7437

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Conversation

Pitoune
Copy link

@Pitoune Pitoune commented Feb 7, 2017

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

@@ -88,7 +88,7 @@ public function findRootNodes()
*/
public function findNodesTreeSorted($rootCode = null)
{
return $this->createQueryBuilder('o')
$queryBuilder = $this->createQueryBuilder('o')
Copy link
Contributor

@pamil pamil Feb 7, 2017

Choose a reason for hiding this comment

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

$queryBuilder = $this->createQueryBuilder('o')
    // ...
    ->getQuery()
    ->getResult()
;

Does not look like a fix. 🌮

Copy link
Author

Choose a reason for hiding this comment

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

My bad...

@pamil
Copy link
Contributor

pamil commented Feb 7, 2017

Anyway, it would be nice to write some tests for this repository, it seems to be a quite tricky part of code :)

@Pitoune
Copy link
Author

Pitoune commented Feb 7, 2017

Working on it ;)

@Pitoune
Copy link
Author

Pitoune commented Feb 8, 2017

I did take a look at the tests suite and I am still wondering where to put my test.
@pamil any recommendation ?

@Pitoune Pitoune force-pushed the fix-taxon-repository branch 5 times, most recently from 1cc4b72 to 7686e37 Compare March 13, 2017 08:51
@Pitoune
Copy link
Author

Pitoune commented Mar 13, 2017

@pamil I finally got time to add some tests !

@pamil
Copy link
Contributor

pamil commented Mar 13, 2017

Awesome, should be much easier to add them now :)

@Pitoune
Copy link
Author

Pitoune commented Mar 13, 2017

I have added a backend ajax root to retrieve taxons from the root code. If I need it I guess some others would too.

@pjedrzejewski pjedrzejewski added this to the v1.0.0-beta.2 milestone Mar 13, 2017
@pamil
Copy link
Contributor

pamil commented Mar 14, 2017

Can you revert last changes and separate this bug fix from adding a new feature, please? This way we can make the repository method correct and discuss the new feature in another PR.

@Pitoune
Copy link
Author

Pitoune commented Mar 14, 2017

I am totally OK to do it but without the new feature I cannot find a way to test the findNodesTreeSorted method. Is that OK for you ?

@pamil
Copy link
Contributor

pamil commented Mar 14, 2017

All right, it's the first repository integration test that we want to introduce, so I can write it instead.

@Pitoune
Copy link
Author

Pitoune commented Mar 14, 2017

OK so revert is done ;)

@pjedrzejewski pjedrzejewski merged commit 50a207a into Sylius:master Mar 14, 2017
@pjedrzejewski
Copy link
Member

Thank you Pierre! 👍

@Pitoune Pitoune deleted the fix-taxon-repository branch June 23, 2017 10:27
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.

[BUG] TaxonRepository::findNodesTreeSorted duplicate return.
3 participants