Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

When order total is less than 0 no payment is made by mollie. So no need to… #183

Merged
merged 2 commits into from
Apr 2, 2020

Conversation

stefro
Copy link

@stefro stefro commented Apr 2, 2020

When order total is less than 0 no payment is made by Mollie. So no need to check for active mandate.

We're using Cashier in a project where we currently support businesses to sell digital gift cards to support them during the Corona lock-down. These customers have a free account without a mandate (no need, since we'll pay them instead of they pay us).

The cashier package is still really useful l because of calculating transaction costs, invoicing etc. We just payout the "credit" amount that is calculated by cashier. It works really well for this.

However, this particular type of client does not have a mandate and we're not going to ask them to create one. This PR checks if the total order amount is 0 or less and if so, it won't throw an error when no mandate is set.

This will allow our particular client type and won't introduce breaking changes. I really hope you're accepting this PR!

@sandervanhooft
Copy link
Contributor

I'm ok with it, just add some decent tests to OrderTest.

You probably will have problems running the rest of the OrderTest methods without the required secrets for Mollie. Just focus on your specific methods, I expect yours will not be interacting with Mollie.

PS. Sounds like a great initiative. 👍

@stefro
Copy link
Author

stefro commented Apr 2, 2020

Thanks @sandervanhooft
And yes, the reason I didn't create any tests is that I couldn't figure out why I can't get them to pass (without my changes). I tried to paste in my mollie test key but even then I couldn't get all of them to pass.

I'll give it another shot to test my changes. If you can give me any pointers on how I can make all tests pass I'd appreciate it. Also for maybe some future PR's. I love to see all tests green before I commit :-)

@sandervanhooft
Copy link
Contributor

There is a test instruction at the end of the readme, but it seems two secrets are missing:

MANDATE_PAYMENT_PAID_ID
SUBSCRIPTION_MANDATE_PAYMENT_PAID_ID

I'll look into drafting a test manual with some more details.

@stefro
Copy link
Author

stefro commented Apr 2, 2020

Thanks @sandervanhooft!
I think I get it now, the key names are very descriptive. I'll create all the mandates, payments etc and will then enter the values in phpunit.xml.

I'll try to create the tests this evening or tomorrow morning. Thanks again for your time!

@sandervanhooft
Copy link
Contributor

LGTM!

@sandervanhooft sandervanhooft merged commit 6eb858b into laravel:develop Apr 2, 2020
@sandervanhooft
Copy link
Contributor

@stefro I'll try to release this tonight so you don't have to wait until next week.

Will be released as a patch (1.12.x).

@stefro
Copy link
Author

stefro commented Apr 2, 2020

Thanks for merging!

@sandervanhooft
Copy link
Contributor

No problem. And it's also shaving off a few calls to the Mollie API. Everybody wins 😄 .

@sandervanhooft
Copy link
Contributor

@Stefo Released in 1.12.2 . (More info)

@sandervanhooft sandervanhooft mentioned this pull request May 11, 2020
4 tasks
@lexdewilligen
Copy link
Contributor

@sandervanhooft @stefro I believe there's still an issue with this solution. Sure it does solve that a mandate guard is not needed when the total order amount is negative. But how about orders that are being paid by credit, they also don't need to guard for a mandate, right?

Currently the mandate check happens before the processing of the billable's credit:

        $minimumPaymentAmount = $this->ensureValidMandateAndMinimumPaymentAmountWhenTotalPositive();
        $this->update(['mollie_payment_id' => 'temp_'.Str::uuid()]);

        DB::transaction(function () use ($minimumPaymentAmount) {
            $owner = $this->owner;

            // Process user balance, if any
            if($this->getTotal()->getAmount() > 0 && $owner->hasCredit($this->currency)) {
                $total = $this->getTotal();
                $this->balance_before = $owner->credit($this->currency)->value;

                $creditUsed = $owner->maxOutCredit($total);
                $this->credit_used = (int) $creditUsed->getAmount();
                $this->total_due = $total->subtract($creditUsed)->getAmount();
            }

            $totalDue = money($this->total_due, $this->currency);

        ......

Shouldn't we move the $minimumPaymentAmount = ... inside of the transaction, after $this->total_due has been calculated and base it upon this total_due?

@stefro
Copy link
Author

stefro commented Aug 28, 2020

@lexdewilligen Yes, that makes sense to me!

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

Successfully merging this pull request may close these issues.

3 participants