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

[ProductAssociation] Show product associations within current channel only #14600

Conversation

coldic3
Copy link
Contributor

@coldic3 coldic3 commented Dec 1, 2022

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

@coldic3 coldic3 added Shop ShopBundle related issues and PRs. Bug Confirmed bugs or bugfixes. labels Dec 1, 2022
@coldic3 coldic3 changed the base branch from 1.13 to 1.11 December 1, 2022 10:31
@coldic3 coldic3 force-pushed the fix/product-associations-within-current-channel-only branch from f43ff14 to 04ecb6e Compare December 1, 2022 10:32
Copy link
Contributor

@NoResponseMate NoResponseMate left a comment

Choose a reason for hiding this comment

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

It seems we're missing a scenario where the associated product is available in a channel, but disabled.

@coldic3 coldic3 force-pushed the fix/product-associations-within-current-channel-only branch from 04ecb6e to 997e72d Compare December 6, 2022 14:40
@coldic3 coldic3 marked this pull request as ready for review December 6, 2022 14:44
@coldic3 coldic3 requested a review from a team as a code owner December 6, 2022 14:44
@coldic3 coldic3 force-pushed the fix/product-associations-within-current-channel-only branch from 1e36dc1 to 6a7b453 Compare December 7, 2022 11:33
@coldic3 coldic3 changed the title [ProductAssociation] Show product associations within current channelonly [ProductAssociation] Show product associations within current channel only Dec 7, 2022
@GSadee GSadee merged commit 35b642f into Sylius:1.11 Dec 8, 2022
@GSadee
Copy link
Member

GSadee commented Dec 8, 2022

Thank you, Kevin! 🥇

@lruozzi9
Copy link
Contributor

These changes contain a bug 😱!! The findWithProductsWithinChannel method should be allowed to return null:
public function findWithProductsWithinChannel($associationId, ChannelInterface $channel): ?ProductAssociationInterface;

This is also suggested by the getOneOrNullResult() method in the Doctrine implementation.

@coldic3
Copy link
Contributor Author

coldic3 commented Dec 14, 2022

@lruozzi9 Thanks for catching it! Indeed returning null should be allowed. Interesting why static analysis didn't catch it 🤔

@lruozzi9
Copy link
Contributor

@coldic3 that's not the only problem. I now get the NotFoundHttpException from the findOr404 method while getting product associated from an empty association. Probably there is some error in the construction of the query.

@coldic3
Copy link
Contributor Author

coldic3 commented Dec 14, 2022

@lruozzi9 I see it! I did inner join instead of left join 🤦‍♂️ At first glance ProductAssociationRepository::findWithProductsWithinChannel should always return ProductAssociationInterface because we pass an ID as the first argument so I guess there is no way for returning null if the ID is correct. However, at least some assertion is needed.

@lruozzi9
Copy link
Contributor

Right! 🤓😉

GSadee added a commit that referenced this pull request Dec 16, 2022
This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.11 |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no |
| Related tickets | fixes #14600                      |
| License         | MIT                                                          |

See the discussion: #14600 (comment)

Commits
-------

1cc8953 [ProductAssociation] Fix empty product associations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Associated Products loaded without proofing if enabled for current channel
4 participants