Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set off_session parameter to true when creating a new subscription #764

Merged
merged 1 commit into from
Aug 30, 2019
Merged

Set off_session parameter to true when creating a new subscription #764

merged 1 commit into from
Aug 30, 2019

Conversation

marcbelletre
Copy link
Contributor

Set off_session parameter to true when creating a new subscription to tell Stripe that the recurring payments can be made off session. Without this parameter (which is false by default) all other payments will throw IncompletePayment exceptions.

This fixes issue #743.

Copy link

@SlyDave SlyDave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested against all the Stripe test cards.

Second 3DS check upon first payment of subscription doesn't occur (unless the test card enforces it to do so). This holds true for subscriptions with trial periods (when the trial ends the first payment is taken without a 3DS check unless the test card enforces it)

I also note that Stripe correctly sends out emails (if configured to do so) when the 3DS check is off-session. which it wasn't doing before.

@skollro
Copy link

skollro commented Aug 29, 2019

Can confirm this fixes #743, hopefully it is merged soon!

@taylorotwell taylorotwell merged commit 87e299b into laravel:10.0 Aug 30, 2019
@taylorotwell
Copy link
Member

@driesvints seems to make sense to me but you can review.

@azimidev
Copy link

azimidev commented Sep 3, 2019

You might want to add option 'confirm' => true as well as 'off_session' => true maybe for charge method according to this:
https://youtu.be/95qSebQrm5E?t=2393

@driesvints
Copy link
Member

But when you're performing a charge you're always on session?

@SlyDave
Copy link

SlyDave commented Sep 5, 2019

Incorrect, a automatically renewing subscription (for example) will process a charge off session. As will collecting payment details on session and processing the charge when the customer isn't there (like how much of America does card payments offline still).

Adding confirm = true is interesting, as it clearly wasn't required to make it all work, but seems like it should be to record the fact the user did authorise us to make the charge. One assumes without it stripe notes we charged off session without confirmation, as to what effect that has...

@driesvints
Copy link
Member

@SlyDave we're talking about the charge method for one-off charges, not subscriptions.

@azimidev
Copy link

azimidev commented Sep 5, 2019

@driesvints not always. with off_session we can charge the customer with $user->defaultPaymentMethod()->id. If the card needs re-authentication then it will throw an exception which from that we need to pass payment_intent->id as a parameter to

$paymentIntent = PaymentIntent::retrieve($paymentIntentId)

(hold off to payment_method->id) and use $paymentIntent response in JS again with stripe.handleCardPayment like this (such a pain)

Stripe.handleCardPayment(paymentIntent.client_secret, {
	payment_method: paymentMethodId,
})
.then(response => {
	if (response.error) {
		console.log(response.error.message);
	} else {
	    // Success!
            // Persist do whatever you want
	}
});

I mean the front end isn't a problem but it would be nice Cashier had API for

$paymentIntent = PaymentIntent::retrieve($paymentIntentId)

Kind of like $user->createSetupIntent(). we don't have this do we?

@SlyDave
Copy link

SlyDave commented Sep 5, 2019

My apologises, must have got confused with this being a pull request for subscriptions and not a new issue raised for a different concern, my bad, carry on. 😁

@driesvints
Copy link
Member

@hassanazimi you can pass this with the $options array I guess?

@azimidev
Copy link

azimidev commented Sep 5, 2019

@driesvints absolutely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants