Fix repeattransaction api to use custom data from the template contribution

[eileenmcnaughton]

Overview

This ensures (& tests) that custom data is copied from the template transaction - generally the most recent transaction

Before

Custom data taken from earliest contribution when copying for repeattransaction

After

Custom data taken from template contribution

Technical Details

The template contribution is generally the most recent one in the series but we have also a concept not-really-implemented that it could be a particular status.

Comments

This replaces #17454 which was stuck on failing tests.

Note there was talk trying to use the api in that approach. @colemanw has been working on a v4 copy api but I don't think we would want to change it without switching to v4 api. We would also want to test more field types if switching methodology

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

@artfulrobot this touches on the custom data - although it looks like some tests are failing so I will check that

[eileenmcnaughton]

@artfulrobot this touches on the custom data

[artfulrobot]

Needs more comments on what it's supposed to be testing. From where do we expect the custom data to be taken?

Looks like

1. create a contribution on a contribution recur with a custom field value "first" (aside: why is that named setupRepeatTransaction when what it means is create a contrib recur and a contribution, neither of which is a transaction?)
2. call repeattransaction and test that the new contribution received "first" for the custom datta
3. update the contribution created in (2) to "second"
4. call repeattransaction on the original contribution and test that the custom data was taken from the contribution in (2), i.e. is "second" not the original one (1)

Which is confusing to me. I'd expect that calling repeattransaction and passing in a contribution record would take details from that contribution record, not a different one.

Some comments as to what is suppsed to happen (and why) would be helpful here.

[eileenmcnaughton]

Hmm - Ok I did start from the assumption that what #17454 was trying to do was right & that I just needed to write a test & fix the implementation to get it over the line

I could make a case that the API should require the template contribution to be passed in and just use that. However, we are already calculating the template contribution in the BAO function for line items so I think it makes sense we should do it for facets. I think reversing course doesn't make sense because at least some processors only 'know' the first contribution & are already passing that in

[eileenmcnaughton]

I've updated the comment block

[artfulrobot]

• I'm OK with the idea that when there's no is_template contribution we use the last contribution when repeating a contribution from a contrib recur. Happy to believe that that's what we agreed on in that case.

• I'm confused by the idea that when we pass in a reference contribution ID we're mixing its data with the latest one though. That seems like it's only going to confuse things.

• I think the question "which contribution should be used as a template for a repeat contribution belonging to a recurring contribution" was answered with "we need a contribution that has is_template == 1 and to copy that". Is it because there's no UI for this yet that we're not implementing it?

[eileenmcnaughton]

@artfulrobot the contribution we are using is the one retrieved by the CRM_Contribute_BAO_ContributionRecur::getTemplateContribution function - which will retrieve an 'actual' template contribution if there is one, but which will get most recent otherwise.

(from Barcelona sprint)

#15456

[mattwire]

@artfulrobot In the context of this PR we should ignore "which contribution should be used as a template for a repeat contribution belonging to a recurring contribution". That is an issue but it's covered by #17455.
In this case, we just want to fix the "copy from contribution to new contribution" and assume we have the right contribution to start with!

[artfulrobot]

@eileenmcnaughton @mattwire you are both right, sorry. I got confused with original_contribution_id which I mistook for the ID of the contribution to copy.

I see that, correctly, the contrib recur is used to lookup the template (whether that's a template or the latest).

[artfulrobot]

As tests serve to document, more comments explaining what's being tested would be helpful to other readers. I'm happy to have a go at adding these in in a later PR.

[mattwire]

@eileenmcnaughton Thanks for picking this up. There is a nuance that I may not have explained very well on #17454 and this PR does not resolve - namely that the custom data should be copied at the same time as the contribution entity is created otherwise it's impossible to override in a sensible way because the custom data does not trigger a hook and does not exist when the "create" hook is called for the contribution. The API3 solution on #17454 does this and I was hoping that the API4 copy solution could be an even better way.

I know that @KarinG has an interest in overridding custom data copy on repeattransaction. I originally came across this with the giftaid extension because we specifically don't want to copy giftaid custom fields.

[eileenmcnaughton]

@mattwire I did notice a reference to that - but I think it needs a much more comprehensive set of tests than I was interested in doing. It's possible the apiv4 method might need less tests (if they are well baked into the apiv4) but we shouldn't be going down the apiv3 path on this at this point

[KarinG]

@mattwire - we're not doing anything specific re: repeattransaction but for a number of projects - we've got some bits like this:

function revenuesharing_civicrm_post( $op, $objectName, $objectId, &$objectRef ) {

  // don't autofill batch number, feetype__itemnumber, deposit date on recurring contribs
  if ($objectName == 'Contribution' && $op == 'create') {
    if (isset($objectRef->contribution_recur_id)) {
      // set the values.
      $params = array(
        'id' => $objectRef->id,
        'custom_664' => '',
        'custom_293' => '',
        'custom_294' => '',
      );
      civicrm_api3('contribution', 'create', $params);
    }
  }

[colemanw]

Ok. I think this is ready to merge as it's well tested and fixes an existing problem. I understand that there are additional improvements which would come with switching to APIv4 but that isn't quite ready yet so that should be addressed in a follow-up PR.

[KarinG]

👍 Thanks @eileenmcnaughton