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

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.

grevil → changed the visibility of the branch 2881056-make-tax-label-themable to hidden.

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!

grevil → changed the visibility of the branch 2881056-make-tax-label-themable-3.0.x to hidden.

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

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 🐛 Can't distinguish two taxes of different percentages in the order summary Needs review 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 in #7.

I do plan to try to update that site to as much D11 compatible releases as possible, 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 that is a change we probably should have 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"... Would remove the need for the subscriber and can be easily toggled within the UI.

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 “Show tax rate in adjustment label” (key: "show_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:

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

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]);
}

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!

Automatically closed - issue fixed for 2 weeks with no activity.