Potential trouble with create#InvoicePayment when the amount passed is 0

Technical Discussion
1

I ran into an issue (causing us to overpay an ACH split direct deposit).

When mantle.account.PaymentServices.create#InvoicePayment is called with a “false” amount (be it zero/null, etc.)

It assumes the desire is to create a payment for the the unpaid invoice amount. It specifically calls mantle.account.InvoiceServices.get#InvoiceTotal to acquire the unpaid invoice amount.

This amount is totaled using the payment applications (mantle.account.payment.PaymentApplication) matching invoiceId or toInvoiceId.

That is fine and expected behavior.

But, if forInvoiceId is set on the payment, no PaymentApplication is generated, as it will generate that later when the payment is marked Delivered or Confirmed (i.e. it is delayed).

When doing a split direct deposit, we had a customer define a rate and flat amount of 0 on their secondary bank account. When payments were being produced it allocated the full first amount, as it should have done. On the second payment, it allocated the full invoice amount (not what would end up being the unpaid invoice amount). These payments were direct deposited to the employee even though there is a balance left on the payment. I take responsibility for allowing that to get through.

We could adjust get#InvoiceTotal to further inspect payments with a forInvoiceId, but just because they are earmarked for that doesn’t mean they will have to be applied to that invoice…so that seems poor.

In the end, we left mantle’s implementation alone, changed the edit screen to disallow creating a $0 payment, changed the split direct deposit allocation to skip $0 payments, and for good measure now inquire about the amount create#InvoicePayment creates a payment for to make sure it doesn’t happen again (fairly overkill, but clawing back money from employees who like the bank error in their favor is sometimes more difficult than it would seem).

It is plausible mantle’s could be changed instead to instead check against null…and reject $0 payments, but I have a suspicion existing logic may at least accidentally pass a 0 in, and which case it seems safest to leave that alone.

Anyway, this is more of a “heads up”, passing $0 to create#InvoicePayment could conceivably cause you grief if you make multiple payments towards the same invoice while any of the payments have not been applied to the invoice.

1 Like
2

Thanks for your post.

This sounds like a pretty vital patch to anyone who’s using the split payments. This is a good candidate for a PR.

3

Perhaps, though I’m not sure either should change. Here’s my thinking on them anyway:

mantle.account.PaymentServices.create#InvoicePayment:

While I didn’t expect it, its behavior should remain. A case could be made that the test for !amount be changed to only apply to nulls and a no-op or error for a 0 amount. This would be a possibly breaking change for those relying on it just to pay off the invoice.

mantle.account.InvoiceServices.get#InvoiceTotal:

This would be a candidate for a change in how it determines the unpaidTotal out parameter.

It is currently calculated as follows:

            <!-- PaymentApplication by invoiceId (expect either paymentId to toInvoiceId to be populated) -->
            <!-- PaymentApplication by toInvoiceId (expect invoiceId to be populated) -->
            <entity-find entity-name="mantle.account.payment.PaymentApplication" list="paymentApplicationList">
                <econditions combine="or">
                    <econdition field-name="invoiceId"/>
                    <econdition field-name="toInvoiceId" from="invoiceId"/>
                </econditions>
            </entity-find>
            <set field="appliedPaymentsTotal" from="0.0"/>
            <iterate list="paymentApplicationList" entry="paymentApplication">
                <set field="appliedPaymentsTotal" from="appliedPaymentsTotal + (paymentApplication.amountApplied ?: 0.0)"/></iterate>

            <set field="unpaidTotal" from="invoiceTotal - appliedPaymentsTotal"/>

An additional query could be added for payments with:
*) A forInvoiceId that matches the invoiceId
*) Are NOT in a status of payment delivered or confirmed
*) Have remaining unapplied amounts

I’m not sure the earmark of forInvoiceId is a reliable enough indicator that other payments “will be” applied to this invoice, but it is likely a fairly safe change if it seems like a good idea to others.

Our solution was to skip invoice payment creation altogether if the payment was 0…which was really the best solution all along, as the side effect of it attempting to pay the balance of the invoice was not a desired behavior.

1 Like
4

It sounds like a good idea, but I’m not familiar enough with the use case and the code to really have a strong opinion on it.

Thank you @sbessire for sharing this