🇮🇱Israel @jsacksick

The current version already works with Commerce 3?

hm... I tried installing Ludwig and it didn't crash, but I guess because my dependencies are already correctly installed via Composer.
Can you ensure the commerceguys/intl is properly downloaded and in the vendor directory?

Change looks ok to me and is not really invasive, so merged it! thanks!

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

Hi @core, I'll try reproducing the issue sometime this week, but there are some discussions in the Ludwig issue queue about retiring it, See 💬 Retire Ludwig? Active .

This likely won't get committed as the field_type_categories.yml exists, you're likely running an outdated version of Commerce.
So perhaps when we made the category change we should have bumped the minimum required Commerce version.

https://git.drupalcode.org/project/commerce/-/blob/3.0.x/commerce.field_...

Considering this breaks tests, updating the status to "Needs work".

True that there isn't one, you could detect this via code by implementing an event subscriber reacting to the payment update event.
Commerce would likely need to do the same if it were to provide one as the refundPayment is handled by the payment gateway plugin.

Your event subscribe could check that $payment->original->refunded_amount is not equal to $payment->refunded_amount. You could use the field method for that.

So something like:
if (!$payment->original->get('refunded_amount')->equals($payment->get('refunded_amount')

There are also two links above the button for Place order and Cancel Order. Place Order seems to complete the check out but why are there now two sets of buttons on here?

Could you provide screenshots? Reproducible steps? Any way you could share a backtrace. With the information that was shared doesn't allow us to be helpful here.

hm... I get the request, however this would essentially mean that the variations order is no longer respected if we change this... Currently variations can be reordered from the product variations tab, if we change this, this would essentially make the variations ordering "useless".

Perhaps we should also update the OrderSummary checkout pane default configuration to use the view, just wondering what would happen in case the view is removed from config.

jsacksick → created an issue.

Error: Class "CommerceGuys\Intl\Currency\CurrencyRepository" not found in include() (line 20 of /public/modules/commerce/modules/price/src/Repository/CurrencyRepository.php).

That seems to be it... But I don't really get why this would happen on 3.0.1 but not on 3.0.0.

It could be due to changes made around the services autowiring... Any reason to stick with Ludwig? Pretty sure you wouldn't experience this with composer.

Hm... I haven't been experiencing this... Could this be due to a contrib module? Could you turn on detailed messages logging so we can see what's in the backtrace?

Also since CartBlock::getCartViews() is unused, it can be safely removed as it is not a public method.

I prefer the second version better with the service as that allows overridding the logic easily.
We might need a change record for that change though, other than that, perhaps I should test this manually as well, I just reviewed the code which looks ok at first glance.

Can you do an MR against 3.0.x?

I don't understand? The setting is there so the description is correctly added, so no need for injecting it from the PromotionForm?

Referring to PromotionOrderProcessorTest:: testMultipleCouponRedemption().

Also, the kernel promotion test doesn't need an extra method, but I can refactor that myself prior to commit, I can also make the final changes myself prior to commit when I get some time to finally wrap this up.

hm... tests are failing but I'm not sure the failures are related.

I actually encountered this as well recently while setting up a straight redirection to checkout on add to cart, while skipping the intermediate /checkout route which doesn't need an order parameter, I believe that is the right and only possible fix.... So, let's see if the tests are passing now, just re-ran the pipeline.

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

Ok almost there, let's change the description to:
"Allow multiple coupons to apply to a single order."

Also, let's add setSetting('display_description', TRUE) to the field definition. Thanks to this no form alteration would be needed.

Also, let's update the promotion order processor test to test the case where multiple coupons are allowed using the new setting (we're missing that).

Something like the following:

namespace Drupal\my_module\EventSubscriber;

use Drupal\commerce_price\Price;
use Drupal\commerce_product\Event\ProductEvents;
use Drupal\commerce_product\Event\ProductVariationEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Provides a subscriber to the Product events.
 */
class ProductVariationSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    return [
      ProductEvents::PRODUCT_VARIATION_CREATE => 'onCreate',
    ];
  }

  /**
   * When a variation is created, set a de fault price
   *
   * @param \Drupal\commerce_product\Event\ProductVariationEvent $event
   *   The product variation event.
   */
  public function onCreate(ProductVariationEvent $event) {
    $variation = $event->getProductVariation();
    if (empty($variation->getPrice()) {
       $variation->setPrice(new Price('1', 'ARS'));
    }
  }

}

I believe this is fixed, please reopen if it is not after upgrading to the latest version.

Can you try the patch from 🐛 not correct export character Postponed: needs info ?

But why would the updates run if the fields are already there? I'm confused?

I just don't understand why the existing code in CouponTest was touched. Would have been better to add new code rather than modifying the existing logic which makes this more complicated to review.

Marking this as a duplicate as this has been requested multiple times before and there are already opened issues.
However, no plan in tackling those in a near future. Ideally a contrib module could help filling the gaps with Commerce core.

See #2869505: [meta] Marketplace model → , 🐛 Product variation access makes it impossible to restrict product management to owners Needs work and ✨ Administer store orders permission Needs work for example.

Ok decided to go ahead and merge this! Hopefully I won't regret this :).

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

A release was tagged, and the tests were fixed.

But the tests are failing, those need to be fixed, I don't really have time to look into this today.

This got committed directly to the repo after I got pinged on Slack, closing this.

jsacksick → created an issue.

Merged, thank you!

The tests are failing on with D11 on dev as well... But this seems to alter the behavior of OR facets... But I think it fixes it... I'm not 100% sure though so a bit hesitant to commit this.

Thank you, this looks like an interesting change... Wondering if this is what's causing OR facets to act weirdly as well, I seem to be getting the right facet values back with this patch, I'll open an MR to see if this broke any test.

Merged, thanks

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

Definitely not the right fix... The patch in the MR just loads all profiles... This would crash on a site with a lot of profiles, I don't believe we should do that, we should do a count query instead. Also, you could have profiles of other types too.

Committed a fix!

jsacksick → created an issue.

Thank you!

I don't think we want to solve this programatically... This to me is a configuration issue... The fallback that currently exists in the code and that is touched by the patch is in case a product type reference multiple variations types which cannot be supported by the attributes widget.

This is more like " won't fix" to me.

I kind of forgot about this change... Lately, what we've been doing for example in Stripe is each gateway provides its own payment method type plugins... So will likely mark this as "won't fix".

Closing this one in favor of 🐛 Errors in the EventSubscriber when adding to cart Active which has a fix.

Ok I now understand why the initial approach from 📌 AjaxCartEventSubscriber::$entityTypeManager declaration is redundant / problematic Active didn't work... We were both defining the service from the service.yml and overridding it, causing issues...

The attached MR should do the trick and should work with both Commerce 2 and 3.

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

Nope, this isn't what broke stuff. The Commerce change broke this... Let's follow up in the new issue that got created. I was trying to prevent this with the initial fix...

Why would this change not work with Commerce 2? The code is redeclaring a variable already defined by the parent and there is no reason for that.
This change can be safely committed.

Can you actually re-run the tests with the current code as I'm not sure the changes are causing the break.

With the changes from the MR, the code is now protected from changes made to the parent constructor.
Without the change, the current code would break due to the changes made in ✨ Provide service aliases for autowiring Active .

The code added looks wrong... The order module shouldn't have promotion related code? The coupons field is not necessarily present and is added by the promotion module for example.

Also, another possibility is to skip overridding the parent constructor all together by doing the following:

calls:
      - [setRequestStack, ['@request_stack']]

jsacksick → created an issue.

Nope, Commerce recurring doesn't work with PayPal checkout, the PayPal checkout gateway doesn't support tokenizing payment methods.

jsacksick → created an issue.

hm... This update hook looks a bit risky, isn't there a way to condition the update. There is no "name" we can filter on?

This is why updating the ProductVariation cache contexts works.

In this case I believe the right fix would be for the PriceCalculatedFormatter to add a cache contex on the user role.
It currently has:

        '#cache' => [
          'tags' => $purchasable_entity->getCacheTags(),
          'contexts' => Cache::mergeContexts($purchasable_entity->getCacheContexts(), [
            'languages:' . LanguageInterface::TYPE_INTERFACE,
            'country',
          ]),
        ],

Yes, not sure we can easily fix this... The adjustment label is set by order processors which are invoked on order refresh and the labels are set using the current site language. So basically the adjustment labels are in theory using the customer language.

I opened 📌 Stop unsetting the order ID reference from the order refresh Active a while ago... The patch is actually applied on one of the projects I'm working on but the tests are failing, so never actually committed the change...

The reason is simple, this was introduced after the fact, so calling this method by default would break if the condition plugin didn't implement it.

I think what you're looking for is the ParentEntityAwareTrait + its interface.

See https://git.drupalcode.org/project/commerce/-/blob/3.0.x/modules/promoti....

Let me know if this doesn't help.

I've been rereading this multiple times, and not 100% sure what you're asking here.
Which target entity are you trying to target? The shipment? The shipping method?

fwiw, you can also attach the condition to the shipment.

Also we might need a description explaining what the setting does. Setting to needs work for the tests.

Hi, how do I reproduce this? The method is added conditionally to the string. Never experienced this. Do you have a payment without method? Is that what is causing this?

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

You can either write your own event subscriber (Copying what is done in https://git.drupalcode.org/project/commerce/-/blob/3.0.x/modules/order/s...).

Or just use Commerce email. https://www.drupal.org/project/commerce_email →

Code looks ok at first glance, not sure why the MR was closed. Any chance we could get tests coverage for this extra setting?

Definitely, didn't pay attention to that, 3.0.x is the recommended version.

I thought you'd be able to debug this yourself :p.
We're supposed to set an empty string when the amount is empty.

$row['amount']['data'] = '';

Pretty simple fix, try the patch attached.

I'm guessing an error occurs while installing the module preventing the installation of entity bundle plugins defined by the module.

See entity_modules_installed(). You might need to run this logic yourself for the bundles to install properly... This shouldn't be needed but you can try.

Ok, this isn't really a "usage" setting, so I'm thinking we could go with:

• Field name: allow_multiple_coupons
• Method: isMultipleCouponsAllowed()

This plugin is really complex as is... Feel free to give it a shot as I personally don't have the bandwith to look into it.

Apologies... This is what I see when reviewing the MR:

As you can see, very little "context"... Merging this.

I'm confused by these changes... For example, prepareJob() currently cannot return NULL? In which case can it return a NULL job now?

Perhaps we should make this a public const (or a protected const) that can be overridden by a child backend plugin if needed.