Allow BuyXGetY discount conditions to be collection or variant, rewards to be variant#2214
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
glennjacobs
left a comment
glennjacobs
left a comment
There was a problem hiding this comment.
@ryanmitchell I'm not against this idea, in fact I can see it would be handy, but the implementation feels wrong at the moment.
The relation table is called discount_purchasables and so you'd expect the entries to be a purchasable. To now start putting non-purchasables in is confusing.
I'd probably do the database tables differently now, with perhaps discount_conditions & discounts_rewards... it would need some proper thought, but it doesn't help us right now. That sort of change would have to be done in v2.
Can you have a think about how we can implement this, but keep things clean?
|
Yeah I agree. Currently there are 4 types of 'purchasable' - conditions, rewards, exclusions and limitations, so that would mean 4 tables, and probably more as other discount types are made. Keeping one table makes more sense to me and is more flexible when it's just the 'type' changing each time. Some suggestions for what we could rename
or just simply: Do any of these sit well with you? |
|
Yeah, discountables I think would work. Would you be willing to make the change in this PR? |
|
Yep absolutely, leave it with me and I'll get it updated today. |
|
@glennjacobs thats updated. theres a failing test but its not related to this code - seem to be a random failure. |
|
@alecritson you happy with this route? |
From reading it seems sound, have you pulled down and tried it out in the panel? |
|
If you're happy with this direction of travel I'd love to go a step further and allow variants to be selected on Buy X get Y too. |
Sure, I don't see any issue with that. |
ryanmitchell
changed the title
|
thanks @glennjacobs thats been done |
WalkthroughThis update refactors discount-related models, relationships, and documentation by renaming "DiscountPurchasable" to "Discountable," affecting factories, migrations, interfaces, and relationship methods. The changes broaden discount applicability to include products, variants, and collections. Admin UI language, resource managers, and test cases are updated to use the new terminology and logic. Supporting code and migrations reflect these structural adjustments. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (1)
79-81: Potential N+1 query issue acknowledged.As discussed in previous reviews, the ThumbnailImageColumn may cause N+1 queries due to different media relations across models. The team has accepted this trade-off for now.
🧹 Nitpick comments (3)
packages/core/src/Models/ProductVariant.php (1)
221-224: LGTM! Consider implementing HasThumbnailImage interface.The method implementation is correct and follows the same pattern as
ProductandCollectionmodels. However, consider explicitly implementing theHasThumbnailImageinterface in the class declaration for better contract compliance.-class ProductVariant extends BaseModel implements Contracts\ProductVariant, Purchasable +class ProductVariant extends BaseModel implements Contracts\ProductVariant, Purchasable, \Lunar\Base\HasThumbnailImagepackages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php (1)
73-81: Consider improving the search for ProductVariant.The current implementation searches variants by SKU prefix. Consider also searching by product name for better user experience.
Forms\Components\MorphToSelect\Type::make(ProductVariant::modelClass()) ->titleAttribute('sku') ->getSearchResultsUsing(static function (Forms\Components\Select $component, string $search): array { return get_search_builder(ProductVariant::modelClass(), $search) - ->orWhere('sku', 'like', $search.'%') + ->orWhere('sku', 'like', '%'.$search.'%') + ->orWhereHas('product', fn($q) => $q->whereRaw('JSON_UNQUOTE(JSON_EXTRACT(attribute_data, "$.name.value.en")) LIKE ?', ['%'.$search.'%'])) ->get() ->mapWithKeys(fn (ProductVariantContract $record): array => [$record->getKey() => $record->product->attr('name').' - '.$record->sku]) ->all(); }),packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (1)
62-69: Remove incorrect titleAttribute for ProductVariant.ProductVariant doesn't have a 'name.en' attribute. Since
getSearchResultsUsinghandles the search logic, the titleAttribute can be omitted.Forms\Components\MorphToSelect\Type::make(ProductVariant::modelClass()) - ->titleAttribute('name.en') ->getSearchResultsUsing(static function (Forms\Components\Select $component, string $search): array { return get_search_builder(ProductVariant::modelClass(), $search) ->get() ->mapWithKeys(fn (ProductVariantContract $record): array => [$record->getKey() => $record->sku]) ->all(); }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
docs/core/reference/discounts.md(1 hunks)docs/core/upgrading.md(1 hunks)packages/admin/resources/lang/en/discount.php(2 hunks)packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php(4 hunks)packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductLimitationRelationManager.php(3 hunks)packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php(4 hunks)packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductVariantLimitationRelationManager.php(3 hunks)packages/admin/src/Support/Tables/Columns/ThumbnailImageColumn.php(1 hunks)packages/admin/src/helpers.php(4 hunks)packages/core/database/factories/DiscountPurchasableFactory.php(0 hunks)packages/core/database/factories/DiscountableFactory.php(1 hunks)packages/core/database/migrations/2024_03_15_100000_remap_polymorphic_relations.php(1 hunks)packages/core/database/migrations/2025_06_18_100000_rename_discount_purchasables_table.php(1 hunks)packages/core/src/Base/HasThumbnailImage.php(1 hunks)packages/core/src/DiscountTypes/AmountOff.php(1 hunks)packages/core/src/DiscountTypes/BuyXGetY.php(4 hunks)packages/core/src/Managers/DiscountManager.php(2 hunks)packages/core/src/Models/Collection.php(3 hunks)packages/core/src/Models/Contracts/Discount.php(1 hunks)packages/core/src/Models/Contracts/Discountable.php(1 hunks)packages/core/src/Models/Discount.php(4 hunks)packages/core/src/Models/DiscountCollection.php(2 hunks)packages/core/src/Models/Discountable.php(5 hunks)packages/core/src/Models/Product.php(3 hunks)packages/core/src/Models/ProductVariant.php(1 hunks)packages/core/src/Observers/DiscountObserver.php(1 hunks)tests/core/Stubs/Models/DiscountPurchasable.php(0 hunks)tests/core/Stubs/Models/Discountable.php(1 hunks)tests/core/Unit/DiscountTypes/AmountOffTest.php(4 hunks)tests/core/Unit/DiscountTypes/BuyXGetYTest.php(7 hunks)
💤 Files with no reviewable changes (2)
- tests/core/Stubs/Models/DiscountPurchasable.php
- packages/core/database/factories/DiscountPurchasableFactory.php
🧰 Additional context used
🧬 Code Graph Analysis (15)
packages/core/src/Base/HasThumbnailImage.php (3)
packages/core/src/Models/Collection.php (1)
getThumbnailImage(153-156)packages/core/src/Models/ProductVariant.php (1)
getThumbnailImage(221-224)packages/core/src/Models/Product.php (1)
getThumbnailImage(216-219)
tests/core/Stubs/Models/Discountable.php (1)
packages/core/src/Models/Discountable.php (1)
Discountable(21-69)
packages/core/src/Observers/DiscountObserver.php (4)
packages/core/src/Models/DiscountCollection.php (1)
discount(34-37)packages/core/src/Models/Discountable.php (1)
discount(49-52)packages/core/src/Models/Contracts/Discount.php (1)
discountables(20-20)packages/core/src/Models/Discount.php (1)
discountables(95-98)
packages/core/src/Models/DiscountCollection.php (1)
packages/core/database/factories/DiscountableFactory.php (1)
DiscountableFactory(8-19)
packages/core/src/Models/Collection.php (5)
packages/core/src/Base/BaseModel.php (1)
BaseModel(8-25)packages/core/src/Base/HasThumbnailImage.php (1)
getThumbnailImage(10-10)packages/core/src/Models/ProductVariant.php (1)
getThumbnailImage(221-224)packages/core/src/Models/Product.php (1)
getThumbnailImage(216-219)packages/core/src/Base/Traits/HasMedia.php (1)
thumbnail(35-39)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductVariantLimitationRelationManager.php (4)
packages/core/src/Models/ProductVariant.php (1)
ProductVariant(54-225)packages/core/src/Base/Traits/HasModelExtending.php (1)
morphName(97-100)packages/core/src/Models/Contracts/Discountable.php (1)
discountable(19-19)packages/core/src/Models/Discountable.php (1)
discountable(57-60)
packages/core/src/Models/Product.php (5)
packages/core/src/Base/BaseModel.php (1)
BaseModel(8-25)packages/core/src/Base/HasThumbnailImage.php (1)
getThumbnailImage(10-10)packages/core/src/Models/Collection.php (1)
getThumbnailImage(153-156)packages/core/src/Models/ProductVariant.php (1)
getThumbnailImage(221-224)packages/core/src/Base/Traits/HasMedia.php (1)
thumbnail(35-39)
packages/core/src/Models/Contracts/Discountable.php (3)
packages/core/src/Models/Discountable.php (3)
Discountable(21-69)discount(49-52)discountable(57-60)tests/core/Stubs/Models/Discountable.php (1)
Discountable(5-5)packages/core/src/Models/DiscountCollection.php (1)
discount(34-37)
packages/core/database/factories/DiscountableFactory.php (4)
packages/core/src/Models/Discountable.php (1)
Discountable(21-69)tests/core/Stubs/Models/Discountable.php (1)
Discountable(5-5)packages/core/src/Models/ProductVariant.php (1)
ProductVariant(54-225)packages/core/src/Base/Traits/HasModelExtending.php (1)
morphName(97-100)
packages/core/database/migrations/2025_06_18_100000_rename_discount_purchasables_table.php (1)
packages/core/src/Base/Migration.php (1)
Migration(7-38)
packages/core/src/Managers/DiscountManager.php (5)
packages/core/src/Models/Product.php (1)
collections(129-135)packages/core/src/Models/Discount.php (1)
collections(125-133)packages/core/src/Models/Cart.php (1)
lines(210-213)packages/core/src/Models/CartLine.php (1)
purchasable(147-150)packages/core/src/Models/ProductVariant.php (1)
product(88-91)
packages/core/src/Models/Discountable.php (2)
packages/core/database/factories/DiscountableFactory.php (1)
DiscountableFactory(8-19)packages/core/src/Models/Contracts/Discountable.php (1)
discountable(19-19)
packages/core/src/Models/Contracts/Discount.php (1)
packages/core/src/Models/Discount.php (5)
discountables(95-98)discountableConditions(100-103)discountableExclusions(105-108)discountableLimitations(110-113)discountableRewards(115-118)
packages/admin/src/Support/Tables/Columns/ThumbnailImageColumn.php (4)
packages/core/src/Base/HasThumbnailImage.php (1)
getThumbnailImage(10-10)packages/core/src/Models/Collection.php (1)
getThumbnailImage(153-156)packages/core/src/Models/ProductVariant.php (1)
getThumbnailImage(221-224)packages/core/src/Models/Product.php (1)
getThumbnailImage(216-219)
tests/core/Unit/DiscountTypes/BuyXGetYTest.php (5)
packages/core/src/Models/Discount.php (6)
discountableLimitations(110-113)discountableRewards(115-118)discountableConditions(100-103)Discount(32-250)customerGroups(145-158)collections(125-133)packages/core/src/Models/CustomerGroup.php (1)
CustomerGroup(23-111)packages/core/src/Models/Product.php (1)
Product(43-220)packages/core/src/Models/ProductVariant.php (1)
ProductVariant(54-225)packages/core/src/DiscountTypes/BuyXGetY.php (1)
BuyXGetY(17-307)
🪛 markdownlint-cli2 (0.17.2)
docs/core/reference/discounts.md
92-92: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
93-93: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
94-94: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🪛 PHPMD (2.15.0)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php
64-64: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php
75-75: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
packages/admin/src/Support/Tables/Columns/ThumbnailImageColumn.php
11-11: Avoid unused parameters such as '$state'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (52)
packages/core/src/Base/HasThumbnailImage.php (1)
1-11: LGTM! Well-designed interface for thumbnail standardization.The interface provides a clean abstraction for models that need to expose thumbnail images. The method signature and documentation are clear and consistent with the implementations in
Product,ProductVariant, andCollectionmodels.tests/core/Stubs/Models/Discountable.php (1)
1-5: LGTM! Clean test stub implementation.The stub correctly extends the core
Discountablemodel and follows standard testing patterns. The minimal approach is appropriate for test scenarios.packages/core/src/Observers/DiscountObserver.php (1)
18-18: LGTM! Correctly updated relationship call.The change from
purchasables()todiscountables()aligns with the broader refactor and maintains referential integrity. TheDiscountmodel has a properdiscountables()relationship as confirmed in the relevant code snippets.packages/core/src/Models/DiscountCollection.php (2)
8-8: LGTM! Factory import updated correctly.The import change from
DiscountPurchasableFactorytoDiscountableFactoryaligns with the broader refactor from purchasables to discountables.
31-31: LGTM! Factory usage updated correctly.The factory method now returns
DiscountableFactory::new()which is consistent with the refactor and the factory's proper definition shown in the relevant code snippets.packages/core/src/Models/Product.php (1)
18-18: LGTM: Consistent HasThumbnailImage interface implementationThe implementation correctly follows the interface contract and matches the pattern used in other models (Collection, ProductVariant). The method properly handles the case where no thumbnail exists by returning an empty string.
Also applies to: 43-43, 216-219
packages/core/database/migrations/2024_03_15_100000_remap_polymorphic_relations.php (1)
29-29: LGTM: Model class name updated for polymorphic relation mappingThe change from
DiscountPurchasabletoDiscountableis consistent with the broader refactor and ensures polymorphic relations are properly remapped to the new model class.docs/core/upgrading.md (1)
21-28: Well-documented breaking change with clear upgrade guidanceThe documentation effectively explains the database schema change and provides clear guidance for users who were directly using the old
discount_purchasablestable orpurchasablesrelation.packages/core/src/Models/Collection.php (1)
13-13: LGTM: Consistent HasThumbnailImage interface implementationThe implementation mirrors the
Productmodel's approach and correctly implements the interface contract. The method properly handles cases where no thumbnail exists by returning an empty string.Also applies to: 37-37, 153-156
packages/core/src/Models/Contracts/Discountable.php (1)
9-9: LGTM: Interface rename aligns with broader refactorThe interface rename from
DiscountPurchasabletoDiscountableand the method rename frompurchasable()todiscountable()are consistent with the broader refactor. The method contract remains unchanged, ensuring proper polymorphic behavior.Also applies to: 17-19
packages/core/src/DiscountTypes/AmountOff.php (1)
173-179: LGTM! Consistent refactoring from purchasables to discountables.The property name changes from
purchasableLimitations/purchasableExclusionstodiscountableLimitations/discountableExclusionsand the corresponding property access updates frompurchasabletodiscountableare consistent with the broader refactoring objective. The logic remains intact while expanding the scope to support more entity types.packages/admin/resources/lang/en/discount.php (2)
251-255: LGTM! Language updates reflect the expanded scope.The terminology changes from "Product Rewards" to "Rewards" and "Add Product" to "Add Reward" appropriately reflect the expanded functionality where discounts can now apply to products, variants, and collections rather than just products.
286-290: LGTM! Improved terminology for conditions.The updates from "Product Conditions" to "Conditions" and "Add Product" to "Add Condition" make the UI language more accurate and general, reflecting that conditions can now include various entity types beyond just products.
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductVariantLimitationRelationManager.php (4)
18-18: LGTM! Updated relationship to use new discountables model.The change from
purchasablestodiscountablesrelationship aligns with the broader refactoring and supports the expanded entity types.
38-39: LGTM! Query constraints updated consistently.The updates from
whereDiscountableTypeandwhereHas('discountable')are consistent with the new relationship structure and maintain the same filtering logic.
43-43: LGTM! Form component updated to use discountable relationship.The MorphToSelect component now correctly targets the
discountablerelationship, maintaining the same functionality with the new model structure.
67-85: LGTM! Table columns updated to use discountable relationship.All table column references have been systematically updated from
purchasabletodiscountable, maintaining the same display logic while using the new relationship structure.packages/core/database/factories/DiscountableFactory.php (1)
8-19: LGTM! Well-structured factory for the new Discountable model.The factory correctly implements the new
Discountablemodel with appropriate defaults:
- Uses
discountable_idanddiscountable_typefollowing the new naming convention- Defaults to
ProductVariantas the discountable entity, which is appropriate for testing- Follows Laravel factory conventions
packages/core/src/Models/Discountable.php (5)
10-10: LGTM! Factory reference updated consistently.The factory reference has been correctly updated from
DiscountPurchasableFactorytoDiscountableFactoryto align with the new naming convention.
21-21: LGTM! Model renamed to reflect broader scope.The class name change from
DiscountPurchasabletoDiscountableappropriately reflects the expanded functionality where discounts can apply to various entity types beyond just purchasables.
33-34: LGTM! Fillable attributes updated for polymorphic relationship.The fillable attributes have been correctly updated from
purchasable_typeandpurchasable_idtodiscountable_typeanddiscountable_id, maintaining the polymorphic relationship structure with the new naming convention.
43-43: LGTM! Factory method updated consistently.The factory method now correctly returns the new
DiscountableFactoryinstance, maintaining consistency with the renamed factory class.
57-57: LGTM! Polymorphic relationship method renamed appropriately.The method name change from
purchasable()todiscountable()is consistent with the model refactoring and maintains the same polymorphic relationship functionality while using the new naming convention.packages/core/src/Managers/DiscountManager.php (1)
141-141: LGTM: Relationship name updated correctly.The change from
'purchasables'to'discountables'aligns with the broader refactor.packages/core/database/migrations/2025_06_18_100000_rename_discount_purchasables_table.php (1)
1-35: LGTM: Migration correctly renames table and columns.The migration properly:
- Renames the table from
discount_purchasablestodiscountables- Updates polymorphic column names from
purchasable_*todiscountable_*- Includes reversible
down()method- Extends the appropriate base migration class
tests/core/Unit/DiscountTypes/AmountOffTest.php (4)
324-329: LGTM: Test updated correctly for new relationship naming.The changes from
purchasableLimitations()todiscountableLimitations()and updating the array keys are consistent with the broader refactor.
421-426: LGTM: Test updated correctly for new relationship naming.The changes from
purchasableLimitations()todiscountableLimitations()and updating the array keys are consistent with the broader refactor.
518-523: LGTM: Test updated correctly for new relationship naming.The changes from
purchasableExclusions()todiscountableExclusions()and updating the array keys are consistent with the broader refactor.
615-620: LGTM: Test updated correctly for new relationship naming.The changes from
purchasableExclusions()todiscountableExclusions()and updating the array keys are consistent with the broader refactor.packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductLimitationRelationManager.php (4)
17-17: LGTM: Relationship property updated correctly.The change from
'purchasables'to'discountables'aligns with the broader refactor.
36-38: LGTM: Query filters updated correctly.The method calls have been updated from
purchasabletodiscountableterminology while maintaining the same filtering logic.
41-52: LGTM: Form component updated correctly.The
MorphToSelectcomponent has been updated to use'discountable'instead of'purchasable'while maintaining the same functionality.
61-73: LGTM: Table columns updated correctly.The table columns have been updated to reference
discountablerelationships instead ofpurchasablewhile maintaining the same display logic.packages/admin/src/Support/Tables/Columns/ThumbnailImageColumn.php (1)
1-26: LGTM: Well-designed column class for thumbnail image handling.The class provides a clean abstraction for resolving thumbnail URLs with:
- Optional custom resolution via closure
- Fallback to model's
getThumbnailImage()method- Fluent interface for configuration
The unused
$stateparameter is expected when overriding parent methods and is not an issue.packages/admin/src/helpers.php (3)
10-10: LGTM: Added necessary import for search column expression helper.The import supports the standardization of search column expression generation.
79-79: LGTM: Added explicit string type hints.The type hints improve function signature clarity and provide better IDE support and static analysis.
97-119: LGTM: Improved search expression generation with proper database context.The changes enhance search functionality by:
- Using the standardized
generate_search_column_expressionhelper- Passing database connection context for proper SQL generation
- Enforcing case-insensitive search behavior
These improvements ensure consistent search behavior across different database drivers.
packages/core/src/DiscountTypes/BuyXGetY.php (4)
12-15: LGTM: Added necessary imports for expanded discount support.The imports for
CollectionandProductVariantsupport the new functionality allowing discounts to be applied to collections and variants in addition to products.
61-83: LGTM: Expanded discount conditions to support products, variants, and collections.The refactored logic correctly handles three discountable types:
- Products: Matches against the cart line's product ID
- Product Variants: Matches against the cart line's purchasable (variant) ID
- Collections: Matches when the cart line's product belongs to the specified collection
The implementation maintains the original product logic while cleanly extending support for variants and collections.
111-125: LGTM: Consistent reward filtering logic with expanded type support.The reward filtering logic mirrors the condition logic, supporting the same three discountable types. The implementation correctly identifies eligible reward lines based on product, variant, or collection matching.
218-218: LGTM: Updated automatic rewards to use discountable terminology.The method call correctly uses the renamed
discountableRewardsrelationship while maintaining the same functionality.tests/core/Unit/DiscountTypes/BuyXGetYTest.php (5)
180-189: LGTM: Updated test to use discountable terminology.The test correctly uses the new
discountableLimitations()method anddiscountable_type/discountable_idarray keys, maintaining the same test logic while adopting the new terminology.
293-308: LGTM: Updated existing test with new terminology and assertions.The test correctly uses the new
discountableConditions()anddiscountableRewards()methods. The assertions verify both the expected cart total and the presence of free items, ensuring the discount logic works correctly.
310-404: LGTM: Added comprehensive test for variant-based discounts.This new test verifies that discounts can be applied when:
- The condition is based on a product
- The reward is based on a specific product variant
The test setup and assertions are comprehensive, ensuring the new variant-based discount functionality works correctly.
406-504: LGTM: Added comprehensive test for collection-based conditions.This new test verifies that discounts can be applied when:
- The condition is based on a collection
- Products in the cart belong to that collection
- The reward is based on a different product
The test includes proper collection-product relationship setup and comprehensive assertions.
506-600: LGTM: Added comprehensive test for variant-based conditions.This new test verifies that discounts can be applied when:
- The condition is based on a specific product variant
- The reward is based on a product
The test complements the other new tests by covering the variant-as-condition scenario.
packages/core/src/Models/Contracts/Discount.php (1)
18-40: LGTM: Updated interface to reflect discountable terminology.The interface correctly updates all method names and documentation from "purchasable" to "discountable" while maintaining the same method signatures and return types. This aligns with the implementation changes in the concrete
Discountmodel and supports the expanded functionality for collections and variants.packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php (2)
10-16: LGTM! Clean imports for extended functionality.The imports properly support the new discountable types (Collection, Product, ProductVariant) and the custom ThumbnailImageColumn.
47-48: Good implementation of polymorphic filtering.The query correctly filters for multiple discountable types and ensures the related model exists.
packages/core/src/Models/Discount.php (3)
95-118: Well-structured relationship refactoring.The renaming from
purchasable*todiscountable*relationships is consistent and properly maintains the type filtering logic. This change successfully broadens the scope beyond just purchasable items.
180-199: Excellent implementation of the collections scope.The new
scopeCollectionsmethod follows the established pattern and properly handles:
- Collection ID normalization
- Type filtering
- Polymorphic relationship queries with correct morph type checking
This enables filtering discounts by collections, completing the feature set alongside products and variants.
209-240: Consistent updates to existing scopes.The
scopeProductsandscopeProductVariantsmethods have been properly updated to use the newdiscountablesrelationship while maintaining their original filtering logic.
...in/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (1)
45-46: Method casing inconsistency remains unfixed.The method name
WhereInshould bewhereInfollowing Laravel's camelCase conventions.- ->WhereIn('discountable_type', [Product::morphName(), ProductVariant::morphName()]) + ->whereIn('discountable_type', [Product::morphName(), ProductVariant::morphName()])
🧹 Nitpick comments (1)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (1)
64-64: Remove unused parameter in closure.The
$componentparameter is not used in the closure and should be removed.- ->getSearchResultsUsing(static function (Forms\Components\Select $component, string $search): array { + ->getSearchResultsUsing(static function (string $search): array {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/core/reference/discounts.md(2 hunks)packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/core/reference/discounts.md
🧰 Additional context used
🪛 PHPMD (2.15.0)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php
64-64: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: shipping - PHP 8.4 - L12.* ↑ E
- GitHub Check: admin - PHP 8.4 - L12.* ↑
- GitHub Check: search - PHP 8.4 - L12.* ↑ E
- GitHub Check: shipping - PHP 8.4 - L11.* ↑ E
- GitHub Check: core - PHP 8.4 - L12.* ↑ E
- GitHub Check: search - PHP 8.4 - L11.* ↑
- GitHub Check: stripe - PHP 8.4 - L11.* ↑ E
- GitHub Check: admin - PHP 8.4 - L11.* ↑ E
- GitHub Check: core - PHP 8.4 - L11.* ↑ E
- GitHub Check: search - PHP 8.3 - L12.* ↑
- GitHub Check: core - PHP 8.3 - L12.* ↑
- GitHub Check: core - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L11.* ↑ E
- GitHub Check: admin - PHP 8.3 - L11.* ↑ E
- GitHub Check: admin - PHP 8.3 - L11.* ↑
- GitHub Check: core - PHP 8.3 - L11.* ↑
- GitHub Check: PHP 8.3 - L^12.0 ↑
🔇 Additional comments (6)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (6)
10-14: LGTM: Imports updated correctly for new functionality.The addition of
ThumbnailImageColumnandProductVariantimports aligns with the expanded discount functionality.
20-20: LGTM: Relationship name updated consistently.The change from
'purchasables'to'discountables'is consistent with the broader refactoring initiative.
50-70: LGTM: MorphToSelect configuration supports both Product and ProductVariant types.The form configuration correctly handles both entity types with appropriate search functionality.
79-81: ThumbnailImageColumn implementation looks correct.The implementation properly resolves thumbnail URLs from the polymorphic discountable relationship. This addresses the image handling challenges discussed in previous reviews.
83-89: Conditional display logic correctly handles both Product and ProductVariant types.The implementation properly differentiates between ProductVariant (showing SKU) and Product (showing name) types.
91-97: New discountable type column provides valuable context.The addition of the type column helps users understand what type of entity each reward represents, enhancing the admin interface usability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (1)
80-82: Verify the ThumbnailImageColumn implementation addresses N+1 concerns.The implementation uses a closure to resolve thumbnail URLs from the discountable relation. Based on past discussions, this approach may still cause N+1 queries when dealing with media relations across different models.
Let me verify the ThumbnailImageColumn implementation to ensure it properly handles the media loading:
#!/bin/bash # Description: Check the ThumbnailImageColumn implementation to understand how it handles media loading # Search for the ThumbnailImageColumn class definition ast-grep --pattern 'class ThumbnailImageColumn $$$' # Check if there are any eager loading optimizations rg -A 10 -B 5 "getThumbnailImage|resolveThumbnailUrlUsing" packages/admin/src/Support/Tables/Columns/
🧹 Nitpick comments (1)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (1)
50-70: Remove unused parameter in search callback.The
$componentparameter is not used in the search callback function on line 64. Consider removing it to clean up the code.- ->getSearchResultsUsing(static function (Forms\Components\Select $component, string $search): array { + ->getSearchResultsUsing(static function (string $search): array {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php(4 hunks)packages/core/src/Models/ProductVariant.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Models/ProductVariant.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php
64-64: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: search - PHP 8.4 - L12.* ↑
- GitHub Check: search - PHP 8.4 - L12.* ↑ E
- GitHub Check: core - PHP 8.4 - L12.* ↑ E
- GitHub Check: shipping - PHP 8.4 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L11.* ↑
- GitHub Check: search - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L12.* ↑ E
- GitHub Check: admin - PHP 8.3 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L11.* ↑ E
- GitHub Check: shipping - PHP 8.3 - L12.* ↑
- GitHub Check: search - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L11.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L11.* ↑ E
- GitHub Check: admin - PHP 8.3 - L11.* ↑
- GitHub Check: core - PHP 8.3 - L11.* ↑
- GitHub Check: PHP 8.3 - L^11.0 ↑
- GitHub Check: PHP 8.4 - L^11.0 ↑
- GitHub Check: PHP 8.4 - L^12.0 ↑
🔇 Additional comments (5)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (5)
10-14: LGTM: Import statements correctly updated for new functionality.The new imports for
ThumbnailImageColumn,ProductVariantContract, andProductVariantare necessary for the enhanced functionality to support variant-level discounts.
20-20: LGTM: Relationship name updated to reflect new terminology.The change from
'purchasables'to'discountables'aligns with the broader refactoring effort and maintains consistency across the codebase.
45-46: LGTM: Query filtering updated correctly.The query modifications correctly use the new
discountable_typeanddiscountablerelationship names, maintaining the same filtering logic with updated terminology.
84-90: LGTM: Text column logic properly handles both Product and ProductVariant.The conditional formatting correctly displays either the product name for products or "product name - SKU" for product variants, providing clear identification for each discountable type.
92-98: LGTM: New type column provides helpful context.The addition of a discountable type column with proper formatting enhances the user interface by clearly showing whether each reward is a product or variant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php (1)
91-109: Consider refactoring shared column logic.The table column implementations (ThumbnailImageColumn, name formatting, type display) are identical between ProductConditionRelationManager and ProductRewardRelationManager. This creates maintenance overhead and potential inconsistencies.
Consider extracting the common column logic into a shared trait or base class method to reduce code duplication.
Example approach:
// In BaseRelationManager or a new trait protected function getDiscountableColumns(): array { return [ ThumbnailImageColumn::make('discountable_id') ->resolveThumbnailUrlUsing(fn (?Model $record) => $record?->discountable?->getThumbnailImage()) ->label(''), Tables\Columns\TextColumn::make('discountable.id') ->label(__('lunarpanel::discount.relationmanagers.conditions.table.name.label')) ->formatStateUsing( fn (Model $record) => $record->discountable instanceof ProductVariantContract ? $record->discountable->product->attr('name').' - '.$record->discountable->sku : $record->discountable->attr('name') ), Tables\Columns\TextColumn::make('discountable_type') ->label(__('lunarpanel::discount.relationmanagers.conditions.table.type.label')) ->formatStateUsing( fn (Model $record) => str($record->discountable->morphName())->replace('_', ' ')->title(), ), ]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php(4 hunks)packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php(4 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php
75-75: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php
64-64: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: shipping - PHP 8.4 - L11.* ↑
- GitHub Check: shipping - PHP 8.4 - L11.* ↑ E
- GitHub Check: search - PHP 8.4 - L11.* ↑ E
- GitHub Check: admin - PHP 8.4 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L12.* ↑
- GitHub Check: shipping - PHP 8.3 - L12.* ↑
- GitHub Check: stripe - PHP 8.3 - L12.* ↑
- GitHub Check: shipping - PHP 8.3 - L12.* ↑ E
- GitHub Check: stripe - PHP 8.3 - L11.* ↑ E
- GitHub Check: admin - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L11.* ↑
- GitHub Check: admin - PHP 8.3 - L11.* ↑
- GitHub Check: PHP 8.3 - L^11.0 ↑
- GitHub Check: PHP 8.3 - L^12.0 ↑
🔇 Additional comments (9)
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductRewardRelationManager.php (6)
20-20: LGTM: Relationship renamed consistently.The relationship name change from 'purchasables' to 'discountables' aligns with the broader refactoring effort described in the PR objectives.
45-46: LGTM: Query logic updated correctly.The query modifications properly:
- Use correct camelCase method naming (
whereIninstead of the previousWhereIn)- Update column references from
purchasable_typetodiscountable_type- Update relationship checks from
purchasabletodiscountable- Include both Product and ProductVariant types for reward selection
50-70: LGTM: MorphToSelect implementation supports multiple entity types.The form field correctly implements support for both Product and ProductVariant types with appropriate search logic. The ProductVariant search includes both name and SKU matching, and the display format clearly distinguishes variants with "Product Name - SKU" format.
80-82: LGTM: ThumbnailImageColumn implementation follows established pattern.The image column correctly uses the new
ThumbnailImageColumnclass with a closure to resolve thumbnail URLs from the discountable relationship. This addresses the previous discussions about image handling in the admin panel.
84-90: LGTM: Text column handles multiple entity types appropriately.The column correctly formats display names based on the discountable type:
- ProductVariant: Shows "Product Name - SKU"
- Other types: Shows the entity name
This provides clear identification for users.
92-98: LGTM: Type column provides useful context.The discountable type column helps users understand what type of entity they're working with, using a clean formatted display.
packages/admin/src/Filament/Resources/DiscountResource/RelationManagers/ProductConditionRelationManager.php (3)
47-48: LGTM: Query includes all supported condition types.The query correctly includes Collection, Product, and ProductVariant types for discount conditions, expanding the functionality as described in the PR objectives.
55-62: LGTM: Collection support implemented consistently.The Collection type implementation follows the same pattern as Product and ProductVariant types with appropriate search functionality using the collection name attribute.
73-81: LGTM: ProductVariant support matches reward manager implementation.The ProductVariant implementation is consistent with the ProductRewardRelationManager, ensuring a uniform user experience across condition and reward management.
5 participants
This PR extends the BuyXGetY discount conditions to support
CollectionorVariantas a condition, andVariantas a reward:This allows you to choose collections or variants that the discount applies to, not just products, and rewards can be scoped to specific variants, not just products.
As its no longer 'Product Conditions' and
Product Rewards, I've also changed lang strings for some elements on the page to make things match the new approach.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores