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

#9699 Fix for viewing products when they belong to a taxon and to one… #10070

Merged
merged 2 commits into from
Jan 13, 2019

Conversation

laurent35240
Copy link
Contributor

… of children taxon

Q A
Branch? 1.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #9699
License MIT

@laurent35240 laurent35240 requested a review from a team as a code owner January 5, 2019 21:28
@CoderMaggie CoderMaggie added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Jan 7, 2019
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.

But can we also add some products with translations?

@laurent35240
Copy link
Contributor Author

@lchrusciel Is it in the behat scenario that you'd like to have a product with translation?

@bartoszpietrzak1994
Copy link
Contributor

@laurent35240 I think that is what @lchrusciel meant. Maybe we should check if adding the distinct clause does not affect joining the translations table in any way.

@lchrusciel
Copy link
Member

Yup. I prefer to ensure, that we are not adding any regression. However, there should be some tests for that anyway.

@@ -70,6 +70,7 @@ public function createShopListQueryBuilder(
array $sorting = []
): QueryBuilder {
$queryBuilder = $this->createQueryBuilder('o')
->distinct()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's so simple when you see it, but couldn't figure it out when I was trying to solve this issue, good job 🎉

@pamil pamil merged commit ce290b0 into Sylius:1.2 Jan 13, 2019
@pamil
Copy link
Contributor

pamil commented Jan 13, 2019

Thank you, Laurent! 🎉

lchrusciel added a commit to lchrusciel/Sylius that referenced this pull request Jan 31, 2019
lchrusciel added a commit that referenced this pull request Jan 31, 2019
This PR was merged into the 1.2 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.2
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | introduced in #10070, replaces #10114 and #10134, fixes #7389
| License         | MIT

Based on a PR by @yohang (thank you for your work!). The test was failing, as there is another bug with setting max results with paginator (that's why I've reduced the amount of products in the scenario) 🚀 😄 But the 500 error bug is fixed (I've also checked it manually).

Commits
-------

e229211 Added productTaxon to product list query to prevent an SQL error
261236e Quick fix for the scenario
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.

5 participants