Account created on 8 October 2010, about 14 years ago
#

Merge Requests

More

Recent comments

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

Do you have a proposal for improving this?

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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()) {
  ... 
}
🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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']);
🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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?

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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().

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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?

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

The answer to that question is no.

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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

🇮🇱Israel jsacksick

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.

🇮🇱Israel jsacksick

hm... this event is reallllly specific... I'd be more in favor of introducing a setting for that (maybe at the gateway level?) Problem is, this method doesn't have access to the gateway... Could be just a setting that we fetch via Settings::get(), we'd just need to document it, but similar to how we have an event that you'd need to know about...

🇮🇱Israel jsacksick

Do you define a custom checkout pane extending the Login pane? This isn't happening for me on a default install, so I'm assuming custom code is triggering this, therefore lowering the issue priority.

🇮🇱Israel jsacksick

After more than 3 years, we can safely assume this ain't going to happen :).

🇮🇱Israel jsacksick

This code was added as part of #3137636: commerce_payment.order_updater should sync order payment method from payment transaction . Not sure how we can improve it...But this logic is needed for the usecase mentioned in the referenced issue.

Production build 0.71.5 2024