Issue with discountTotal and fixed value discount

[Sartoric]

Today I've faced an issue due to the rounding for the property discountTotal.

Something mentioned also here : #48

Long story short. I apply some fixed amount custom coupon to the cart, so I convert the value to percentage based on cart total. I already have a percentage discount applied to the cart.

discount	value
%	1,96
Coupon	25
Total	26,95 (clearly wrong 😁 )

The issues seems to be due to the discountFloat() getting for each item in the cart the already rounded discountTotal value (and of course to the fact that the discount is a fixed value converted to percentage)

Avoid rounding at Calculator level seems to solve (or at least minimize the issue) so I'll send a PR to update the Calculator.

Maybe we could rethink if this can be applied to other totals too.

[Sartoric]

Based on a quick check, for sure priceTotal and subtotal needs a rework too, and very likely priceTarget

[Sartoric]

I came to conclusion 😁 that it's probably better to build a custom calculator (as @bumbummen99 suggested somewhere) as issues are more related to a combination of factors (value to percentage discount, starting prices with 5 decimals etc.) and the Default calculator should works fine in most of the cases.

I'm closing the PR

[Sartoric]

Just for everyone knowledge 😁

This is my custom calculator

       switch ($attribute) {
            case 'discount':
                return $cartItem->price * ($cartItem->getDiscountRate() / 100);
            case 'tax':
                return $cartItem->priceTarget * ($cartItem->taxRate / 100);
            case 'priceTax':
                return round($cartItem->priceTarget + $cartItem->tax, $decimals);
            case 'discountTotal':
                return $cartItem->discount * $cartItem->qty;
            case 'priceTotal':
                return $cartItem->price * $cartItem->qty;
            case 'subtotal':
                return $cartItem->priceTotal - $cartItem->discountTotal;
            case 'priceTarget':
                return round(($cartItem->priceTotal - $cartItem->discountTotal) / $cartItem->qty, $decimals);
            case 'taxTotal':
                return round($cartItem->subtotal * ($cartItem->taxRate / 100), $decimals);
            case 'total':
                return $cartItem->subtotal + $cartItem->taxTotal;
            default:
                return;
        }

Everything that is used in the Cart::watheverFloat() functions is left unrounded

Then, just to keep it clean I've extended the Cart class adding the following method to round values using the config options

    /**
     * Get the Formatted number as a float.
     *
     * @param $value
     * @param null $decimals
     * @param null $method
     *
     * @return float
     */
    private function numberRound($value, $decimals = null, $method = null)
    {
        if (is_null($decimals)) {
            $decimals = config('cart.format.decimals', 2);
        }
        return round($value, $decimals, $method);
    }

and I've overridden the Cart::watheverFloat() functions in order to use it (since I still need float rounded values) without breaking existing code, but adding a way to get unrounded values too.

    public function subtotalFloat($round = true)
    {
        $result = $this->getContent()->reduce(function ($subTotal, CartItem $cartItem) {
            return $subTotal + $cartItem->subtotal;
        }, 0);
        if ($round) {
            return $this->numberRound($result);
        }
        return $result;
    }

@bumbummen99 maybe we can change at least the Cart class ? I don't know 😁

[bumbummen99]

Hey buddy, glad to see you are still with us :) I hope you are well and good 🙏

The changes look good, just spin a PR if you want it added to the lib :)

Tought i feel we could really simplify these calculation issues by using MoneyPHP, it has some rounding settings included and would even allow for integer based calculation if i recall it correctly. I have also tought about just using polymorphic relation ship for the models instead of serializing the content, that would simplify loading and use in APIs / parallel editing, in combination with an extra package or trait to permanently store a Cart for documentation purposess of the checkout. Also it might make sense to bind the Calculator to the ServiceContainer for easier injection/resolution and some persistence between calls.. So many ideas but not so much time :/

[Sartoric]

Tnx. Yes I'm fine, busy but fine 😁 I'll peek MoneyPHP later today

Let's split all the other stuff in enhancement requests, I'll probably will have some time to look into it the next weeks. Polymorphic relations could be overkill sometimes

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days