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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hanoii’s picture

Attached is the patch.

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

        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.

hanoii’s picture

Title: subtotal excluding tax logic is wrong » subtotal excluding tax line item logic is wrong
longwave’s picture

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.

hanoii’s picture

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.

hanoii’s picture

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.

a_c_m’s picture

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

longwave’s picture

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).

hanoii’s picture

@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.

hanoii’s picture

Status: Needs work » Needs review
TR’s picture

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.

TR’s picture

Status: Needs work » Needs review
TR’s picture

Status: Needs review » Needs work

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

TR’s picture

Bumping to bring this to the attention of longwave.

longwave’s picture

Component: Code » Taxes
Status: Needs work » Needs review
FileSize
804 bytes

Let's see what testbot thinks.

TR’s picture

Version: 6.x-2.x-dev » 8.x-4.x-dev
Issue summary: View changes
Status: Needs review » Needs work
dpovshed’s picture

TR’s picture

Version: 8.x-4.x-dev » 7.x-3.x-dev
FileSize
822 bytes

This is the patch from #16, re-rolled to apply to 7.x-3.x.

TR’s picture

Status: Needs work » Needs review
FileSize
822 bytes

Status: Needs review » Needs work

The last submitted patch, 20: 644840-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

TR’s picture

Status: Needs work » Needs review
FileSize
823 bytes

Typos again.

TR’s picture

OK, so #22 is a patch for 7.x-3.x that works.

Ideally, before this is committed, it should be accompanied by a test containing a custom line item with calculated = FALSE, to demonstrate that the subtotal excluding tax renders correctly.

TR’s picture

Bumping so this stays on my to-do list ...

It would be great if someone would test the patch in #22 ...