The problem is that the logic uses to calculate the subtotal excluding tax in the line item is adding up all line items except tax and subtotal. This used to work fine until now but I just defined a line item with calculated => FALSE (as subtotal and total) and it only got added to the subtotal excluding tax line. The reason is that this property is not queried properly in the logic.

For reference, you can see uc_line_items_calculate().

It took me a while to get to this. Even more because I use uc_vat and that module overrides a few functions but borrowed the same logic into it.

The patch I will submit I haven't been able to test it myself properly, because I tested the same patch for uc_vat, but I'd think it's pretty simple and it should definitely work.

Files: 
CommentFileSizeAuthor
#16 644840-uc_taxes-subtotal.patch804 byteslongwave
PASSED: [[SimpleTest]]: [MySQL] 2,146 pass(es).
[ View ]
#4 644840_proper_line_item_logic_in_subtotal-2x.patch776 byteshanoii
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 644840_proper_line_item_logic_in_subtotal-2x.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#1 644840_proper_line_item_logic_in_subtotal.patch649 byteshanoii
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 644840_proper_line_item_logic_in_subtotal.patch.
[ View ]

Comments

StatusFileSize
new649 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 644840_proper_line_item_logic_in_subtotal.patch.
[ View ]

Attached is the patch.

I can go just a little bit further and I think that

<?php
       
if ($line_item['type'] == 'subtotal') {
          continue;
        }
?>

can be removed from that bit of the code in which the patch applies, because subtotal is one of the line items with calculated = FALSE, but just leaving it there for now

Also it may be worth looking if this has been missed in some other place, I looked but you probably will know better.

Title:subtotal excluding tax logic is wrongsubtotal excluding tax line item logic is wrong

Status:Active» Needs review

Subscribing here so I can patch uc_vat with a similar fix once this has been discussed. There is also #600478: Subtotal excluding taxes doesn't use uc_price() which might be worth looking into at the same time, but I have no time to look at either of these right now.

StatusFileSize
new776 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 644840_proper_line_item_logic_in_subtotal-2x.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

It's been a while, but I just had to review a bug I found on a site of my own and it was because of this patch. Basically the subtotal excluding ... line item was not being displayed because the $has_taxes variable was never being set because 'tax' line item has 'calculated' to FALSE, which is right.

Attached is a new patch with a revised logic, properly setting the $has_taxes variable and with the proper calculated logic, also I did clean up the code a little bit removing that bit of unneeded code.

Just took a quick look at #600478: Subtotal excluding taxes doesn't use uc_price() and I noticed it modifies the same bit of code, so probably both should be reviewed in parallel, although I think this one is simpler and covers one specific issue while the other tries to handle a few different problems.

subscribe (i did leave a comment, but its invalid, so i've removed it)

A near-identical version of this patch has been committed to uc_vat in #644838: subtotal excluding tax line item logic is wrong, bumping this as I believe hanoii's reasoning is correct and think this should also be applied to Ubercart (however, I haven't had time to apply and test it on a plain Ubercart install yet, so I'm not marking as RTBC for this reason).

@longwave, do you think you can have a bit of time to review this one?

Status:Needs review» Needs work

The last submitted patch, 644840_proper_line_item_logic_in_subtotal-2x.patch, failed testing.

Status:Needs work» Needs review

Version:6.x-2.2» 6.x-2.x-dev
Status:Needs review» Needs work

The testbot is now working, so let's give it a try.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 644840_proper_line_item_logic_in_subtotal-2x.patch, failed testing.

Bumping to bring this to the attention of longwave.

Component:Code» Taxes
Status:Needs work» Needs review
StatusFileSize
new804 bytes
PASSED: [[SimpleTest]]: [MySQL] 2,146 pass(es).
[ View ]

Let's see what testbot thinks.