Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lost penny management #6391

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Conversation

piloujp
Copy link
Contributor

@piloujp piloujp commented Apr 11, 2024

In case of a penny difference between invoice total and sum of numbers on the same invoice, this modification will adjust the total to equal the sum of other numbers.
The one penny difference is still visible between invoice with prices including tax and invoice with prices without tax, but one by one, they are coherent, all numbers add correctly.

See forum thread 'The Lost Penny of ZC'.

Saving results of each module allows adding them in order total to check penny loss.
This total could be used as the final total instead of the actual one, eliminating penny loss and making invoice numbers always add correctly to give the final total.
Removed discount coupon and GV modification which belongs to another branch.
@proseLA
Copy link
Sponsor Contributor

proseLA commented May 16, 2024

i will attempt to be brief. as i stated here, the problem resides in the need for the ot_total module.

lets go over why i do not like your code:

  • it is overly complex.
  • it hard codes various elements into a new array in the $order->info object whose key is some new name that you think is somehow tied to the order total module.
  • it does not account for other store owners developing their own order total modules where they run into the same problem that you have exposed here.

and i would solve this problem completely differently. i would look at this method:

function process()

and potentially change it to something like this:

function process()
    {
        global $order, $currencies;
        $order_total_array = [];
        if (is_array($this->modules)) {
            $this->notify('NOTIFY_ORDER_TOTAL_PROCESS_STARTS', ['order_info' => $order->info]);
            foreach ($this->modules as $value) {
                $class = substr($value, 0, strrpos($value, '.'));
                if (!isset($GLOBALS[$class])) continue;
                $GLOBALS[$class]->process();
                $this->notify('NOTIFY_ORDER_TOTAL_PROCESS_NEXT', ['class' => $class, 'order_info' => $order->info, 'ot_output' => $GLOBALS[$class]->output]);
                if (empty($GLOBALS[$class]->output)) {
                    continue;
                }
                for ($i = 0, $n = sizeof($GLOBALS[$class]->output); $i < $n; $i++) {
                    if (zen_not_null($GLOBALS[$class]->output[$i]['title']) && zen_not_null($GLOBALS[$class]->output[$i]['text'])) {
                        $order_total_array[] = [
                            'code' => $GLOBALS[$class]->code,
                            'title' => $GLOBALS[$class]->output[$i]['title'],
                            'text' => $GLOBALS[$class]->output[$i]['text'],
                            'value' => $GLOBALS[$class]->output[$i]['value'],
                            'sort_order' => $GLOBALS[$class]->sort_order,
                        ];
                    }
                }
            }
        }
        $orderTotal = 0;
        $notCalc = ['ot_total',];
        foreach ($order_total_array as $key => $totalItem) {
            if ($totalItem['code'] === 'ot_total') {
                $totalKey = $key;
            }
            if (in_array($totalItem['code'], $notCalc)) {
                continue;
            }
            $orderTotal += $currencies->value($totalItem['value'], false, $order->info['currency']);
        }
        if (empty($totalKey)) {
            return $order_total_array;
        }
        $order_total_array[$totalKey]['value'] = $orderTotal;
        $order_total_array[$totalKey]['text'] = $currencies->format($orderTotal, false, $order->info['currency'], $order->info['currency_value']);

        $order->info['total'] = $orderTotal;

        return $order_total_array;
    }

we are now recalculating the order total based on the addition of the all of the order total modules; have the ability to exclude certain modules if their total is included elsewhere and hopefully make address your lost penny issue.

the issues i see in my code are:

  • it does not display the correct total on the checkout confirmation page (at least it did not, i tested it on a dev site with OPC).
  • i rarely use foreign currencies, and in the 2 instances above i am not using the $calculate_using_exchange_rate as true.

perhaps you can solve those 2 issues.

and let us know if this solution works for you.

again, while i applaud your effort, i would hope we do not see code this confusing merged into the repo.

@piloujp
Copy link
Contributor Author

piloujp commented May 17, 2024

i will attempt to be brief. as i stated here, the problem resides in the need for the ot_total module.

lets go over why i do not like your code:

* it is overly complex.

* it hard codes various elements into a new array in the `$order->info` object whose key is some new name that you think is somehow tied to the order total module.

* it does not account for other store owners developing their own order total modules where they run into the same problem that you have exposed here.

and i would solve this problem completely differently. i would look at this method:

I must say I have a hard time understanding your comments:

  • What is 'overly complex'? There is nothing complicated in this PR... Is it because it modifies many files? Or is it because of the line that determinates if a penny correction is necessary?
  • When you speak of hard coded elements, I suppose you refer to keys associated with modules results like, for example, 'cod_fee' in $order->info['option_modules']['cod_fee'] = $cod_cost;.
    If it is that, I agree it is not a good practice and I already changed it to: $order->info['option_modules'][$this->code] = $cod_cost;
  • I don't see why this PR does not account for other owners developed ot modules, honestly...

This said your approach is interesting, although in the way you show it here it can't work as all modules have already done their calculations and displayed results. It happens on the line with this code: $GLOBALS[$class]->process(); just after the beginning of the foreach loop.
I did work on the idea though, and came out with this:

    function process()
    {
        global $order, $currencies;
        $order_total_array = [];
        $orderTotal = 0;
        $mod_value = 0;
//        $notCalc = [];
        if (is_array($this->modules)) {
            $this->notify('NOTIFY_ORDER_TOTAL_PROCESS_STARTS', ['order_info' => $order->info]);
            foreach ($this->modules as $value) {
                $class = substr($value, 0, strrpos($value, '.'));
                if (!isset($GLOBALS[$class])) continue;
                if ($GLOBALS[$class]->code !== 'ot_total') {
                    $GLOBALS[$class]->process();
                } else {
                    $mod_value = $orderTotal;
                    $order->info['total'] = $orderTotal;
                    $GLOBALS[$class]->process();
                }
                $this->notify('NOTIFY_ORDER_TOTAL_PROCESS_NEXT', ['class' => $class, 'order_info' => $order->info, 'ot_output' => $GLOBALS[$class]->output]);
                if (empty($GLOBALS[$class]->output)) {
                    continue;
                }
                for ($i = 0, $n = sizeof($GLOBALS[$class]->output); $i < $n; $i++) {
                    if (zen_not_null($GLOBALS[$class]->output[$i]['title']) && zen_not_null($GLOBALS[$class]->output[$i]['text'])) {
                        if ($GLOBALS[$class]->code !== 'ot_total') {
                            $mod_value = $GLOBALS[$class]->output[$i]['value'];
//                            if (($GLOBALS[$class]->code !== 'ot_tax' || DISPLAY_PRICE_WITH_TAX !== 'true') && !in_array($GLOBALS[$class]->code, $notCalc)) {
                            if ($GLOBALS[$class]->code !== 'ot_tax' || DISPLAY_PRICE_WITH_TAX !== 'true') {
                                if(substr($GLOBALS[$class]->output[$i]['text'], 0, 1) === '-') {
                                    $orderTotal -= $currencies->value($GLOBALS[$class]->output[$i]['value'], true, $order->info['currency']);
                                } else {
                                    $orderTotal += $currencies->value($GLOBALS[$class]->output[$i]['value'], true, $order->info['currency']);
                                }
                            }
                        }
                        $order_total_array[] = [
                            'code' => $GLOBALS[$class]->code,
                            'title' => $GLOBALS[$class]->output[$i]['title'],
                            'text' => $GLOBALS[$class]->output[$i]['text'],
                            'value' => $mod_value,
                            'sort_order' => $GLOBALS[$class]->sort_order,
                        ];
                    }
                }
            }
        }

        return $order_total_array;
    }

It does the job although with actual ZC code, I don't think it was meant to be done here. Solution looks nicer in simpler too. But it is just an appearance. There are two potential problems that can occur:
First is the code to exclude some module from total, actually commented out.
It will only remove the module result from final total, but nothing would be changed in order->info and removed module will still be taken in account by other modules calculations. It might be possible to completely remove it from processing, like if it was not installed, if it is you meant.
Second is when a customer uses a currency different from the default one.
Currency conversion is normally done at display time, but with this solution it as to be done before each module total is added. If not, we are at square zero with two different calculation methods for total that will end up with a difference at the end. I had to set the currency conversion parameter to false in to_total.
Problem is that the converted total is saved in order->info in the displayed currency, but all other values are saved in default currency, and order->info is later used to save order data to database... Unfortunately, I can't see any solution for that.

@piloujp
Copy link
Contributor Author

piloujp commented May 20, 2024

I found a solution to the second problem of this code when using a different currency. By replacing $order->info['total'] = $orderTotal; by $order->info['total'] = $_SESSION['currency'] !== DEFAULT_CURRENCY ? $currencies->value($orderTotal, true, DEFAULT_CURRENCY) : $orderTotal;
Total is converted back to default currency before being saved. Then ot_total module just uses it as in original code and does not need any modification. The only inconvenient is that there is one more conversion, but the original PR does something similar. With numbers I tested, there was no visible error.

@piloujp
Copy link
Contributor Author

piloujp commented May 27, 2024

Well, if the new code is preferred by most, I could change this PR. But I would appreciate some other opinions first. I think only the code to exclude a module should not be included. As long as calculations are not made in the ot_total class, it is not a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants