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

[13.x] Add pay method #1345

Merged
merged 15 commits into from
Apr 19, 2022
Merged

[13.x] Add pay method #1345

merged 15 commits into from
Apr 19, 2022

Conversation

mozex
Copy link
Contributor

@mozex mozex commented Apr 17, 2022

To use Stripe's new Payment Element we need a Payment Intent, before this PR there was just a createSetupIntent method, but now we can use the createPaymentIntent method that requires an amount parameter and an optional options parameter that according to Stripe's API Docs we need to add this option to our intent request:
'automatic_payment_methods' => [ 'enabled' => true ]
in order to activate new Payment Gateways.

@driesvints
Copy link
Member

Heya. First of all, there's no automatic_payment_methods param in your PR even though you say it's needed. Second, all payment intent invocations go into PerformsCharges, not ManagesPaymentMethods. Thirdly, I think we can refactor the charge method a bit to re-use some content for a new createPaymentIntent method like you proposed. Let me take over this PR and work on something.

@driesvints driesvints marked this pull request as draft April 18, 2022 09:04
@driesvints driesvints changed the title Add createPaymentIntent method [13.x] Add createPaymentIntent method Apr 18, 2022
@mozex
Copy link
Contributor Author

mozex commented Apr 18, 2022

Hi, automatic_payment_methods is an option that we can pass to the createPaymentIntent method, this is a working example that I am using:

$this->intent = auth()->user()->createPaymentIntent(
    $this->invoice->amount_to_pay * 100,
    [
        'automatic_payment_methods' => [
            'enabled' => true,
        ]
    ]
);

as you see if I don't pass automatic_payment_methods it only shows the card element but when I do this, every other payment element will be activated, I didn't force this option because there was another way to activate elements manually with payment_method_types param that you can specify which elements you need, that is why I didn't add it to PR.
I used ManagesPaymentMethods because createSetupIntent was there.

@driesvints
Copy link
Member

Hi, automatic_payment_methods is an option that we can pass to the createPaymentIntent method

Yeah I realize that but I think we can wrap that into a separate method. Working on this now.

I used ManagesPaymentMethods because createSetupIntent was there.

That's because setup intents is directly related to payment methods. It's only used to setup payment methods for future usage.

@mozex
Copy link
Contributor Author

mozex commented Apr 18, 2022

Awesome, I made a complete working example for Payment Elements if you had any questions just let me know.

@driesvints driesvints changed the title [13.x] Add createPaymentIntent method [13.x] Add pay method Apr 18, 2022
@driesvints
Copy link
Member

So I did some work on this and added a new pay method to the performsCharges trait. This method will instantiate a new payment intent with automatic_payment_methods enabled. This method will not perform a payment but will prepare the payment intent so it can be used on the front-end to initiate a payment element.

This method will return a Laravel\Cashier\Payment object that encapsulates a payment intent object. I've also added the ability to forward method calls on it so you can easily call the payment intent object without having to retrieve it first.

There's also a createPayment method that does the same thing but without enabling automatic_payment_methods. And I've refactored the charge method to make use of this method.

There's one concern that I have. Although it's not a breaking change to add a new pay method, I suspect maybe some apps will have this method also implemented on their billable model. That could lead to a breakage. However, I feel this method name is perfect for the thing it tries to implement. I'll leave it up to you to decide to ship it with this name or not @taylorotwell but I say let's go for it.

@driesvints driesvints marked this pull request as ready for review April 18, 2022 12:55
@mozex
Copy link
Contributor Author

mozex commented Apr 18, 2022

There was a problem in the pay method because you are forcing automatic_payment_methods if you add payment_method_types in the options it's not going to work because you can only specify one of these parameters so I just ignored it but we could throw an error instead.

@driesvints
Copy link
Member

Why are you adding payment_method_types? This isn't added by Cashier anywhere afaik.

@mozex
Copy link
Contributor Author

mozex commented Apr 18, 2022

in the docs you can see that you can either add automatic_payment_methods to options so it shows any available method or you can add payment_method_types then you can choose methods manually so obviously they are opposite of each other so if you want to force automatic_payment_methods we should do something about the payment_method_types.

@driesvints
Copy link
Member

Yeah I guess there's something to be said about payment_method_types. I deliberately made pay force the settings from the Stripe Dashboard because I feel that's the more natural take.

I just pushed a new payWith method that can act as the opposite to the pay method:

$user->payWith(2000, ['card', 'bancontact']);

@mozex
Copy link
Contributor Author

mozex commented Apr 18, 2022

Yeah, this is a good one, actually, I already am using this feature in my project so it is not that unnatural.

@mozex
Copy link
Contributor Author

mozex commented Apr 18, 2022

I think it would be good to have a retrievePayment method as well because Stripe suggests that we shouldn't create new intent for the same payment so at some point we have to save the payment intent id and then retrieve it.
I am doing exactly this in my project and retrieving it like this.

public function retrievePaymentIntent(string $id)
{
    return Cashier::stripe()->paymentIntents->retrieve($id);
}

@driesvints
Copy link
Member

@mozex added a findPayment method 👍

@taylorotwell
Copy link
Member

@driesvints would it make more sense to name these methods createPaymentIntent and findPaymentIntent ... the word pay feels like the user will be charged right then but I guess that isn't the case? What do you think? Is pay still better / cleaner?

@driesvints
Copy link
Member

@taylorotwell bit torn as I really like the clean api of the word pay. I feel like with some good documentation and a description in the docblocks it could be clear. createPaymentIntent isn't a good fit as we already have createPayment. I'm open to anything though.

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.

None yet

3 participants