🇮🇱Israel @jsacksick

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.

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

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.

Ok, I actually committed a followup change, which calls allowedIfHasPermission() which basically calls allowedIf() which does the following:

return $condition ? static::allowed() : static::neutral();

So under the hood, neutral() is called if the user doesn't have the manage permission.

Maybe we should have went with neutral().

Best Practice:
Use neutral() when your handler can't definitively decide or you want to allow flexibility for other modules.

Use forbidden() only when you're certain that access must be denied, no matter what.

Just wondering if we shouldn't have went with neutral() similarly to the EntityAccessControlHandler provided by the entity module.

Like the following:

      if ($entity instanceof EntityPublishedInterface && !$entity->isPublished()) {
        if ($account->id() != $entity->getOwnerId()) {
          // There's no permission for viewing other user's unpublished entity.
          return AccessResult::neutral()->cachePerUser();
        }

        $permissions = [
          "view own unpublished {$entity->getEntityTypeId()}",
        ];
        $result = AccessResult::allowedIfHasPermissions($account, $permissions)->cachePerUser();
      }

I think it's not possible to grant access from a hook_entity_access() when forbidden() is explicitly called

Too bad I didn't see this prior to tagging 3.1.0, merged in 3.x, thanks!

jsacksick → made their first commit to this issue’s fork.

jsacksick → changed the visibility of the branch 3416998-stop-unsetting-the to hidden.

jsacksick → changed the visibility of the branch 3.x to hidden.

Attaching a static patch that applies to 3.x.

I'm wondering if I should only set the data flag for logged in customers. Perhaps no need to store extra data for anonymous customers... What do you think?

Merged 🎉, thanks everyone!!

Ok, I merged the 3.x changes, and we're now outputting both. Let's see if the tests are green. If they are, I'll merge this. I also tweaked the descriptions a bit.

Hm now that ✨ Display 'default_step' property of @CommerceCheckoutPane plugins in administration Needs work is in. This needs to be rebased, though I'm not curious to see how it'd look like with both piece of information displayed...

Looks better with GIN, will commit that. Thanks for the contribution!

I'd say this is a nice UX improvement. However it doesn't output as nicely as in the screenshot on my local test installation.
See the screenshot below:

Is this still happening now that ✨ Remove Commerce Product dependency from Commerce Cart and Commerce Checkout Needs review was addressed? Commerce checkout no longer depends on commerce_product, I believe this issue is outdated. Feel free to reopen if I'm in error.

Do we really have to load the order afresh from the DB? This can be quite expensive in terms of performance... I'd prefer to avoid that if possible...

Unless we drastically change the way this works, I think we're going to have to live with this issue. I suppose regular customers aren't going to change the language back and forth. And if they do so... They're going to get the updated label on the next order refresh.
If the language was really bothering the customer, I guess the customer would have switched to the correct language prior to adding items to cart.

There is no "super reduced" rate it seems, however the 5% rate (now 9%) applies to press publications. So perhaps we should do:

'rates' => [
  [
    'id' => 'standard',
    'label' => $labels['standard'],
    'percentages' => [
      ['number' => '0.20', 'start_date' => '2009-07-01', 'end_date' => '2023-12-31'],
      ['number' => '0.22', 'start_date' => '2024-01-01', 'end_date' => '2025-06-30'],
      ['number' => '0.24', 'start_date' => '2025-07-01'],
    ],
    'default' => TRUE,
  ],
  [
    'id' => 'reduced',
    'label' => $labels['reduced'],
    'percentages' => [
      ['number' => '0.09', 'start_date' => '2009-01-01', 'end_date' => '2024-12-31'],
      ['number' => '0.13', 'start_date' => '2025-01-01'],
    ],
  ],
  [
    'id' => 'super_reduced',
    'label' => $labels['super_reduced'],
    'percentages' => [
      ['number' => '0.05', 'start_date' => '2022-08-01', 'end_date' => '2024-12-31'],
      ['number' => '0.09', 'start_date' => '2025-01-01'],
    ],
  ],
],

Instead? Even though again there is not really a super reduced rate, but I think this still works. Will commit this to 3.x since 8.x-2.x is no longer maintained.

This is quite outdated, but committed this to 3.x. Commerce 2.x is no longer actively maintained. Not sure if the fix is still relevant, but it should be harmless.

Most likely a duplicate of 💬 User is sent to 403 page after successful payment (anonymous checkout) Needs work .

2.x is no longer maintained, so this won't get committed.

Actually adjusted the tweak further from the commit related to 🐛 Undefined array key moderation_state in commerce promotion view form alter Active .

Committed a slightly different patch to 3.x that should solve this. 2.x is no longer maintained.

Committed the patch from #4 to 3.x.

I guess this is a support request that aren't answered from this queue. Also this issue is 8 years old, closing.

I don't think the patch from #19 is the correct fix. This will cause the discounted price to be the new overridden unit price, so the price will keep reducing on each refresh.
What we need instead is an unprocessed_unit_price field I guess?

But I guess the question is how do we actually restore the overridden unit price on each refresh? I'd say it is the responsability of an order processor that runs early to constantly set the right unit price?

jsacksick → changed the visibility of the branch 3.x to hidden.

Instead of commenting out the part where the order email is synced, perhaps we should flag the order from the pane?
So basically, something like:
$order->setData('customer_email_overridden', TRUE);

This way we could skip the email refresh, when this data flag is set?

jsacksick → changed the visibility of the branch 3531993-support-translation-in to hidden.

Committed a different patch that does the replacement straight from the order receipt mail service. Thanks for your contribution anyway.

Since this is already tracked in Commerce email and Commerce cannot do anything about it, closing this.

No response for several months, closing this.

What is "customer_email", this key isn't defined in Commerce anywhere. This probably exists in your project, but not in Commerce.

A Rules integration isn't planned, so marking this as "won't fix".

Committed, thanks!

This is probably outdated, if the order is locked it shouldn't be refreshed and therefore the price shouldn't be refreshed.
Since this is from 2019 I'm going to close this and mark it as outdated.

I don't believe this issue is still accurate / relevant.

Attaching a static patch for use with composer.

@grevil: Could you address the phpcs & CSPELL issues? Those are pretty simple to address so this can all be green. Tom would still be the one signing off on this as I wasn't really involved, but would be great to have a green MR.

Currently, when a provider is passed, the persistent cache is bypassed. With this change, we always use the static cache if present, then fallback to the persistent cache. The provider filtering is done after the fact. I think this makes more sense than bypassing the persistent cache entirely when a $provider is passed.

jsacksick → created an issue.

Not sure I fully understand the use case, but this seems inoffensive, so merging.

jsacksick → made their first commit to this issue’s fork.

Commerce 2.x is no longer maintained, please open a merge request against 3.x.
Also I think you need to ensure the translation exists:

      if ($promotion->hasTranslation($content_langcode)) {
        $promotion = $promotion->getTranslation($content_langcode);
      }

Committed, thanks Roman!

jsacksick → created an issue.

Will tag a new release as soon as I get commit access, but marking this as a duplicate.

Marking this as a duplicate as this was fixed in dev. As soon as I get commit access and the tests are green, I'll tag a new release.

I just realized this was already addressed in dev 📌 Commerce 3.x / Drupal 11 compatibility Needs review .

jsacksick → made their first commit to this issue’s fork.

{{ order.order_number }} should work if it's configured in the view mode. Have you tried that?

We are not planning to change how profiles are assigned and owned to the orders / payment methods and then transfered to the customer addressbook. See https://www.centarro.io/blog/understanding-drupal-commerce-2x-address-book.

Ok here is what I think we should do...

If there is only one purchasable entity type referenced (commerce_product_variation, or something else), and it is known, we can probably offer the autocompletion? I don't think we have to hardcode commerce_product_variation right? Probably makes the code a bit more complex, but it should be doable?

In case there are multiple purchasable entity types possible (probably 5% of the usecases), we should offer the current UX as a fallback... Thoughts?

We are really close! Good job.
Wanted to include this one in the next release but it looks like it requires a bit more work:

1. We are currently assuming the order items can only reference product variations, breaking support for referencing other purchasable entity types.
2. I'm thinking we could update the loop in getListOfBundles() to check the purchasable entity type ID? Like the following:
   

       foreach ($order_item_types as $order_item_type) {
         // We currently only support adding variations from the admin.
         if ($access_handled->createAccess($order_item_type->id()) &&
           $order_item_type->getPurchasableEntityTypeId() === 'commerce_product_variation') {
           $bundles[] = $order_item_type->id();
         }
       }
   

   The only problem is, on new installs, the purchasable entity type isn't set... (just tried).

3. Since commerce_order doesn't depend on commerce_product, I'm suspecting the current code could break...
4. It would be great if we could implement a fallback to the current behavior in case there are order item types referencing other purchasable entity types? The order item type selector... I'm just afraid not having this fallback means you cannot add order items from the admin? Unless we accept this limitation and in this case people would need to use the IEF widget?

Also, not 100% convinced we should force people to use the new widget by writing an update hook... But maybe...?

jsacksick → created an issue.

Attaching a static patch.

jsacksick → created an issue.