🇮🇱Israel @jsacksick

Blind fix... Could you confirm the fix works?

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

Hm... Perhaps we can simply check that $payment_method->getBillingProfile() isn't NULL?

The \Drupal calls are causing phpstan errors... What Bojan was suggesting was to invoke the Calculator directly.
The only problem is that you'd need access to the currency fraction digits, and that requires loading the currency... Which when done properly requires loading the currency from the currency storage (and that again will require adding \Drupal:: calls which isn't clean...

So that lives us with this:

  public function round($mode = PHP_ROUND_HALF_UP) {
     $currency = Currency::load($price->getCurrencyCode());
     $rounded_number = Calculator::round($this->getNumber(), $currency->getFractionDigits(), $mode);
     return $rounded_number;
  }

Loading the currency like the following:
$currency = Currency::load($price->getCurrencyCode());

isn't best practice...

Won't fix it is! Thanks Bojan!

Ok, sure why not, let's add an optional scale parameter then.

jsacksick → created an issue.

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

So with this, we're no longer checking that the order can be updated, is that intended? I guess the permission should be sufficient, but still feels weird to allow unlocking an order if you cannot update the order.

johnpitcairn → credited jsacksick → .

Merged thanks!

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

This was reported and fixed in Commerce 3 already. Commerce 2.x is no longer maintained.

jsacksick → created an issue.

Allright, thank you for this!

https://www.globalvatcompliance.com/vat-rates-in-slovakia/ here it says "Second reduced", we might also need a 0% rate... But the 0% rate is missing for other countries as well.

Oh, on your article, 5% is called "super_reduced", so maybe I should mirror that?

@kazrarober: Changes look good to you? Having a 19% VAT rate called reduced feels weird lol but these are the changes that seem to make sense to me.

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

The MR should target the 3.x branch. 8.x-2.x is no longer maintained.

That's already done below and doesn't work. I think the reason is that we'd need a transaction here then to ensure the PHP object status is not different from the database status. The race conditions seems to run this function twice concurrently and the save has been called already while the second save has an outdated object...

Do you have a stacktrace to share for the exception? I'd really want to make sure we're fixing this at the right place.

I don't believe this is the right fix... Also reading at the previous comments containing the stack trace, the problematic code seems to be in the LateOrderProcessor.
Also, if this the place that needs to be fixed (from your MR), you shouldn't override the profile as you're essentially disregarding the date from the profile returned by the inline form. Instead you should make sure the profile isn't marked as new...

Definitely a won't fix here, closing as there is an issue opened in the Field Validation queue.

This sounds like a support request to me, which aren't answered from this queue. Given that this refers to a non standard field (SKU field added to products and not variations), closing this as this isn't a Commerce core bug per say.

Do you have a proposal for improving this?

Well, this is why it is called "initialNumber"? it is not disabled because this only becomes a problem once an order is placed.
This isn't really a bug, we probably should disable the field. But there is also a separate form for resetting the sequence.

I guess as a UX improvement, we can disable the initial number field.

Merged, 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.

@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 what do you think about the idea @Berdir suggested:

I'm open to any solution that could fix this. The challenge would be reproducing this in a test, to ensure the fix works...

Thank you! Merged.

I'd make this a feature request and not a bug... The solution I can think of would be to alter the form action to append the ?v query parameter.
Also, feature requests should be opened against the 3.x core version.

Have you considered using the https://www.drupal.org/project/commerce_combine_carts → module?

Why this change? throw new \PaymentGatewayException? Why did we even have phpcs:ignore for these exceptions I'm confused?

@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.

Regarding:

This means your default order type, which has shippable products, can use the Fulfillment workflow. Meanwhile, your digital goods order type can have the more simplistic Default workflow.

I believe this just describes a use case, that you don't HAVE to follow, this is just an example? For a digital workflow, you probably don't need to collect a shipping address for example?

Agreed that we could make it clearer that multiple orders are created, contributions to the documentation are more than welcome... If you find it confusing, indeed it might confuse others, so feel free to make a contribution to improve this :).

Anyway, this feature request has been opened years ago and there is nothing really actionable here. On top of that there are contrib modules that are there to alter the core behavior if needed, so closing this.
The documentation improvement can be addressed separately.

Fixed, thanks!

Ok I think we need to call the hasResponse() method first, or add a failsafe. I think we should log the response only if present, otherwise fallback to the exception message.

Something like:

if ($exception->hasResponse()) {
  ... 
}

Either that or you could load the promotion using the adjustment source ID, and then do different logic based on the promotion offer plugin.

This is no longer fresh for me, would be great if you could investigate and report back.

All promotions are applied / distributed to order items. There is no order level promotion per say. $order->getAdjustments() would return order level adjustments (so none in case of promotions). The main difference lies in the fact that the adjustment is included or not...

@trevorkjorlien: What you're describing sounds like a misconfiguration? If you want the 2 products to be on the same order, they need to share the same order type?

Again, without reproducible steps... Not much I can do, On a test install I have, with commerce 3 enabled, commerce_license & commerce_recurring, I'm not experiencing this...

hm... Probably would have make sense to coordinate this change with us prior to working on this... For such a simple feature, we now have a 123kb patch to review... and I'm not even sure this is an approach we'd actually want to pursue. Usually there is a setting that can be toggled to skip adding the destination in collection views... I'm really not sure all of this is needed TBH.

Ofc, take a look at the order entity type defined by Drupal commerce for example:

   $fields['state'] = BaseFieldDefinition::create('state')
      ->setLabel(t('State'))
      ->setDescription(t('The order state.'))
      ->setRequired(TRUE)
      ->setSetting('max_length', 255)
      ->setDisplayOptions('view', [
        'label' => 'hidden',
        'type' => 'state_transition_form',
        'settings' => [
          'require_confirmation' => TRUE,
          'use_modal' => TRUE,
        ],
        'weight' => 10,
      ])
      ->setDisplayConfigurable('form', TRUE)
      ->setDisplayConfigurable('view', TRUE)
      ->setSetting('workflow_callback', ['\Drupal\commerce_order\Entity\Order', 'getWorkflowId']);

@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?

I'm sorry but a fix like the one in the patch isn't acceptable as is...
The added logic is making too many assumptions. The contact information pane might not be used or might not be present... This can't go in as is...

But then you'd need to ajaxify the pane and save the order from there... We're doing this in shipping and getting bug reports every now and then due to race conditions... So I'd suggest you to create your own pane if you'd like to see this fixed faster.

Yeah, I got this complaint too, but this is a big change request... For this to work, we'd need conditions to return a reason, and not just a boolean... So we'd probably need to introduce a value object similar to the AvailabilityResult...

Something like ConditionResult or ConditionEvaluationResult. Because fixing this from available isn't sufficient... If the coupon cannot be applied due to an order total condition, you'd like to know that either... But this would require a ton of changes.

The email is set when the pane is submitted, so if it's placed in the same step, this isn't going to work unless the pane is ajaxified.

That is because you have a usage limit per customer set... This is a tricky problem, on one hand some merchants would like the coupon code to be allowed even if the order email is not, but at the same time they usually don't want the coupon code to be reused by the same customer... I think the solution to that problem is generating unique coupon codes that can be used only once in total.

Related issues #3017503: Allow users to explicitly require customer email for coupon usage → and #3278227: Total per customer at the coupon level is ignored for unknown customers → .

This is probably more like a feature request however, fixing this could lead to bug reports... so... not sure what to do here.

This is definitely a breaking change... Considering the problematic / conflicting constraint was added later inside the field_validation module, I think fixing it there makes more sense...

Merged, thanks :).

@anybody: Changed the text to:

The countries where the store is additionally registered to collect taxes. For further details see the Commerce Tax documentation.

Problem with the added sentence is that this only applies to the EU VAT plugin and could create confusion for other plugins...
The Commerce tax documentation needs to be expanded and the current rules should be described under the EU VAT section...

Are you ok with this?

We should probably expand the Tax documentation yes... And maybe the description should include a link to that page.

Well I'm not sure that the text added helps, because there is also the case of digital products, for which the customer zone is always used, regardless of the tax registration.

Maybe we can change: "If items are shipped into the selected countries, their respective VAT rules are applied" to If items are shipped into the selected countries, their respective tax rates are applied?

So it is up to the merchant to specify whether it is registered in other EU countries and if the 10k€ threshold is crossed.

Without the description (at all) I just thought this field is to define the tax residence of the company...

This is what the field is used for... See the following response from ChatGTP which summarizes this:

When selling physical products to customers in different EU countries, the tax rate you should apply depends on whether you are registered for VAT in those countries and whether you meet the EU VAT distance selling thresholds or the OSS (One-Stop-Shop) scheme applies. Here’s a breakdown:

1. B2C Sales (Business to Consumer)
OSS Scheme (One-Stop-Shop):
If you are using the OSS scheme and your total cross-border EU sales exceed the EU-wide threshold of €10,000 per year (or the equivalent in local currency), you must charge the VAT rate of the customer's country.
Below the €10,000 Threshold:
If your cross-border sales remain below this threshold, you can charge your store's local VAT rate unless you voluntarily opt into the OSS scheme.
Distance Selling Rules (Without OSS):
Before the OSS scheme, you needed to register for VAT in each EU country where you exceeded their national threshold. With OSS, this is no longer required.
2. B2B Sales (Business to Business)
In a B2B transaction, the VAT is usually reverse-charged to the customer's country. This means you typically do not charge VAT but must include the customer's VAT number on the invoice.
Practical Scenarios:
Using OSS: Charge the customer’s VAT rate.
Not using OSS but under €10,000 sales threshold: Charge your store’s VAT rate.
Not using OSS and over the €10,000 sales threshold: Register in the customer's country or use OSS, and charge their VAT rate.
Compliance:
Ensure proper configuration in your e-commerce system to calculate and display the correct VAT rates based on the customer's shipping address.
Maintain records of where your customers are located and their VAT details for auditing purposes.

Not sure we can append: "If items are shipped into the selected countries, their respective VAT rules are applied." Remember this screen isn't specific to VAT... There are also the Sales tax scenario etc.

This isn't a core setting, but a Commerce core alteration... Not really sure why it was added though TBH...

This isn't the right fix... We need to add a setSetting('display_description', TRUE); to the field. See commerce_field_widget_single_element_form_alter().

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

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.

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.

Would be great if we could get additional tests to demonstrate the failure... Also don't want to shoot myself in the foot here :)... As this is really an edge case.

Might be due to a custom import do a format disallowing HTML? In which case this might be due to how your format is configured... Not so much we can do from Commerce. Plenty of modules, including core define translatable strings containing HTML.

Could you provide reproducible steps? Do I need to the Content moderation module? What else is needed?

hm... Basically this is about outputting "regions", that aren't set anywhere besides custom code? Since this requires custom code to be written, why not just overridding the template in this case? I guess to just benefit from potential changes to the order template? Feels weird to inject variables that we aren't setting anywhere.

Do you have a default store set? Did you somehow accidentally delete your default store?

I recently experienced this when upgrading to D11 / Commerce 3 on a project... Unfortunately I can't remember how I fixed this... I think one of the update hooks was problematic.
I think I also tried uninstalling the page_cache module / big pipe.

I wanted to commit the fix, but the tests are not completing without it... So this line is indeed fixing something... But breaking others...

Committed!

Setting this as needs work... We already have tests coverage for attributes, and the tests are under the commerce_cart module. We don't need a new one under commerce_product.

Could you perhaps expand AddToCartMultiAttributeTest.php?

Perhaps we need to introduce a "data" array to adjustments... The problem is, serialized adjustments are already taking up a lot of space in DB... On sites with a relatively large number of orders, the order adjustments table is often the table that takes the most space... Might not be a good idea to just allow storing everything.

Also, with that said, adjustments are currently "final". Marking this as "won't fix" considering nobody expressed interest since this was first opened 4 years ago.

Also, the 150€ thereshold implemented as in the patch cannot work... as there are EU countries that aren't in the Euros zone...
I'd need to refresh my memory on this...
@anybody: FYI David Kitchen (dwkitchen) no longer works with us, I doubt you'll get a feedback from him.

@bojanz @jsacksick @rszrama: Can you tell if Drupal Commerce already implements EU OSS?

The answer to that question is no.

If this isn't about the OSS, isn't this a duplicate of ✨ Support the EU VAT Import One Stop Shop Needs review ?

Closing this as this isn't happening on a Vanilla install... Feel free to reopen in case you are able to provide reproducible steps on a clean install.

Marking this as won't fix. The add to cart message can be disabled via a setting. It doesn't make sense for Commerce core to hardcode the Ajax rule...

@anybody: I don't think this is related at all... The other issue can be addressed by using the percentage that is stored in the adjustment object.

There is already a fallback to the store address in case the order doesn't reference a billing or a shipping address... I believe this is outdated.