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.
Comment | File | Size | Author |
---|---|---|---|
#22 | 644840-22.patch | 823 bytes | TR |
| |||
#20 | 644840-20.patch | 822 bytes | TR |
#19 | 644840-19.patch | 822 bytes | TR |
#16 | 644840-uc_taxes-subtotal.patch | 804 bytes | longwave |
#4 | 644840_proper_line_item_logic_in_subtotal-2x.patch | 776 bytes | hanoii |
Comments
Comment #1
hanoiiAttached is the patch.
I can go just a little bit further and I think that
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.
Comment #2
hanoiiComment #3
longwaveSubscribing 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.
Comment #4
hanoiiIt'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.
Comment #5
hanoiiJust 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.
Comment #6
a_c_m CreditAttribution: a_c_m commentedsubscribe (i did leave a comment, but its invalid, so i've removed it)
Comment #7
longwaveA 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).
Comment #8
hanoii@longwave, do you think you can have a bit of time to review this one?
Comment #10
hanoiiComment #11
TR CreditAttribution: TR commentedThe testbot is now working, so let's give it a try.
Comment #12
TR CreditAttribution: TR commentedComment #13
TR CreditAttribution: TR commented#4: 644840_proper_line_item_logic_in_subtotal-2x.patch queued for re-testing.
Comment #15
TR CreditAttribution: TR commentedBumping to bring this to the attention of longwave.
Comment #16
longwaveLet's see what testbot thinks.
Comment #17
TR CreditAttribution: TR commentedComment #18
dpovshed CreditAttribution: dpovshed as a volunteer and at Drupal Ukraine Community commentedsee related #2933102: In email notifications 'subtotal excluding taxes' is wrong
Comment #19
TR CreditAttribution: TR commentedThis is the patch from #16, re-rolled to apply to 7.x-3.x.
Comment #20
TR CreditAttribution: TR commentedComment #22
TR CreditAttribution: TR commentedTypos again.
Comment #23
TR CreditAttribution: TR commentedOK, 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.
Comment #24
TR CreditAttribution: TR commentedBumping so this stays on my to-do list ...
It would be great if someone would test the patch in #22 ...