Deprecate calls to createCreditNoteId
authoreileen <emcnaughton@wikimedia.org>
Thu, 10 Oct 2019 10:31:49 +0000 (12:31 +0200)
committereileen <emcnaughton@wikimedia.org>
Sun, 13 Oct 2019 19:17:39 +0000 (08:17 +1300)
commitf0d18278b3e869197321f315fcc3f13177f10d63
tree07df89a1ab04f0aa98a26ef7a930ae4e71525f7b
parent4408637ca3ab242eac95bee2ad76b47248bf6746
Deprecate calls to createCreditNoteId

createCreditNoteId is called from BAO_Contribution::add whenever the contribution
status is 'Cancelled' or 'Refunded.

A few lines later recordFinancialAccounts is called, so by the time recordFinancialAccounts is called
it is already set & the empty check will prevent these lines being hit.

All 4 places where updateFinancialAccounts are called in the
code are from recordFinancialAccounts.

Record financial accounts is called from Contribution Create & Payment create.

contribution.create does not rely on these lines to set the creditnote_id, as discussed.

In the BAO_Payment there are 2 scenarios
1) payment is positive
2) payment is negative - in this case recordRefundPayment is called.

It's logically OK for the code to  NOT to create a credit note when being called from payment.create
 because payment.create is NOT cancelling the contribution or refunding it and the funtion NEVER changes
 the contribution status to Refunded, Cancelled, Chargeback (those are 'business' statuses).

Payment.create updates contribution status to reflect a payment has been made - eg change from
Pending to partially paid or various payment-related-statuses to 'Completed'.

ie Payment.create is
- only adding payments (or refunds) to an order
- and the decision as to whether something is being credited back & warrants a credit note id or
whether the payment is being returned due to overpayment or other payment related concerns does
not belong in the payment code.

Payment create never changes contribution
status to Refunded, Cancelled or ChargeBack so the lines would never be hit from payment.create

This leaves us with the conclusion the lines are never hit. For safety/sanity we can deprectate them rather
than remove them by now. That way if the above analysis is wrong a test will fail.

Less likely (given financial test coverage) the deprecation notice will show up in the wild in the next few months.

(It's rather unclear how it DOES fit in since it feels like creditnote_id only applies if the entire
contribution is being refunded & really should be generated at the point at which, for example, the event
fees associated with it are changed - however, complaints to date about creditnote_id have all focussed
on performance issues, not logic issues
CRM/Contribute/BAO/Contribution.php
CRM/Contribute/Form/Task/Invoice.php
tests/phpunit/api/v3/ContributionTest.php