Create and sync prices for currencies#2219
Conversation
…ub.com:lunarphp/lunar into lun-297-create-prices-for-currency-on-creation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@alecritson could you still manually edit the other currency prices individually if you wanted to, when set to automatic? |
There is nothing stopping you from editing prices but they would be overwritten if you was to then adjust the price in the default currency. |
There was a problem hiding this comment.
@alecritson I enabled sync, but when editing prices on the variant edit page, e.g. /lunar/product-variants/1/pricing it doesn't update the other prices.
When editing a price from the variants page, e.g. /lunar/products/1/variants it works and keeps the price sync'd.
When trying to add a Customer Group Price or Price Break, I get Attempt to read property "exchange_rate" on null error.
|
@glennjacobs Made some tweaks to hopefully solve the problem. |
|
Warning Rate limit exceeded@glennjacobs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a boolean sync_prices attribute and admin UI toggle for currencies; creates actions and queued jobs to generate and synchronize converted prices; registers a Price observer and updates Currency observer to dispatch jobs and clean up prices; updates product pricing handling, Currency casts, translations, and a migration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (3)
packages/core/src/Observers/PriceObserver.php (1)
10-22: Consider optimizing currency relationship loading to avoid N+1 queries.The observer logic correctly dispatches sync jobs only for default currency prices. However, accessing
$price->currency->defaultmay cause N+1 queries if the currency relationship isn't loaded.Consider using eager loading or checking if the relationship is already loaded before accessing it.
public function created(Price $price): void { - if ($price->currency->default) { + if ($price->currency?->default) { SyncPriceCurrencies::dispatch($price); } } public function updated(Price $price): void { - if ($price->currency->default) { + if ($price->currency?->default) { SyncPriceCurrencies::dispatch($price); } }Alternatively, you could load the relationship if not already loaded:
public function created(Price $price): void { + if (!$price->relationLoaded('currency')) { + $price->load('currency'); + } if ($price->currency->default) { SyncPriceCurrencies::dispatch($price); } }packages/core/src/Observers/CurrencyObserver.php (1)
42-50: Fix docblock comment and approve the cleanup logic.The cleanup logic in the
deletingmethod is correct and follows the same pattern as other observers (e.g.,CollectionObserver). However, the docblock comment incorrectly says "deleted" instead of "deleting"./** - * Handle the Currency "deleted" event. + * Handle the Currency "deleting" event. * * @return void */ public function deleting(CurrencyContract $currency)packages/admin/src/Filament/Resources/CurrencyResource.php (1)
114-120: Consider implications of default true value.The
sync_pricestoggle defaults totrue, which means new currencies will automatically sync prices. This could lead to unexpected behavior if users aren't aware of this setting.Consider adding a confirmation or making the default
falseto be more explicit:- ->default(true); + ->default(false);Or add additional helper text to clarify the implications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/admin/resources/lang/en/currency.php(1 hunks)packages/admin/src/Filament/Resources/CurrencyResource.php(4 hunks)packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php(3 hunks)packages/core/database/migrations/2025_06_18_100000_add_sync_prices_to_currencies_table.php(1 hunks)packages/core/src/Actions/Currencies/CreateCurrencyPrices.php(1 hunks)packages/core/src/Jobs/Currencies/CreateCurrencyPrices.php(1 hunks)packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php(1 hunks)packages/core/src/LunarServiceProvider.php(3 hunks)packages/core/src/Observers/CurrencyObserver.php(3 hunks)packages/core/src/Observers/PriceObserver.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php (4)
packages/admin/src/helpers.php (1)
price(13-16)packages/core/src/Models/Price.php (2)
Price(26-139)currency(63-66)packages/core/src/Base/Traits/HasPrices.php (2)
basePrices(26-29)prices(15-21)packages/core/src/Managers/PricingManager.php (1)
currency(98-103)
packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php (4)
packages/core/src/Models/Currency.php (1)
Currency(25-75)packages/core/src/Jobs/Currencies/CreateCurrencyPrices.php (2)
__construct(21-21)handle(28-47)packages/admin/src/helpers.php (1)
price(13-16)packages/core/src/Actions/Currencies/CreateCurrencyPrices.php (1)
handle(10-39)
packages/core/src/Actions/Currencies/CreateCurrencyPrices.php (4)
packages/core/src/Models/Currency.php (1)
Currency(25-75)packages/core/src/Jobs/Currencies/CreateCurrencyPrices.php (2)
CreateCurrencyPrices(12-48)handle(28-47)packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php (1)
handle(29-60)packages/core/src/Base/Traits/HasPrices.php (1)
basePrices(26-29)
packages/core/src/Jobs/Currencies/CreateCurrencyPrices.php (2)
packages/core/src/Models/Currency.php (1)
Currency(25-75)packages/core/src/Actions/Currencies/CreateCurrencyPrices.php (2)
CreateCurrencyPrices(8-40)handle(10-39)
packages/core/src/Observers/CurrencyObserver.php (4)
packages/core/src/Jobs/Currencies/CreateCurrencyPrices.php (1)
CreateCurrencyPrices(12-48)packages/core/src/Actions/Currencies/CreateCurrencyPrices.php (1)
CreateCurrencyPrices(8-40)packages/core/src/Observers/CollectionObserver.php (1)
deleting(26-34)packages/core/src/Models/Currency.php (1)
prices(56-59)
⏰ 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: stripe - PHP 8.4 - L12.* ↑
- GitHub Check: search - PHP 8.4 - L12.* ↑
- GitHub Check: shipping - PHP 8.4 - L12.* ↑
- GitHub Check: admin - PHP 8.4 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L12.* ↑
- GitHub Check: admin - PHP 8.4 - L11.* ↑ E
- GitHub Check: admin - PHP 8.4 - L12.* ↑ E
- GitHub Check: admin - PHP 8.4 - L11.* ↑
- GitHub Check: stripe - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L12.* ↑ E
- GitHub Check: core - PHP 8.4 - L11.* ↑ E
- GitHub Check: search - PHP 8.3 - L12.* ↑
- GitHub Check: core - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑
- GitHub Check: admin - 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: fix-code-style
🔇 Additional comments (18)
packages/admin/resources/lang/en/currency.php (1)
49-52: LGTM! Clear and informative translations.The translation entries follow the established pattern and provide clear, helpful text for users to understand the sync_prices functionality.
packages/core/database/migrations/2025_06_18_100000_add_sync_prices_to_currencies_table.php (1)
9-21: LGTM! Well-structured migration.The migration correctly adds the
sync_pricesboolean column with appropriate default value and positioning. The rollback method is properly implemented.packages/core/src/LunarServiceProvider.php (3)
73-73: LGTM! Proper model import.The Price model import is correctly added following the alphabetical order of existing imports.
92-92: LGTM! Proper observer import.The PriceObserver import is correctly added following the alphabetical order of existing observer imports.
318-318: LGTM! Consistent observer registration.The PriceObserver registration follows the established pattern and is properly positioned alphabetically among other observer registrations.
packages/core/src/Observers/CurrencyObserver.php (2)
5-5: LGTM! Proper job import.The CreateCurrencyPrices job import is correctly added.
19-19: LGTM! Automatic price creation for new currencies.Dispatching the CreateCurrencyPrices job when a new currency is created ensures prices are automatically generated based on the default currency and exchange rate.
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php (4)
66-66: Good optimization to prevent unnecessary updates.The condition to only update prices when
value != original_valueis a solid optimization that prevents redundant database operations during price synchronization.
180-180: Consistent tracking of original values.The addition of
original_valuefield properly tracks the original price value for comparison during updates, supporting the optimization introduced in the update logic.
198-202: Correct calculation of original_value for missing currencies.The logic properly calculates the
original_valuefor missing currencies using the exchange rate, ensuring consistency with the existing price calculation logic.
73-73: No impact from variant parameter removal ingetBasePrices()Confirmed that the
getBasePrices()method in
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php(line 168) is defined without any parameters, and every invocation across the codebase (e.g., in the Filament pages and within the concern) calls it with empty parentheses. No call sites pass a variant argument, so removing that parameter is safe.packages/admin/src/Filament/Resources/CurrencyResource.php (2)
45-52: Well-structured form method addition.The
getDefaultFormmethod follows the established pattern and properly integrates the main form components within a section structure.
63-63: Proper integration of sync prices component.The addition of
getSyncPricesFormComponent()to the main form components array maintains consistency with the existing form structure.packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php (2)
36-42: Comprehensive price counterpart query.The query correctly matches all relevant price attributes (priceable_id, priceable_type, currency_id, min_quantity, customer_group_id) to find the corresponding price in the target currency.
49-49: Price calculation is consistent across SyncPriceCurrencies and CreateCurrencyPricesBoth implementations multiply the base price by the currency’s exchange rate:
- packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php
•$this->price->price->value * $currency->exchange_rate- packages/core/src/Actions/Currencies/CreateCurrencyPrices.php
•DB::raw('price * '.$incomingCurrency->exchange_rate.' as price')No further changes required.
packages/core/src/Jobs/Currencies/CreateCurrencyPrices.php (1)
35-40: Good approach to handle default currency changes.The logic to detect when a currency becomes default and find the previous default currency within a 15-second window is a reasonable approach to handle currency transitions.
packages/core/src/Actions/Currencies/CreateCurrencyPrices.php (2)
28-38: Efficient bulk operation implementation.The use of
insertUsing()with a query builder is an efficient approach for bulk price duplication, avoiding the overhead of individual model creation.
22-22: No SQL injection risk for currency ID
TheincomingCurrency->idoriginates from an Eloquent model’s integer primary key, so it cannot contain malicious SQL. Embedding it in aDB::raw()string literal (wrapped in quotes) prevents breaking out of the expression.Since the ID is guaranteed to be an integer from the database and isn’t user-supplied text, no additional escaping or parameter binding is required here.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…s://github.com/lunarphp/lunar into lun-297-create-prices-for-currency-on-creation
…s://github.com/lunarphp/lunar into lun-297-create-prices-for-currency-on-creation
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php (3)
82-83: Remove stray debug comment.Leftover debug can confuse future readers.
- // dd($this->basePrices);
143-146: Translation key for compare price tooltip.You're using the
...form.price.sync_pricekey for the compare_price tooltip as well. If a distinct message is intended for compare price, add and use a dedicated key; otherwise, consider reusing a neutral key name.Example:
- return __('lunarpanel::relationmanagers.pricing.form.basePrices.form.price.sync_price'); + return __('lunarpanel::relationmanagers.pricing.form.basePrices.form.compare_price.sync_price');Please confirm which translation keys exist in packages/admin/resources/lang/en/currency.php to avoid missing-key issues.
222-236: Also set original_compare_price for computed currencies.For currencies without existing prices, you set original_value but not original_compare_price. Adding it keeps the state symmetric and future-proofs change detection if logic ever extends to handle create/update in one pass.
$prices->put($currency->code, [ 'id' => null, 'original_value' => $value, 'value' => $value, + 'original_compare_price' => round( + ($defaultCurrencyPrice['compare_price'] ?? 0) * $currency->exchange_rate, + $currency->decimal_places + ), 'compare_price' => round( ($defaultCurrencyPrice['compare_price'] ?? 0) * $currency->exchange_rate, $currency->decimal_places ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php(6 hunks)packages/core/src/Actions/Currencies/CreateCurrencyPrices.php(1 hunks)packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php(1 hunks)packages/core/src/Models/Currency.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/Actions/Currencies/CreateCurrencyPrices.php
- packages/core/src/Models/Currency.php
- packages/core/src/Jobs/Currencies/SyncPriceCurrencies.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php (6)
packages/admin/src/helpers.php (1)
price(14-17)packages/core/src/Models/Currency.php (2)
prices(67-70)Currency(26-86)packages/core/src/Base/Traits/HasPrices.php (2)
prices(15-21)basePrices(26-29)packages/core/src/Models/Price.php (2)
Price(26-139)currency(63-66)packages/admin/src/Filament/Resources/ProductResource/Pages/ManageProductPricing.php (1)
getOwnerRecord(36-39)packages/admin/src/Filament/Resources/ProductVariantResource/Pages/ManageVariantPricing.php (1)
getOwnerRecord(21-24)
🪛 PHPMD (2.15.0)
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php
143-143: Avoid unused parameters such as '$component'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 GitHub Actions: Tests
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php
[error] 105-105: Undefined array key 'sync_prices' (View: /home/runner/work/lunar/lunar/vendor/filament/forms/resources/views/components/field-wrapper/index.blade.php)
🔇 Additional comments (6)
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php (6)
59-59: Rounding fix for compare_price looks good.Casting to int with a rounded float aligns compare_price storage with price behavior and avoids off-by-one in minor units.
Also applies to: 69-69
73-74: Refreshing basePrices post-update is appropriate.Recomputing base prices after mutations keeps the UI state consistent, especially with computed/exchange-rate values.
121-123: Confirm UX: disabling edit when sync is enabled.Inputs are disabled when
sync_pricesis true, which blocks manual edits. In the PR discussion, it was mentioned manual edits should be possible but overwritten on default-currency changes. If that's still the desired behavior, consider allowing edits and showing a prominent “Will be overwritten when default currency price changes” hint instead of disabling.Do you want to keep inputs disabled for synced currencies, or allow edits with a warning?
Also applies to: 154-156
185-189: Currency ordering improvement looks good.Sorting by default desc and then by name ensures determinism and keeps the default currency prominent.
192-198: Eager-loading and sorting base prices by default currency is sound.This reduces N+1 queries and makes downstream logic simpler.
199-216: Including original_ values and sync_prices in state is a good addition.*This enables change detection and per-currency UI behavior without extra queries.
packages/admin/src/Support/Concerns/Products/ManagesProductPricing.php
Outdated
Show resolved
Hide resolved
…cing.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…oductPricing.php" This reverts commit 133b341.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/ManageProductPricingTest.php (1)
78-114: Add targeted tests for cross-currency sync behaviorTo cover the core of this PR, consider adding tests (in this or a dedicated suite) for:
- Creating a non-default currency with sync_prices = true and verifying the initial product prices are generated based on the exchange rate.
- Updating a default currency price and asserting corresponding prices in sync-enabled currencies are updated.
- Verifying manual edits in a sync-enabled currency are overwritten after the default price changes, and that disabling sync prevents overwrite.
- Ensuring the default currency does not expose or honor the sync toggle.
I can draft these tests, including queue fakes and assertions for job dispatch or outcomes, if helpful.
🧹 Nitpick comments (3)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/ManageProductPricingTest.php (3)
95-98: Fake the queue to avoid sync job side effects in this unit testSaving prices may now dispatch synchronization jobs via observers. To keep this test focused and deterministic, fake the queue before interacting with the Livewire component.
$this->asStaff(admin: true); + // Prevent queued sync jobs (currency price creation/sync) from affecting this test. + \Illuminate\Support\Facades\Queue::fake(); + \Livewire\Livewire::test(\Lunar\Admin\Filament\Resources\ProductResource\Pages\ManageProductPricing::class, [
99-107: New sync_prices payload key: ensure it’s ignored for Price persistence and only used for currency-level behaviorIncluding 'sync_prices' in basePrices makes sense with the new UI/state, but:
- Confirm it’s not blindly mass-assigned to the Price model (it shouldn’t be a Price attribute).
- For default currency, the toggle is hidden/disabled in the UI; consider omitting it from the payload to mirror actual flows, or explicitly document that passing it is harmless and ignored.
If not already done in ManagesProductPricing, ensure you filter payloads to only the fields needed for Price creation/update (e.g., id, currency_id, value, factor) and ignore UI-only fields like label and sync_prices.
109-111: Tighten the DB assertion to include currency_idAdd currency_id to make the assertion more specific and robust against unrelated prices in the table.
- \Pest\Laravel\assertDatabaseHas((new \Lunar\Models\Price)->getTable(), [ - 'price' => '232', - ]); + \Pest\Laravel\assertDatabaseHas((new \Lunar\Models\Price)->getTable(), [ + 'currency_id' => \Lunar\Models\Currency::getDefault()->id, + 'price' => '232', + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/ManageProductPricingTest.php(1 hunks)
⏰ 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: admin - PHP 8.4 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L12.* ↑ E
- GitHub Check: admin - PHP 8.4 - L12.* ↑ E
- GitHub Check: core - PHP 8.4 - L12.* ↑
- GitHub Check: shipping - PHP 8.4 - L11.* ↑
- GitHub Check: stripe - PHP 8.4 - L11.* ↑ E
- GitHub Check: admin - PHP 8.4 - L11.* ↑
- GitHub Check: shipping - PHP 8.4 - L11.* ↑ E
- GitHub Check: admin - PHP 8.4 - L11.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L12.* ↑
- GitHub Check: core - PHP 8.4 - L11.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑ E
- GitHub Check: admin - 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: admin - PHP 8.3 - L11.* ↑ E
- GitHub Check: fix-code-style
3 participants
Currently when a currency is created, no prices are created and then if the base price changes, other currency prices aren't kept up to date based on the exchange rate.
This PR looks to solve these issues:
sync_pricesboolean to currencies to determine whether prices in that currency should be updated when the default currency pricing is updated.Summary by CodeRabbit
New Features
Bug Fixes
Chores