Fix missing discountable_type checks in Discount scope methods#2250
Fix missing discountable_type checks in Discount scope methods#2250fredmarmillod wants to merge 10 commits intolunarphp:1.xfrom
Conversation
- Added whereDiscountableType() checks to whereDoesntHave subqueries in scopeCollections, scopeProducts, and scopeProductVariants - Ensures proper filtering when discounts have mixed discountable types - Added comprehensive test coverage for all three scope methods - Tests verify correct behavior with mixed types and edge cases
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add discountable_type filtering to products() and productVariants() scopes - Ensures scopes only match discountables of the correct type - Add comprehensive tests covering the new type filtering behavior
Keep local implementation of collections scope tests using pivot table relationships
|
This pull request also fixes the scopeCollections. It was previously using the I also added a |
|
@ryanmitchell could I have your input on this one? |
|
I understand the issue, but I don't understand why some much of the code has been changed, for example using the prefix. Is there a reason why this PR has gone that far? For me it should just be a case of adding ->whereDiscounType() on the whereDoesntHave() subqueries. |
|
I started by just adding whereDiscountType on the whereDoesntHave and tested filtering with discounts having multiple limitations of different types but it triggered some mysql ambiguous column names error. That's why I added prefix. Then I noticed thaat brands and collection limitations are using |
|
hmm, then some of my work on #2214 was incorrect. I had assumed (seemingly incorrectly) that everything was stored in the The options are either:
To me (2) makes more sense as its already a morph table. |
|
@glennjacobs what approach would you prefer? |
|
@ryanmitchell Yes, that would be great, thanks |
I think we stick with the current tables, option 1. |
|
I've opened a PR building on this #2345 - so this one can be closed. |
|
Closing in preference of #2345 |
This PR builds on the work done in #2250 It: 1. Adds a brand check 2. Seperates collections into its own relation manager to allow it to continue to use collection_discount table It's breaking, but for v2 we should consolidate everything into the discountable table - I missed these when I did this initial piece of work. It would make the UI much more streamlined and from a data point of view reduce the redundancy of the tables. --------- Co-authored-by: fredmarmillod <[email protected]> Co-authored-by: Author <[email protected]> Co-authored-by: Frédéric Marmillod <[email protected]> Co-authored-by: Glenn Jacobs <[email protected]>
4 participants
Problem
The
scopeProducts,scopeProductVariants, andscopeCollectionsmethods in the Discount model were missingdiscountable_typechecks in theirwhereDoesntHave('discountables')subqueries. This could lead to incorrect filtering when discounts have mixed discountable types.For example, if a discount had Product discountables but a query was looking for Collection-related discounts, the scope might incorrectly include or exclude that discount because it wasn't properly filtering by the specific discountable type.
Solution
Added
whereDiscountableType()checks to thewhereDoesntHave('discountables')subqueries in all three scope methods:scopeCollections: Now properly filters forCollection::morphName()discountable typesscopeProducts: Now properly filters forProduct::morphName()discountable typesscopeProductVariants: Now properly filters forProductVariant::morphName()discountable typesChanges
packages/core/src/Models/Discount.php:
scopeCollections()method to include discountable type checkscopeProducts()method to include discountable type checkscopeProductVariants()method to include discountable type checktests/core/Unit/Models/DiscountTest.php:
scopeCollections()scopeProducts()scopeProductVariants()