Ensure automatic discounts can be fulfilled - #2509
Conversation
…2631) A merchant picks a specific variant as a Buy X Get Y reward — which the admin lets them do — and ticks "automatically add rewards". Every cart that qualifies then dies with a 500. ### What happens now - `Call to a member function first() on null` in `BuyXGetY::processAutomaticRewards()`, thrown during cart calculation. - It takes down anything that calculates the cart, so the shopper cannot view their basket, not just complete checkout. - Product rewards and collection rewards both work. Only the variant reward, combined with the automatic-add option, fails. ### What should happen - A variant reward is added to the cart in the same way a product reward is. ### Why #2214 made variants selectable as rewards, and #2461 added a branch for collections. The remaining `else` still assumes the reward is a `Product` and reaches for its variants: ```php } else { $purchasable = $selectedRewardItem->variants->first(); } ``` A `ProductVariant` has no `variants` relation, so the property read returns `null` and the `->first()` on it is fatal. The null guard immediately below never runs, because the error is thrown before it is reached. ### The fix Add a branch for a reward that is already purchasable, and use it directly. `ProductVariant` implements `Lunar\Base\Purchasable`, and `Product` does not, so the check separates the two cases without naming concrete models — the same approach the existing collection branch takes. Nothing else in the method changes: the existing product and collection paths are untouched, and the reward still flows through the normal cart-line pipeline afterwards. Worth flagging: #2509 is currently rewriting this method to skip rewards that cannot be fulfilled. The two changes do not overlap in intent, but whichever lands second will want a rebase. ### Tests `tests/core/Unit/DiscountTypes/BuyXGetYTest.php`: - **can add an eligible variant reward when not in cart** — the existing `can add eligible products when not in cart` scenario with the reward registered against the variant instead of the product. Fails on `1.x` with `Call to a member function first() on null` at `BuyXGetY.php:255`, and asserts the same £12.00 total and single free item afterwards. Co-authored-by: Glenn Jacobs <glenn@neondigital.co.uk>
…2632) A reward line added automatically by a Buy X Get Y discount is written to `lunar_cart_lines` with a different `purchasable_type` than every other line holding the same product. ### What happens now - The reward line stores `Lunar\Models\ProductVariant`, while a line the shopper added stores `product_variant`. - The same purchasable therefore appears under two different types in one cart. - Any query that filters cart lines by morph type — reporting, an `whereHasMorph`, a custom pipeline stage — silently skips these lines. - It survives into `lunar_order_lines`, so the inconsistency is persisted against the placed order too. ### What should happen - A reward line stores the same `purchasable_type` as any other line for that purchasable. ### Why Lunar registers a morph map at boot (`ModelManifest::morphMap()`, keyed by the snake-cased class basename), so `ProductVariant::getMorphClass()` returns `product_variant`. Every cart line in the codebase is created with `getMorphClass()`, except this one: ```php $rewardLine = $cart->lines()->make([ 'purchasable_type' => get_class($purchasable), ``` `get_class()` bypasses the map and writes the concrete class name. ### The fix Use `getMorphClass()`, matching how cart lines are created everywhere else. It also means a store that has extended `ProductVariant` gets the mapped alias rather than its own class name written into the table. Deliberately out of scope: with `reward_qty` greater than one this method also creates one cart line per reward unit rather than a single line, because its "is it already in cart?" check reads an in-memory `$cart->lines` the loop never appends to. That is a real but separate defect, and fixing it here would make this diff much harder to review. Also note #2509 is currently rewriting this method for stock availability — no overlap in intent, but a rebase for whichever lands second. ### Tests `tests/core/Unit/DiscountTypes/BuyXGetYTest.php`: - **can store an automatically added reward line using the morph map** — an automatic reward on a qualifying cart, asserting the persisted `purchasable_type`. Fails on `1.x` with: ``` -'product_variant' +'Lunar\Models\ProductVariant' ``` Co-authored-by: Glenn Jacobs <glenn@neondigital.co.uk> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
glennjacobs
left a comment
glennjacobs
left a comment
There was a problem hiding this comment.
Thanks Ryan — the fix is the right idea in the right place: automatically-added reward lines bypass the CartLineStock validator, so checking fulfillability at selection time is exactly where this belongs, and the four test scenarios are thorough.
A few things need addressing before this can land, mostly stemming from the branch pre-dating your own #2624/#2631/#2632 merges (as those PRs predicted, a rebase is needed — GitHub already reports conflicts, so nothing can merge silently):
1. The new filter fatals on variant-typed rewards
Both the fulfillability filter and the selection assume a reward discountable is a Product or Collection ($rewardItem->variants->first()), but a ProductVariant reward is legitimate — #2631 just fixed this exact crash in the old code. ProductVariant has no variants relation, so null->first() throws, and the variant-reward test now on 1.x would fail. The rebase alone won't cover it: the new filter needs its own instanceof Purchasable branch mirroring the one #2631 added to the selection.
2. Collection hydration cost inside Cart::calculate()
The filter loads products()->with('variants')->get() — the entire collection — per collection reward, and the selection loop re-hydrates the whole collection once per remaining reward unit (previously a single inRandomOrder()->first() query). This runs on essentially every cart mutation in a storefront, so for large collections it's O(collection × reward_qty) per recalculation. Could the fulfillable check be pushed into the query (or at least memoised across the filter/selection steps and loop iterations)?
3. ->or isn't a Pest API
expect($cart->freeItems)->toBeNull()->or->toBeEmpty() doesn't do what it reads as: or resolves to a higher-order expectation on a (nonexistent) or property, so the toBeEmpty() trivially passes against null — and a failed toBeNull() throws before it anyway. The tests pass today because freeItems genuinely is null, but the fallback is illusory — best to assert toBeNull() alone.
Smaller notes
- The qty-1 guard checks unchanging in-memory stock per iteration, so
reward_qty3 against a stock-1 variant still allocates 3 units. Still a clear improvement over no check — flagging in case it's cheap to track cumulative allocation while you're in here, especially if #2639 (which touches the same loop) lands first. - The two back-to-back guards with identical outcomes (
! $purchasable/! canBeFulfilledAtQuantity) can collapse into one.
Given #2639 also reworks this loop, it may be easiest to land that first and rebase this on top. Happy to re-review quickly once it's updated.
|
Would probably have preferred this to merge before the others as it would have been easier to merge these changes into the others :) I dont mind refactoring now, but could do with guidance from you on merge order before I spend the time on it. If you're planning on merging 2639 first then I'd need to wait. |
|
#2639 has been merged now. |
b605b14 to
3421c7c
Compare
…kups The fulfillability filter assumed a reward discountable was always a Product or Collection, so a ProductVariant reward crashed the same way lunarphp#2631 fixed for the reward-selection code. Mirror that instanceof Purchasable branch here. Also hydrate each collection reward's products once per apply() call instead of once in the filter and again per remaining reward unit in the selection loop, and collapse the two identical purchasable/ fulfillability guards into one.
3421c7c to
6fc10eb
Compare
|
Thanks, I've updated this now and applied the changes you suggested. It still checks for quantity 1, but if you want that I can change it. |
|
Thanks Ryan, that covers it — I checked the amends over:
One thing left, which is the cumulative-allocation note from last time — now that #2639 has landed it shows up more sharply. A variant with Not a regression — before this PR the reward was added regardless of stock — but it's the same bug class the PR is about, and the loop already tracks what it has added, so it's a couple of lines: $allocated = $addedRewardLines[$rewardKey]->quantity ?? 0;
if (! $purchasable || ! $purchasable->canBeFulfilledAtQuantity($allocated + 1)) {( Happy to merge once that's in — everything else here is good to go. |
canBeFulfilledAtQuantity(1) was checked on every pass through the loop, so a reward_qty of 3 against a stock-1 variant still allocated all 3 units onto the one line lunarphp#2639 now collapses them into. Check against what this run has already put on that line instead.
|
No problem, easily added as you say. Test coverage there now too. |
2 participants
the processAutomaticDiscounts method in BuyXGetY doesnt current consider whether the discount item can be fulfilled, so errors can be thrown in user land when trying to add an item to cart that has no available stock.
this PR updates the logic to ensure we only choose items that can be fulfilled.