cashier-mollie icon indicating copy to clipboard operation
cashier-mollie copied to clipboard

Mandate method check in Billable Trait

Open mmachatschek opened this issue 4 years ago • 9 comments

I"m curios for what reason the check $method = MandateMethod::getForFirstPaymentMethod($planModel->firstPaymentMethod()); of the mandate method has been implemented on the newSubscription method in the Billable trait.

Because of this check, you are always redirected to the mollie checkout in case you have set null in the config cashier.first_payment.method to let the user choose the payment method at checkout. This is mostly relevant when you add multiple subscriptions to the same billable model

public function newSubscription($subscription, $plan, $firstPaymentOptions = [])
{
    if(! empty($this->mollie_mandate_id)) {

(...)

        $planModel = app(PlanRepository::class)::findOrFail($plan);
        $method = MandateMethod::getForFirstPaymentMethod($planModel->firstPaymentMethod());

        if(
            ! empty($mandate)
            && $mandate->isValid()
            && $mandate->method === $method
        ) {
            return $this->newSubscriptionForMandateId($this->mollie_mandate_id, $subscription, $plan);
        }
    }

    return $this->newSubscriptionViaMollieCheckout($subscription, $plan, $firstPaymentOptions);
}

mmachatschek avatar May 05 '20 15:05 mmachatschek

Hi @mmachatschek,

I don't agree. The first line checks if there's a mandate id available, regardless of what method is configured on the subscription plan.

If there is a mandate id set, the subscription is created without having to go through the checkout.

sandervanhooft avatar Jun 11 '20 13:06 sandervanhooft

@sandervanhooft as there is no early return you will always be redirected to the checkout even if you have a mandate set on the billable model because in case of the problem with the payment method I mentioned you won't get into the if statement to

return $this->newSubscriptionForMandateId($this->mollie_mandate_id, $subscription, $plan);

mmachatschek avatar Jun 11 '20 13:06 mmachatschek

I'm trying to understand what you mean.

Is this issue with this line?

&& $mandate->method === $method

Would changing it to this solve your issue?

&& (is_null($method) || $mandate->method === $method)

sandervanhooft avatar Jun 11 '20 14:06 sandervanhooft

@sandervanhooft

The MandateMethod::getForFirstPaymentMethod()-method will always return a string, so the is_null check is not really helping here.

The main problem here is that PayPal is missing in the mollie-php-api MandateMethod::getForFirstPaymentMethod(). The method of the mandate can return directdebit, creditcard or paypal according to the documentation.

If you set the first payment method in the cashier config config('cashier.first_payment.method to e.g. paypal, the check $mandate->method === $method is always false because $method can only return directdebit and creditcard atm.

Also if you pass a comma separated list of possible first payment methods, the value of $method will always be directdebit. So if a customer did the checkout with a credit card, he will be redirected to the checkout again at the next subscription setup even though he already setup everything correctly. Same happens if the config of the first payment method is set to null.

mmachatschek avatar Jun 18 '20 07:06 mmachatschek

Can you suggest a fix?

sandervanhooft avatar Jul 14 '20 16:07 sandervanhooft

@sandervanhooft I added a PR at mollie/mollie-api-php for the paypal issue

mmachatschek avatar Jul 15 '20 11:07 mmachatschek

Can you suggest a fix?

I would suggest to change the line to:

&& ($planModel->firstPaymentMethod() === null || $mandate->method === $method)

This way you can either let the user decide which payment method to use by setting the first payment method of the plan to null or (if explicitly set in the plan model) strictly check if the original mandate method is the same as the $method provided.

mmachatschek avatar Jul 15 '20 12:07 mmachatschek

@mmachatschek I just used that and it works. I was having problems when I wanted to have multiple subscriptions for the same user. Nice one!

juancruzmartino avatar Jul 15 '20 14:07 juancruzmartino

Thanks @mmachatschek for the suggestion. Let's do that. Can you send in a PR?

Thanks for confirming this @juancruzmartino .

sandervanhooft avatar Jul 15 '20 14:07 sandervanhooft