Can't distinguish two taxes of different percentages in the order summary

In my case I needed to display "VAT 10 %" and "VAT 20 %" above the total amount. In order to display the different tax rates on the order overview page including their percentage, I copied commerce-order-total-summary.html.twig from the commerce/modules/order/ dir to my template folder and changed the loop that prints the tax rates like this:

{% for key,adjustment in totals.adjustments %}		
	{% set taxrate = adjustment.percentage * 100 %}
	<div class="order-total-line order-total-line__adjustment">
		<span class="order-total-line-label">{{ adjustment.label }} {{ taxrate }} %</span><span class="order-total-line-value">{{ adjustment.total|commerce_price_format }}</span>
	</div>
{% endfor %}

I'm using a custom tax rate resolver based on the example posted by @Megachriz to apply different tax rates.

@paper boy
I just noticed this issue on a commerce site in development. Thanks for your code example! You were using a code example posted by me, now I'm using a code example posted by you! :D

@bojanz
Would it make sense to add a setting to the tax type for if and how you want to display the percentage of a tax or do you recommend people to add it in theirselves in templates?

@paper boy
I adjusted your code a bit as I was getting a percentage shown for shipping costs as well. This resulted into "Shipping 0%".

{% for key,adjustment in totals.adjustments %}
  <div class="order-total-line order-total-line__adjustment">
    <span class="order-total-line-label">{% if adjustment.type == 'tax' %}{{ adjustment.percentage * 100 }}% {% endif %}{{ adjustment.label }}</span><span class="order-total-line-value">{{ adjustment.amount|commerce_price_format }}</span>
  </div>
{% endfor %}

In my case I put the percentage to show before the label.

I had to make a similar adjustment in the commerce-order-receipt.html.twig template:

{% for adjustment in totals.adjustments %}
<tr>
  <td>
    <p style="margin-bottom: 0;">
      {% if adjustment.type == 'tax' %}{{ adjustment.percentage * 100 }}% {% endif %}{{ adjustment.label }}: {{ adjustment.total|commerce_price_format }}
    </p>
  </td>
</tr>
{% endfor %}

I did this with a simple patch to use the label rate instead zone label. Here is my patch.

Thanks for your additions, @MegaChriz! I created a patch from #4, #6 and #7. Tested it on Drupal 8.5.4 and Commerce 2.x-dev.

Good point bojanz I used with custom no with rest of types.

Thanks

Can confirm, the patch from #10 works on Drupal Commerce 8.x-2.8 and current DEV. Any reason why the patch didn't make it into 8.x-2.8 release?

Hi, the patch looks fine.

Would be possible to do the same for discounts: instead of a generic "discount" label it could be printed the discount name.
EDIT: nevermind, there is already an issue about this bug: https://www.drupal.org/project/commerce/issues/2770731 :-)

Updated patch for the latest 8.x-2.x-dev release

I've run into this problem and had a slightly different approach - rather than changing the template to conditionally display the percentage, I incorporated it directly into the label at the point the adjustment is created. I've attached a patch which does this, however I'm leaning towards the template patch from #15 being a better solution as it has greater flexibility to override how the adjustment is presented through the theme layer.

A slight mod to #15 for whitespace consistency with subtotal and total lines in the order summary

The approach in #15/#18 looks better to me, compared to the one in #16, due to flexibility. Thanks!

-

#10 worked for me! Thank you very much!

The approach in #16 looks like the right one to me, although I'd probably just output the zone display label and the percentage (i.e no rate label). We'd still need to figure out what should be the default output, perhaps the following instead?

>
t('@display_label (@percentage);

Which would output VAT 17% for example.

This patch adjusts the label of tax adjustments in preprocess functions, using the suggestion from #22.

that isn't my suggestion :). My suggestion is to update LocalTaxTypeBase (where we add the adjustment, similarly to what's done in #16.

@jsacksick
I'm not too sure about the approach in #16. That makes the tax label "to be set in stone". And since the translation happens on creation, on multilingual sites you might get different displays of the tax labels - it then depends on which language was active when the user placed the order. The approach in #23 makes the tax label to be displayed in a more consistent way, since then the translation happens upon viewing the order (in Dutch the percentage before the tax display label is more common).

I agree with @MegaChriz, I'm currently using #18 in production to solve this problem.

Updated patch 15/18 for new Drupal Commerce

Hidden my original files as #18 now superceded by #27 re-roll and #16 generally agreed to be an inadequate approach. I'm now using #27 following upgrade to 2.24, but I've also taken a look at 23 and think this preprocess approach has equal merit, perhaps a little better for keeping the logic out of the template and in the preprocess function.

Patch #23 worked for me flawlessly. Thank you!

Took the idea from patch #23 (@MegaChriz) and extended it to be themable. this way the default behavior remains unchanged but the template can be overridden to accommodate for displaying percentages, or whatever needed.

sadly though, my patch does not fix cases where tax adjustments are shown in other modules (I'm specifically looking at commerce_invoice).

Changed MR from hooking into specific theme hooks to using hook_preprocess() so 3rd party usage of order totals are covered too.

Just ran into the same requirement! Even if only one tax is used, it would be super helpful to show the vat rate used for information and easier review.

We'll have a look at this soon to push things forward.

Main branch was way too far behind. Created a new 3.0.x branch and manually applied changes by @attisan.

The new twig template is quite different from the original one. I'll adjust this accordingly.

Adjusted accordingly.

I've hidden the patches to focus on the MR now and get this fixed! :)

I disagree with this approach... I think perhaps it'd be better to introduce an event from the OrderTotalSummary service, and have the Tax module react to it. From there we could alter the adjustment label to include the tax percentage.

Thanks @jsacksick we'll implement that as separate MR tomorrow. Having the VAT rate shown is definitely a big win!

Alright, all done. Ready for review!

Thanks @grevil! LGTM and seems to do what @jsacksick suggested?
I only had a little docs comment, everything else seems fine to me.

Could you post a screenshot with the result?

Here is a screenshot (although in german and using a custom theme, but anyway):

EDIT: Mwst. = VAT

Hm... I think we need to fix the naming as well...

Maybe OrderSummaryBuildTotalsEvent? (a bit wordy though). But just BUILD_TOTAL could confuse people and they'd be under the impression that the event is dispatched when the order total is recalculated.

The event constant should probably be OrderEvents::ORDER_SUMMARY_BUILD_TOTALS.
Also, the alteration should probably only be done for VAT adjustments... So probably makes sense to do this only if the adjustment is included? (as VAT is always included).
Also, the way it is currently done makes the label untranslatable...

It should be something like:

$this->t('@zone_display_label (@percentage%)', [
        '@percentage' => Calculator::multiply($adjustment['percentage'], '100'),
      ]);

Or maybe we keep the original label just like you did if we can't easily get the zone display label.
Also, just so you know, I'd like to wait for feedback from other people that previously contributed here before moving forward with this.

Also, the alteration should probably only be done for VAT adjustments... So probably makes sense to do this only if the adjustment is included

This is already the case, as we are checking the adjustment type in modifyAdjustmentsLabels. Or am I misunderstanding you here?

It should be something like:

$this->t('@zone_display_label (@percentage%)', [
'@percentage' => Calculator::multiply($adjustment['percentage'], '100'),
]);

Not quite sure what the zone display label is here?

@grevil: Checking the adjustment type isn't sufficient, there is also the Sales tax scenario and other taxes as well.

@jsacksick but wouldn't it be always preferable to have the tax percentage displayed on adjustments of type "tax"?

Looks like we need some more fine-graded number formatting:
e.g. 0.09975 * 100
https://git.drupalcode.org/project/commerce/-/blob/3.0.x/modules/tax/src...

So that the number is formatted correctly.

Looks like we need some more fine-graded number formatting:
e.g. 0.09975 * 100

Alright, should be fixed now using the commerce price calculator (as @jsacksick already mentioned in #53 anyway).

Back to needs review!

@grevil: Perhaps we need to allow resetting the whole array? And not just adjustments? I mean, can't really think of a usecase altering the subtotal or total... But I guess it doesn't hurt?

Alright, done!

I implemented it, so that the subtotal and total is only overwritten for the "buildTotals" array, meaning that $order->getSubtotalPrice() and $order->getTotalPrice() will have the unmodified value set.

Static patch for the time being.

Thanks @grevil! Works great and as expected so far!!

@grevil: My suggestion from #59 was to pass the whole array to the subscriber, so it can easily be read / altered. Eventually, additional data could be injected for use in a custom template as well.

So perhaps a getTotals() and a setTotals() helper as opposed to getters / setters for the subtotal & total.

In any event, as mentioned on Slack, I'd like to get feedback from the community on this considering 3-4 different approaches have been previously implemented here.

Alright, we now pass the whole array!

@anybody: you mean RTBCED by your company :), as mentioned, waiting for other community members to chime in here, especially the ones that contributed patches previously. But from a pure code point of view, this looks good now (as I wanted it to look at least).

@jsacksick yes, but there's no option for that. Waiting longer without active people here won't help much I think? The world moves on and Drupal needs to do the same...

But yes of course always important to have as much feedback as possible. Having this RTBC'd doesn't mean will be accepted immediately by maintainers. Thanks :)

There is 2 remaining problems with the MR though. The event dispatcher has a NULL default value. We shouldn't make it optional, or we should conditionally dispatch the event.
Also, I think the label alteration makes the label untranslatable.

$adjustment['label'] = $originalLabel . ' (' . $taxPercentage . '%)';

So perhaps a translatable string here with a context? commerce_tax? t('@label (@tax_percentage%')); or something similar.

So perhaps a translatable string here with a context? commerce_tax? t('@label (@tax_percentage%')); or something similar.

Definitely +1 thanks for the feedback!

I see that on a site where I had this issue I'm still using the solution from #7.

I do plan to try to update that site to as much D11 compatible releases as possible later this month, so perhaps I can test the changes from this issue along with it. But no promises, I noticed that when I tried to do composer update now, quite a few patches could no longer be applied. It depends on how much time it will take to update these if there will be any time left to look at this issue.

Back to NW based on #68

Alright, adjusted accordingly, thanks for the feedback @jsacksick!

as mentioned, waiting for other community members to chime in here, especially the ones that contributed patches previously. But from a pure code point of view, this looks good now

I don't think anyone will chime in here anymore. Can we get this fixed?

I have no problem with the new event subscriber etc... But the label change should have been introduced prior to tagging 3.0.0 as I'm pretty sure the change introduced will not please anyone... Merging this will cause the adjustment label to change instantly on live sites as well...

Also, there are test failures, and we have no new tests testing the new label... So not 100% convinced we can just go ahead and commit this change as is.

We might need to merge the 3.x changes and kick off the tests to see where things stand.

I made some changes / improvements to the patch.

1. We were not checking that the adjustment percentage was not empty from the subscriber (this could have caused errors, altering adjustments with no percentage set)...
2. All tax adjustment labels were altered, regardless if the adjustment was added by a non local tax type plugin (For example, we have to make sure to not alter the adjustments added by the Avatax module as the module provides its own custom labels)
3. Renamed the subscriber to OrderTotalSummary
4. Fix the variables casing to stay consistent with the Commerce codebase and especially code within the same method (i.e changes to the OrderTotalSummary where camelCase was introduced)

Actually, now having second thoughts on the approach... Perhaps we should simply add a tax type setting... "Append the tax rate to the adjustment label"... This would remove the need for the subscriber and can be easily toggled within the UI.

Also, this can be added to the LocalTaxTypeBase class only so it doesn't affect tax type plugins using external APIS like Avatax.

Hmmm... Actually, I think we did it this way so that the alteration is done runtime rather than affecting the adjustment that is stored in DB. This way the adjustment label would always be on the current site language...

Hmmm... Actually, I think we did it this way so that the alteration is done runtime rather than affecting the adjustment that is stored in DB. This way the adjustment label would always be on the current site language...

Can't remember exactly anymore. But we could still add a new setting (off by default) which prevents the subscriber to do anything, until activated!

EDIT: Or maybe there is something like an conditional subscriber in drupal / commerce? Unsure about that.

@jsacksick, so what are your final thoughts on this? I think the current approach is pretty good!

Ok, so let's do the following:

1. Add a new setting to LocalTaxTypeBase “Display tax rate in adjustment label” (key: "display_tax_rate_in_label")
2. The new setting should have a description: “Displays the tax rate percentage next to the adjustment label, e.g., ‘VAT (20%)’.”

Then we adjust the following logic to check for the setting:

$local_tax_type_ids = array_keys(array_filter(
      TaxType::loadMultiple(),
      fn($tax_type) => $tax_type->getPlugin() instanceof LocalTaxTypeInterface
 ));

Also, let's change modifyAdjustmentsLabels() to alterAdjustmentsLabels() (nitpicking here but...)

To also check for the setting.
We could maybe set the default value to TRUE, and people can opt out if they're annoyed.

Alright, on it!

Done! Works great and as expected, please review!

To make the code more efficient, let's pass the following to shouldAlterAdjustment().

 $tax_type_prefixes = [];
  foreach (TaxType::loadMultiple() as $tax_type_id => $tax_type) {
    $plugin = $tax_type->getPlugin();
    if ($plugin instanceof LocalTaxTypeInterface && !empty($plugin->getConfiguration()['display_tax_rate_in_label'])) {
      $tax_type_prefixes[$tax_type_id] = true;
    }
  }

And then update shouldAlterAdjustment() to:

protected function shouldAlterAdjustment(Adjustment $adjustment, array $tax_type_prefixes): bool {
  if ($adjustment->getType() !== 'tax' ||
      !$adjustment->getSourceId() ||
      !$adjustment->getPercentage()) {
    return FALSE;
  }

  // Check the prefix before the first pipe.
  [$prefix] = explode('|', $adjustment->getSourceId(), 2);
  return isset($tax_type_prefixes[$prefix]);
}

We're also missing adjustments to the schema.yml.

All done! Please review!

Let's update the OrderTotalSummaryTest to test this behavior and then we are good to go. We have no tests ensuring this functionality works so there is a risk in breaking this in the future without noticing. So back to "Needs work", for the tests.

Tests were written, if these are green, I'll merge this.

Great stuff! Thank you @jsacksick!