Updated Price Model TaxIncl methods & Cart Item TaxIncl properties to actually provide Tax inclusive values.#2411
Updated Price Model TaxIncl methods & Cart Item TaxIncl properties to actually provide Tax inclusive values.#2411MacTavish-69 wants to merge 6 commits intolunarphp:1.xfrom
Conversation
taxIncl methods internally work on default tax class basis. The moment you've more than 1 tax class, it stops working. Moreover, taxation works on cart when address is set. However, it's a standard use case to display Tax inclusive prices to user when they're from a tax eligible jurisdiction. This use case is not achievable with current way of Lunar. Hence, we need to have tax information on the cart which can take care of it. For one time situations where you need tax Incl price for a given zone, you can pass the zone as parameter
…ice NOT being taxInc as per set zone.
|
|
||
| use Illuminate\Database\Eloquent\Relations\BelongsTo; | ||
| use Illuminate\Database\Eloquent\Relations\MorphTo; | ||
| use Lunar\Models\TaxZone; |
There was a problem hiding this comment.
We should be using the contract here.
| * Call `->save()` afterwards to persist it to the database; the `booted()` retrieved-event listener | ||
| * will then restore the zone automatically on every subsequent page load. | ||
| */ | ||
| public function setTaxZone(?TaxZone $taxZone): Cart |
There was a problem hiding this comment.
Although it would cause a breaking change, this should be added to the Cart contract.
| public function setTaxZone(?TaxZone $taxZone): Cart | ||
| { | ||
| $this->taxZone = $taxZone; | ||
| $meta = $this->meta ?? new \ArrayObject; |
There was a problem hiding this comment.
The meta column is cast with AsArrayObject::class, so $this->meta is already an ArrayObject instance when the model exists. Creating new ArrayObject as a fallback is only needed for a brand-new unsaved cart where meta was never set.
The issue is that AsArrayObject returns the same object instance on repeated access (it is not cloned), so assigning $this->meta = $meta re-assigns the same reference and may not trigger Eloquent's dirty tracking, meaning a subsequent ->save() call might not persist the meta change. The pattern should use $this->forceFill(['meta' => ...]) to ensure dirty state is marked.
| use Lunar\Exceptions\FingerprintMismatchException; | ||
| use Lunar\Facades\DB; | ||
| use Lunar\Facades\ShippingManifest; | ||
| use Lunar\Models\TaxZone; |
There was a problem hiding this comment.
Ensure we reference the contracts and not concrete classes.
|
|
||
| // Cache the TaxZone so that price model taxInc methods accounts for it in computation. | ||
| if ($cart->taxZone) { | ||
| Blink::put('lunar_cart_tax_zone', $cart->taxZone); |
There was a problem hiding this comment.
If application code calculates two carts in the same request (e.g. comparing shipping options across accounts, or a queue job processing multiple carts in the same process), the second cart's CalculateLines pass will overwrite the Blink key set by the first, meaning Price::getPriceableTaxRate() will silently use the wrong zone for calls made after the key is overwritten.
Recommendation: Key the Blink entry by cart ID: 'lunar_cart_tax_zone_'.$cart->id, and have Price::getPriceableTaxRate() accept the cart ID as a lookup hint, or pass the zone directly rather than routing it through Blink at all.
|
@alecritson I've seen your review. I'm quite caught up in these days. I'll try to get this on priority. I'll try to make amendments as per the review over the weekend |
2 participants
Lunar Current Behavior
Fixes in PR
The PR approaches this problem at cart level and Price Model level.
Cart Level
We add TaxZone property to Cart model. The typical flow is to find the user country based on IP in middleware, determine the tax zone and set it on the cart. With this, The items added to cart, will correctly give tax incl price when taxInc methods are called.
Price Model Level
When we have Product/Purchasable instead of Cart Line (Item is added to cart), we get the price from prices relation (prices attached to purchasable) and utilize
priceIncTax,comparePriceIncTaxmethods. An optional TaxZone parameter is added so you can get the tax inclusive price as per the zone of your choice. This is useful when you need it for one time for any reason.Note: You can totally ignore it if you need the prices to be as per the zone set on cart, the PR will fallback to the zone set on cart.
Previous PR: #2406
The previous PR focused strictly on Product/Purchasable display prices. It introduced a taxzone parameter to specific methods, allowing developers to:
This PR is a superset of #2406. It retains the functionality mentioned above but expands it by adding a taxZone property directly to the Cart.
IMPORTANT:
Since this PR includes all changes from #2406, please reject/close that PR if you choose to merge this one.