- First commit to issue fork.
- Merge request !149Issue #2871483: Add checkout settings for payment method behavior → (Open) created by tbkot
- Status changed to Needs review
about 2 years ago 9:36pm 16 March 2023 - last update
almost 2 years ago 771 pass, 2 fail - last update
almost 2 years ago 769 pass, 4 fail Just jumping in here, patch 60 seems to be working in initial testing.
Merge request from 74seems to be missing the checkbox at checkout for some reason. @tBKot can you provide a patch and interdiff between #60?
- 🇺🇸United States loze Los Angeles
I am seeing the same thing as @tonytheferg
patch in #60 seems to work, but the MR does not show the "Save this payment method for later use" checkbox on the checkout form.
- 🇺🇸United States loze Los Angeles
Here is an interdiff of #60 and MR 149
It appears that with the MR, in order for the checkbox to appear on the checkout form, the payment gateway needs to have the public method
public function supportsReusableOption(): bool { return TRUE; }
When it does, the checkbox appears and the payment method is stored at checkout when checked.
I think this is a good addition, but gateway contrib modules will need to support this.
- last update
about 1 year ago Patch Failed to Apply - last update
about 1 year ago 794 pass - last update
about 1 year ago Patch Failed to Apply - 🇺🇸United States gcb
Here's a patch version of the PR as of comment 74, along with patches for commerce_stripe and commerce_authnet to make them compatible.
I suspect those probably belong in the other modules' issue queues, but until we are sure this is how things are being done I think it makes sense to keep it all in one place.
- last update
about 1 year ago 795 pass - 🇺🇸United States gcb
I found that the patch above caused a crash on free orders that didn't have a payment method attached when trying to produce the payment information summary pane. The attached version makes a small check to avoid that crash.
- 🇳🇴Norway zaporylie
Handling of free orders is actually not mentioned in the issue summary here but sneaked into the patch ;) Because currently we always make payment methods reusable, and here this option is squeezed under always reusable, I am creating ✨ Allow creating payment method on free orders Active and will extract the relevant part of #79 in there.
Currently not commenting on the reusability feature but will come back to comment on that soon.
- 🇳🇴Norway zaporylie
Here are some objectives that should be included in this issue's scope:
- customer must be able to opt-out from reusable payment method (::setReusable(FALSE)
). Non-reusable payment methods will only be used to create initial payment.
- gateway must be able to declare whether the payment method it creates is reusable. We must be able to filter out all unreusable payment methods. whether existing or new, depending on the context. For example, commerce_recurring must be able to enforce checkout with a reusable payment method for the sake of subsequent charges
- some gateways, depending on configuration, may be reusable but only in the Customer Initiated Transaction (CIT) scenario. This is already possible for Stripe. Such payment methods should not be reusable for Merchant Initiated Transactions (MIT). We must be able to filter them out ✨ Define payment method's reusability context ActiveI don't think we currently have one but this probably calls for a meta issue so we can divide the scope into some smaller chunks and work on them one by one,
- 🇺🇸United States loze Los Angeles
Should we be making this feature for 3.x now?
It looks like 3.0 already handles free orders with a new setting for
require_payment_method
to 'Collect payment methods on orders with zero balance' I don't think so that the automated collection of payment methods, (credit card details) without user concern is a good behaver, (GDPR compliant).
So this would be great to implement.
- 🇮🇱Israel jsacksick
Should we be making this feature for 3.x now?
Yes, any MR should target 3.0.x.
- 🇮🇱Israel jsacksick
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 tocollectsBillingInformation()
, 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 thecreateForCustomer()
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.