Even while on a regular order page the 'Subtotal excluding taxes' is correct, it is wrong in email notifications. Just because it is adding up the whole total sum there.

The attached image illustrated, and the attached patch fixes this.

Picture Illustrating Wrong Subtotal

Somewhat related task: #644840: subtotal excluding tax line item logic is wrong. Probably adding the value like 'calculated' or so to the special items array will cure that issue as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpovshed created an issue. See original summary.

TR’s picture

Status: Needs review » Needs work

The 'total' should not show up in the list of line items, because 'total' is a 'display_only' item. So while your fix will work for this case, it doesn't fix the underlying problem that 'total' is not supposed to be in the list of line items.

Perhaps this can be fixed by looping over $order->getLineItems() instead of $order->line_items. Can you try that?

dpovshed’s picture

Status: Needs work » Needs review
FileSize
685 bytes

@TR, your idea worked perfectly!

Thus, here is an updated patch attached.

Status: Needs review » Needs work

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

TR’s picture

Status: Needs work » Needs review

If you could try one more thing for me ... remove the whole is_array() check and verify it still works for you - no need for a new patch. getLineItems() will always return an array - it will be an empty array if there are no line items - so there should be no need for this check.

I will try to write a test case so that this bug won't happen again, then I'll commit this fix.

dpovshed’s picture

@TR, I tested this, and, as expected it worked in my case.

Surely, the called function returns an array in all cases.

After removing is_array() I also realized that we do not need a whole if() condition, so I decided to make a patch so you probably are all set here. Just need to make a test.

TR’s picture

Version: 8.x-4.0-alpha5 » 8.x-4.x-dev

Thanks, looks good. I'm still working on the tests, I have a lot of other things to do so it's taking me longer than I expected.