After extensive debugging I found out that uc_order_load() is updating the order total by the following code:

  // Make sure the total still matches up...
  if (($total = uc_order_get_total($order)) !== $order->order_total) {
    db_query("UPDATE {uc_orders} SET order_total = %f WHERE order_id = %d", $total, $order->order_id);
    $order->order_total = $total;
  }

This is causing issues and I think it's wrong even from a semantic point of view, a load should never be altering stored information.

Further more, this is causing actual problems:
1. I have a site in which, after a while, some products were removed from the db, but the information remained (as it should) on the order products table. However, some of these products had tax applied, based on their content types. Because the nodes are not there anymore the content type cannot be used to match against the tax rules, and thus, no tax being applied generating a different order total when the order is viewed. This was detected by a client going over old orders. This is wrong and something should be done.

So I wonder, is this and the other bit of code in uc_order_load() that changes order information necessary? Shouldn't those values be set on save or order alteration exclusively? And a more important question, is it safe to remove that code? I think that my only solution is to do that, but I am worried on the implications, because changing such a core function of ubercart is something I rather not do lightly.

Any help/suggestion/discussion is really appreciated.

Comments

jaykainthola’s picture

jaykainthola commented

I am also getting this issue. In my project I need to add some custom condition for VAT using uc_vat module. The total of the order is showing correct in review page but when I am viewing order from admin, I am getting wrong result.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The order_total field in the database should always match the sum of the products and line items, it is effectively used as a cache to avoid recalculating the order total every time it is needed. The recalculation is done here to ensure that the displayed order total in summaries, Views, etc. is always synchronised with the actual contents of the order.

If the order total is changing unexpectedly, this is likely a symptom of the wider issue #854638: order products price should locked in altered price, which is difficult to fix without breaking backward compatibility with existing sites.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

I have faced a very similar issue, my situations were two:

- Older orders (close to ubercart 2.0) were storing tax information on a different place than the new versions of ubercart, so the recalculation was wrong because new vat rate was being applied. I run a one-off routine to gather the tax rate from where it was stored (on the line item) and store it where it was looked now (order data).

- Misisng products
Another source of wrong total recalcualtion is if you delete your product, and if that product has VAT at the time of being bought. Old price information is actually stored on the order products table, so you would think it's "safe" to remove products afterwards. Problem is, as the order total is being recalculated on order_load, it needs all the information of the product, including it's node-type used to decide whether to apply tax rates or not. So this was, on my case, another source for problems. The solution to this was to create bogus entries on the node table for those node ids that were removed and had VAT, with it proper content type so the vat recalculation worked as expected.

Hope it helps!
a.=

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

@longwave, btw, I am not sure recalculation an order total is of any use. Thinking as a sort of cache is not actually what's going on because it's being 'recalculated' on every order_load, so not much of a cache there. Even more, order can be loaded for other information rather than totals, so rather than being a cache is more like a performance hit (although I know it's minuscule).

It would make more sense to just rely on the order total stored on the db and not recalculate it unless an update on the order is made (product alter/removed, order line item altered/removed/ etc.).

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@hanoii: It is a sort of cache when you load the order overview page, or use Views to display a list of orders and their totals - uc_order_load() isn't called for each order in the list in these cases (AFAIK), so this is a hacky way of ensuring the order_total is usually up to date. I agree it should be fixed, but ensuring that all product and line item operations cause the total to be updated is difficult, especially when contrib comes into play where the order contents may change outside of Ubercart core's control.

Ideally, order total would not be stored in the database at all and always calculated when required, as it is only a summary of other data; this would avoid this problem entirely, but doing that now would also affect contrib that expects to be able to use this value directly from the database.

tajindersingh’s picture

tajindersingh commented

subscribing

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Closed (works as designed)

This isn't the best design but if order_total is treated as a cached computed value, then I think this makes some kind of sense; there should be no reason why order_total should not actually match the real order total. Perhaps this can be redesigned in the future but there is little point changing this in the existing 6.x or 7.x branches now.