woocommerce / woocommerce-gateway-stripe

The official Stripe Payment Gateway for WooCommerce
https://wordpress.org/plugins/woocommerce-gateway-stripe/
231 stars 202 forks source link

Card ID Source reverting to old ID after Stripe update with WooCommerce Subscriptions #476

Closed poolet08 closed 6 years ago

poolet08 commented 6 years ago

Affected ticket(s)

What I expected

What happened instead

We recently updated to the latest version of Stripe with WooCommerce & Subscriptions and we noticed an uptick in failing subscriptions. I took a look further and it appears that for whatever reason some customers card ID source linked to their subscription reverted back to an old card ID Source thus causing their subscription to fail upon renewal even though they have an active card on file and renewed successfully in December.

Steps to reproduce the issue


poolet08 commented 6 years ago

I thought jrick1229 mentioned they were seeing the error on a lot of tickets?

roykho commented 6 years ago

Not reported to us (me) and their issue is probably same as yours where customer is changing their info on the non subs page.

poolet08 commented 6 years ago

I don't think the problem is because one is not found, I think its going back and charging an old one for some reason.

roykho commented 6 years ago

Right thats what I mean. When one is not found, then it goes to find the original that was used and try that. If that is also not found OR it has expired then it fails. Where in the past, if one is not found, it tries to use one (default) that is on customer's file in Stripe wether or not customer agree to be charged on that card for this particular subscription.

Because a customer can potentially have multiple payment sources on your site and imagine they have one card to buy non subscription stuff on your site and one that they specifically use for subscriptions. But now, imagine the one linked to the subs has failed/expired -- question now is would the customer be OK with you going ahead and charging their other card instead?

poolet08 commented 6 years ago

I think if there is a card not found or if it has expired, it should always go to charge the default card on file. That is what the majority of customers expect. I have rarely out of thousands of subscriptions monthly had someone complain that we charged the wrong card on file.

Maybe have an option on the payment menthods page to check which card you want to be used for Subscriptions? There just needs to be a simple way to designate on the payment methods page which card can be used for the subscriptions. If they need to use different cards for different subscriptions, thats a rarity in and of itself and could be updated on the individual subscription page I suppose.

roykho commented 6 years ago

What you're suggesting is pretty much what I have proposed for it to be added in subs. Subs is responsible for knowing which payment should be linked to which subs order. So it makes sense for this kind of logic to go in there.

poolet08 commented 6 years ago

Got it. Has there been any discussion with them on this issue?

poolet08 commented 6 years ago

Here was the latest discussion I had with Nick from Prospress back in December:

Hi Travis,

I'm copy/pasting below the discussion that I've brought up internally. I'm going to close this out for now, since there is no immediate fix to be done, but I can certainly let you know what comes of the discussion after some time. Let me know if there's anything else we can do at the moment.

Thanks, Nick


Synopsis This issue is to discuss the general confusion that still exists around updating payment methods in the right place. There still exists a weak point in the UX. I believe this should be considered a feature, but since it's not something that will end up in the suggestions list (it's a subtler point of the UI), GH appears to be the best place to discuss/record this.

Example Customer's card expires Customer logs into 'My Account' and clicks on 'Payment Methods' Customer clicks on 'add payment method' and then 'make default'

Customer thinks they've properly updated their card for their subscription(s), but they actually haven't. The problem is that the old card is still in use on their subscription, and so will fail in the future. Source of confusion

At no point in that process is there any indication of how adding a payment method or setting it to default will affect active subscriptions. For the average punter, 'make default' implies future renewals, not just future new subscription purchases. The difference is obvious to us, but not users.

Possible Solutions Add 'update active subscriptions' checkbox to 'add payment method' form Add language when 'make default' is clicked to indicate that active subscriptions still need to be changed Add 'update active subscriptions' dialog to 'make default' link

thenbrent commented 6 years ago

So what would be better is a way for Subscription to check if they're on the WC add payment page, to know which card/source is tied to the subscription and if they want to edit/change that, it would link them to the "change payment method" of the Subscription. That way a supposedly valid card will always be linked.

Subscriptions already checks when a payment method is being deleted from the My Account > Payment Methods page to see if it's linked to a subscription (as best it can match it up), docs on that here: https://docs.woocommerce.com/document/subscriptions/customers-view/#section-18

But "link them to the "change payment method" of the Subscription" is not a feasible customer flow.

One payment method can be linked to multiple subscriptions, but the Change Payment Method flow on a subscription is tied to one subscription. Furthermore, that flow would only change the payment method on the subscription, not with the store. Which negates the purpose of the My Account > Payment Methods page meaning if we implemented this suggestion, Subscriptions would prevent a customer ever being able to add a payment method with a store. That's not a suitable outcome.

Subs is responsible for knowing which payment should be linked to which subs order.

That is not exactly true. There is no API that allows Subscriptions to know about how to process a payment with a specific gateway. All payment logic is handled by the payment gateway extension.

Subscriptions has a very limited API to allow the payment gateway to validate tokens entered via the admin, but it doesn't know much more than that API provides, which is really just:

Prior to 4.0, it would go and look for a default card to charge from the customer. This feature was later than removed in 4.0

@roykho I understand the rationale for that change, but that's a major breaking change to just slip in without any discussion or announcement.

It has the potential to cause thousands of recurring payments to fail.

I'm sure many stores would have appreciated some communication about that behaviour being removed, ideally publicly but also at least with the Prospress team directly (or even just a mention in the changelog.txt).

roykho commented 6 years ago

@thenbrent No it was not intentional breaking change. It was not aware to us that people are deleting/updating the card from non subs payment change page expecting that it would link to the subscriptions in question.

That is not exactly true. There is no API that allows Subscriptions to know about how to process a payment with a specific gateway. All payment logic is handled by the payment gateway extension.

That is not what I meant. I meant for subs to direct customer to the subs payment method page when it knows a card is linked to a subs order. No API is needed to do this.

thenbrent commented 6 years ago

It was not aware to us that people are deleting/updating the card from non subs payment change page expecting that it would link to the subscriptions in question

That's not the only path that would lead to this issue either.

To manually setup automatic payments, a store owner could previously set just a customer token against a subscription, leaving the card field empty becuase that was acceptable with the validation applied prior to 4.0, as seen here: https://github.com/woocommerce/woocommerce-gateway-stripe/blob/3.2.3/includes/class-wc-gateway-stripe-addons.php#L364

(The card/source field is now required in 4.0+).

I meant for subs to direct customer to the subs payment method page when it knows a card is linked to a subs order.

OK understood. Unfortunately, that's not a suitable flow for the reasons outlined previously.

Instead, about the only options we have are:

  1. never allow tokens against a subscription to be modified from that page
  2. allow those tokens to be modified, but also loop the customer through the subscription Change Payment Method page anytime a token is removed and require them to update the token against all subscriptions which previously used that (pretty awful UX).

There is a simpler alternative that will still account for the reasons it was removed, but continue to allow stores to enjoy a more customer friendly behaviour.

After finding the $prepared_source for a subscription payment (here), if the $prepared_source->source is empty, the extension could fallback to the customer's default method, but only if a boolean flag which defaults to false, but which can be filtered to return true, using a filter like 'wc_stripe_subscription_use_customer_default_source'.

Something like the code below added after here:

if ( empty( $prepared_source->source && apply_filters( 'wc_stripe_subscription_use_customer_default_source', false, $prepared_source, $order ) ) {
    $stripe_customer = new WC_Stripe_Customer( $customer_id );
    $prepared_source->source = $stripe_customer->get_default_source();
}
roykho commented 6 years ago

@thenbrent yes that is exactly what I have done here https://github.com/woocommerce/woocommerce-gateway-stripe/commit/3498d0320a6f963e66c40934f93d466f7f32afab

Minus the filter.

roykho commented 6 years ago

So if that works for people, I would rather the filter be default true and option turn off if store owners are getting complaints.

thenbrent commented 6 years ago

@roykho awesome. Looks and sounds good to me. 👍

roykho commented 6 years ago

@thenbrent not quite, upon further research this only partially solves the issue. This only solves the issue if the source is empty. There is still a case where if the customer goes to payment method page, added a new card, then deleted the old card that was linked to the subs. Yes there is a warning that shows but imagine they ignored it thinking the new card they added will work. The fix for that will not work as the stored source ID is actually still found so would never trigger the condition. So it seems we need additional logic perhaps when payment fails to renew, then go and try to grab the default source.

thenbrent commented 6 years ago

The fix for that will not work as the stored source ID is actually still found so would never trigger the condition

Ah right, because it's still stored on the order/subscription's post meta. Good catch.

when payment fails to renew, then go and try to grab the default source

That'll work. It should only handle the case where payment fails to renew because of an attempt to use a source that has since been removed. Hopefully the Stripe response codes will make that clear.

There's many other cases where the renewal payment can fail but it should not be reattempted using a different card, like insufficient balance etc. (those cases can be handled by the retry system).

poolet08 commented 6 years ago

Hey Roy,

Thanks for this! When do you think a release will be out?

poolet08 commented 6 years ago

While we are on this topic, I have dealt with two customers who have set their default card and think that it is going to be the default card charged. However, the previous card remains in Stripe and still gets billed. Not sure if anything can be done about that but worth looking into.

Thanks

graphiclux commented 6 years ago

I am having the exact same problem as poolet08. It has been happening since the last Stripe plugin release. Multiple accounts with defaulted cards that work are failing. I also tried the test-sub version and that did not correct the issue.

graphiclux commented 6 years ago

@poolet08 have you found a fix for this issue yet?

poolet08 commented 6 years ago

I believe they have fixed it in the upcoming release but nothing has been mentioned about a release date yet.

roykho commented 6 years ago

It was released today in 4.0.4

poolet08 commented 6 years ago

Thanks Roy!

graphiclux commented 6 years ago

Confirming this is fixed now! Thank you.