[REF] Deprecate calls to createCreditNoteId

[eileenmcnaughton]

Overview

Code cleanup - createCreditNoteId is called from 3 places but only one is necessary, deprecate the other 2.

Before

Calls to createCreditNoteId are distributed & confusing

After

Only one 'active' place - in Contribution::add when the status is updated. The others are deprecated

Technical Details

createCreditNoteId is called from BAO_Contribution::add whenever the contribution
status is 'Cancelled' or 'Refunded. If was not previously called for 'ChargeBack' but it turned out it was later set so ChargeBack handling is move to be included in that call for no change in behaviour.

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

Comments

@bjendres @monishdeb it turns out we can get the createCreditNoteID to being something that could just be called from a pre hook as the other places in the code are not needed after some digging

[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).

[mattwire]

@eileenmcnaughton test fail is related

[eileenmcnaughton]

@mattwire thanks - looks like it's test setup related - will fix

[eileenmcnaughton]

It's passing now