Fix api Payment.create to support overpayments

[eileenmcnaughton]

Overview

Fix api Payment.create to support overpayments

Before

Calling Payment.create api to add a payment to a fully paid order results in an error

After

Payment is created. Status remains at 'Completed' and no related entities are updated. No financial items are linked to the new payment

Technical Details

We've discussed this before - it's OK to add a payment to a fully paid contribution because ... life. Arguably we could make this more obvious in the UI when it happens but it's an edge case so let's not go there on a 'this might be useful'. Adding another 'should be calculated really' status would not be helpful but it would add complexity- Completed is fine.

When this happens there should be no financial items linked to the over payment

Comments

@lcdservices @JoeMurray @kcristiano @monishdeb @mattwire @magnolia61

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@kcristiano any chance you can take a look at this or @mattwire - it's only api accessible as yet but is a necessary step towards permitting 'add payment' regardless of contribution status

[kcristiano]

@eileenmcnaughton I've done an r-run with the PR using payment.create.

I can add a new payment to an existing contribution id. If I just create the payment, the financial type is not set (not visible in UI or bookkeeping report. That can be set manually, But civicrm_financial_trxn does have an entry for the payment.

cv api FinancialTrxn.get sequential=1 id=99

{

    "is_error": 0,
    "version": 3,
    "count": 1,
    "id": 99,
    "values": [
        {
            "id": "99",
            "from_financial_account_id": "7",
            "to_financial_account_id": "6",
            "trxn_date": "2019-11-26 00:00:00",
            "total_amount": "3000.00",
            "currency": "USD",
            "is_payment": "1",
            "status_id": "1",
            "payment_instrument_id": "4"
        }
    ]
}

Is this the expected behavior? With No UI, putting the onus on those using would be acceptable. However, without the PR we get "error_message": "Contribution already completed" Are we risking having extensions add new payments without intending to since the code currently has a blocker?

[eileenmcnaughton]

@kcristiano so the earlier discussion with @lcdservices & @JoeMurray was that if a payment is received we should record it. We don't allocate it to line items - hence the lack of financial types but it has been received so it exists.

I'm not too worried about calling code relying on the exception - it's not as widely used as we would like and the fact remains that if a payment has been received it should be recorded

[kcristiano]

@eileenmcnaughton in that case this PR does exactly what it intends to. Targeting master/5.21 (I hope)

I'd prefer it in master as it's targeted and not backported to 5.20 as I am doing a good deal of testing on that and would like to see this drop separately.

[eileenmcnaughton]

@kcristiano yes it's against master

[kcristiano]

Thanks for confirm. I think this is good to merge

[eileenmcnaughton]

OK I'll merge this but @JoeMurray & @lcdservices might want to check in - I'm going off previous comments by them (will link when I find them)