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

[12.x] requiresConfirmation status helper method #1068

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

benjamindoe
Copy link
Contributor

@benjamindoe benjamindoe commented Feb 19, 2021

In some cases, users needs to confirm a payment or setup intent. This is probably due to the Stripe customer account having a card added through the Stripe dashboard (Unconfirmed, only speculating).

You can currently just do

$payment->status === \Stripe\PaymentIntent::STATUS_REQUIRES_CONFIRMATION

However, this requiresConfirmation method would bring this status inline with the other statuses

@benjamindoe benjamindoe changed the title requiresConfirmation status helper method [12.x] requiresConfirmation status helper method Feb 19, 2021
@driesvints
Copy link
Member

Can you add a method for requires_capture as well?

@driesvints driesvints marked this pull request as draft February 19, 2021 10:17
@driesvints
Copy link
Member

Would you say we need to update the public function validate() method as well to throw an exception? This could maybe have some implications on existing apps so unsure myself.

@benjamindoe
Copy link
Contributor Author

it depends, you could argue it's a bug fix as it was missing before and it's a state that the payment intent could potentially find itself in

@benjamindoe
Copy link
Contributor Author

On a side note, it's very very similar to "Requires action" and I'm actually using the same Stripe code to confirm

@driesvints
Copy link
Member

Right. We'll also need to update the payments view then if we're going to add these to the validate method. I'm going to leave this in draft for now so I can take a thorough look at all the implications soon. Sorry if this is gonna take a bit longer.

@benjamindoe
Copy link
Contributor Author

Sure no problem. I figured it might be quick as it's marked as "Optional" in the documentation

@driesvints driesvints marked this pull request as ready for review March 12, 2021 14:01
@driesvints
Copy link
Member

Hey @benjamindoe. Sorry this took longer. I'm already going to approve this PR for 12.x. I've been working on integrating the status checks for the validate method locally but I'm gonna send that in to 13.x as that could be a breaking change potentially. Thanks!

@taylorotwell taylorotwell merged commit b4197fd into laravel:12.x Mar 12, 2021
@driesvints
Copy link
Member

@benjamindoe Here's a PR that'll also implement this check on the payment page: #1094

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.

3 participants