Respect calling code passing in 'null' for creditnote_id.

[eileenmcnaughton]

Overview

Fixes an issue where the contribution bao disregards 'null' when passed in for 'credit_note_id'. This has performance implications as the code to get the credit note id is very slow and there is no precedent anywhere in our code for the BAO to deliberately override efforts by
calling code to null out a value

Before

The 'null' in the below query is ignored and the credit note id is calcluated anyway in a way that performs very badly, even on sites without the requirement for this particular calculation.

civicrm_api3('Contribution', 'create', [
  'id' => 'x',
 'contribution_status_id' => 'Refunded',
 'creditnote_id' => 'null'
);

After

The api call results in a credit note id of NULL and performance is not damaged.

civicrm_api3('Contribution', 'create', [
  'id' => 'x',
 'contribution_status_id' => 'Refunded',
 'creditnote_id' => 'null'
);

Technical Details

The credit note code was implemented in core quite a while ago, before we were really pushing these things
out to extensions. It was implemented such that it iterates through all credit notes & has performance
implications in order to meet a use case that is not common to all.

Ideally we would move it to an extension, but short of that we can at least respect
calling code setting the value to 'null' in order to override it.

Probably we should longer term simply have a hook

hook::getCreditNoteID() & more this function to an extension called by that hook

Comments

@JoeMurray @pfigel @jamienovick @lucianov88 it seems Patrick's fix mitigated the performance issue but it's still a thing - too much so for us to be able to have that code in production with hacking it out. This makes the credit note behaviour 'opt out' & makes our code more consistent - although per above - it should still be moved to an extension IMHO

[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]

test this please

[pfigel]

@eileenmcnaughton there's two more places in the BAO where "null" is checked before createCreditNoteId() is called. At least one gets hit by the API during cancellation (Contribution.create with status changing to "Cancelled").

civicrm-core/CRM/Contribute/BAO/Contribution.php

Line 1128 in 84275e2

if (empty($params['contribution']->creditnote_id) || $params['contribution']->creditnote_id == "null") {

civicrm-core/CRM/Contribute/BAO/Contribution.php

Line 1143 in 84275e2

if (is_null($params['contribution']->creditnote_id) || $params['contribution']->creditnote_id == "null") {

[pfigel]

@eileenmcnaughton I put together a simple extension that uses this to disable credit note generation. It has a test that can be used to verify this implementation works.

[eileenmcnaughton]

@pfigel thanks - I've updated it - is it OK to merge on pass now.

@ejegg @bjendres @sluc23 we can start using Patrick's extension above to resolve this performance issue in a less hack-core way from 5.20

@jamienovick this shouldn't affect much since it will still set creditnote_id using the slow-but-useful-for-UK-sites unless a developer intervenes to stop it from doing so. Longer term we have been discussing that an extension should opt in rather than out of slow-but-useful-for-UK functionality & @bjendres will log a gitlab issue to discuss further (don't worry nothing more than this will happen in the short-term)

[bjendres]

See lab/Core #1308

[pfigel]

Couldn't find any issues during r-run, MOP.

[mattwire]

Merging based on comments and tests passed