🇮🇱Israel @jsacksick

The main reason why locked orders aren't returned / filtered out from the cart provider is we don't really want another add to cart operation to consider this cart, and allow updating the cart content while the customer is supposedly offsite... This would cause a lot of issues (mismatching order content / total etc)...

What we could potentially do is extend getCarts(), getCartIds(), getCartId() and loadCartData() to accept an extra parameter bool $exclude_locked_orders = TRUE. This could allow for example the CartController to not excluded locked orders.
At the same time, we'd need the order to be readonly there, but that doesn't really help with checkout as again in this case you don't want to allow any update to the order... This is a tricky problem.

Add a function to the PaymentGatewayBase plugin (and related interface) called supportsReusableOption() that returns FALSE by default.

Looking at the patch, I don't really understand why OffsiteRedirect overrides supportsReusableOption() and returns TRUE.

Also, not sure "option" is fully necessary for the method name, as it seems to imply "Forms API" / Fullstack Drupal though I'm not really sure how this could resurface this "setting" on a Headless setup.

Several comments:

• The "payment_methods_reusable" checkout pane setting: Not sure this setting makes sense as is as existing payment gateways wouldn't respect it from the start since gateways are currently conditionally flagging a payment method as reusable.
• I think we might just need a setting to allow a customer to opt out from saving the payment method (So perhaps only 'payment_methods_always_reusable').

Also, I have a problem with a general method / setting at the gateway level that basically indicates whether the gateway supports reusable payment methods.
The main issue I see here is a payment gateway like Braintree or Stripe for example that supports both PayPal and credit cards for example. Some payment method types might be reusable, while others might not.
Also, I see that the Stripe module defines an isReusable() method at the payment method type level, the "Affirm" payment methods aren't reusable for instance.

The current approach is too simplistic, and doesn't take into account the current state of the different contrib payment gateways, I'm really concerned we could end up with a crappy UX where the reusable checkbox is exposed when it shouldn't and vice versa.

Regarding:

We used an annotation for a very similar transition with requires_billing_information. Maybe better BC with an annotation.

We could go with "supports_reusable_payment_methods", which probably needs to default to TRUE, to take into account the existing behavior (i.e. for BC reasons).
I think similarly to "requires_billing_information", we need an associated method (similar to collectsBillingInformation(), where the default implementation reads from the annotation/ gateway attribute, but more complex use cases like the Stripe one could have conditional logic based on the payment method type.

In such case, I think supportsReusableOption() needs to either accept a $payment_method or a $payment_method_type. Passing the payment method gives more flexibility as the payment method type can be obtained like the following:

$payment_method_type = $payment_method->getType();

From the PaymentInformation pane, we could call supportsReusableOption() (or its equivalent right after calling creating the payment method via the createForCustomer() method.

So the code would look like the following:

    $payment_method = $payment_method_storage->createForCustomer(
      $payment_option->getPaymentMethodTypeId(),
      $payment_option->getPaymentGatewayId(),
      $this->order->getCustomerId(),
      $this->order->getBillingProfile()
    );

   if ($payment_method->getPaymentGateway()->getPlugin()->supportsReusableOption($payment_method)) {
   }

Additionally, rather than passing flags via the $pane_form['add_payment_method'] array, we should probably extend the PaymentGatewayForm configuration.
From:

    $inline_form = $this->inlineFormManager->createInstance('payment_gateway_form', [
      'operation' => 'add-payment-method',
    ], $payment_method);

To:

    $inline_form = $this->inlineFormManager->createInstance('payment_gateway_form', [
      'operation' => 'add-payment-method',
      'supports_reusable_payment_methods' => ...,
     // We need to expose one or 2 different settings here, one to allow opting out saving the payment method, and another flag to indicate whether the reusable checkbox should be exposed. 
      'expose_reusable_checkbox' => ...,
    ], $payment_method);

One last thing, we either need as @zaporylie mentioned a way for commerce_recurring to enforce saving a payment method as always reusable... Or... Since this is the default behavior that should be explicitly turned off, we can assume that somebody using commerce_recurring is not going to change the default behavior?

Since the same site could in theory sell digital subscriptions and physical products, it does make sense to have this configurable at the checkout pane level so the behavior can be tweaked per checkout flow.

Should we be making this feature for 3.x now?

Yes, any MR should target 3.0.x.

Support requests are generally not answered from this queue.

Furthermore, you should be using the CartProvider to get/create a cart for a given user and then the CartManager to add items to it.
You can find examples in the Drupal\Tests\commerce_cart\Kernel\CartManagerTest.

Are you able to work on this?

This needs to be rerolled due to 📌 Lazy load the cart block Active .
Also, we can't just the store default currency code, or fallback to USD.
Additionally, if one of the carts has a different currency, an exception will be thrown by Price::assertCurrency().

A D11 compatible release was tagged just now.

jsacksick → created an issue.

Merged! 🎉

OK hadn't realized the MR was opened against 2.x, will open another one against 3.0.x.

@prem suthar: Yes I know, that is exactly my point, it needs to be in the OrderReceiptMail service?

@sorabh.v6: Thank you for your work on this, committed!

I tweaked the setting description, moved the setting under "Usage limits", and manual testing works as expected... Let's see if the tests are still green but I believe this is good to go.

Wait this is not the right place for this... This should be fixed from the OrderReceiptMail service which already does token replacements for the subject. We already have the token service available there.

@jurgenhaas: I tried returning early in the ProductTrait like the following:

// Convert selected IDs into UUIDs, and store them.
    $values = $form_state->getValue($form['#parents']);
    if (!is_array($values['products'])) {
      return;
    }

That does fix the error reported here, but then the configuration isn't set... So I'm not sure what else we could/should do... The ECA Commerce module probably has to translate the string into an array when submitting the configuration form.

Even if Commerce doesn't cause a crash by assuming the "products" key is an array, you then end up with an empty configuration?

I've been trying to use the "Order contains product variation types" condition which currently doesn't work and I'm wondering about the following code:

$config = [];
      foreach ($commercePlugin->defaultConfiguration() as $key => $value) {
        if (is_array($value) && is_string($this->configuration[$key])) {
          $config[$key] = array_map('trim', explode(',', $this->configuration[$key]));
        }
      }
      $commercePlugin->setConfiguration($config);

$this->configuration already looks ok, and the result of this code will be ECA Commerce passing an empty $config array. instead of using the correctly set configuration values? Am I missing anything?

Shouldn't we initialize $config with $this->configuration? And convert the string to an array only if necessary like it is today but preserving the config is set?

I'd suggest a refactor of commerce conditions such that they keep arbitrary config values when config forms get submitted and only process them when processed.

Could you elaborate a bit more? I'm not sure I fully get the suggestion. Is that referring to your comment from #9? The reason IDS are converted to UUIDS is for exportability, and that could be quite handy in a scenario were default content is used and the UUID is preserved for example.

it should be in vendor/commerceguys/intl

How is this working without changing the services.yml? I know we have autowiring now, but the arguments are explicitly specified for the subscriber.

Merged, thank you!

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

I don't believe the fix is correct... I think the problem here is the product type that is misconfigured and doesn't have the variation types specified so the following code fails:

$variation_types = $product_type->getVariationTypeIds();
$value = $product_variation_storage->create([
  'type' => reset($variation_types),
]);

I'm not sure what the right fix is, we could default to the first variation type found, but that isn't an ideal fix. I think the problem here is the product type that is misconfigured so it will break elsewhere if the configuration isn't fixed.

hm... you're either running an outdated version of Drupal or possibly even JSONAPI Resources.

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.