Get the template contribution instead of the first contribution when using repeattransaction

[mattwire]

Overview

When calling repeattransaction we should be preferring the template contribution (and using the function in CRM_Contribute_BAO_ContributionRecur to get the correct template contribution).
Currently we're using the first contribution.

Logic should be: template contribution ?? latest contribution.

Before

Contribution.repeattransaction uses first contribution as template.

After

Contribution.repeattransaction uses standard method to get template contribution.

Technical Details

Switch to standard function.

Comments

@eileenmcnaughton @monishdeb @artfulrobot This came out of #17032 and causes issues such as contribution source text not being what you expect it to be, lineitems matching first contribution instead of latest etc.

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

Can we get a test on this?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

New zero warnings highscore: no warnings for 2,003 days!

  | Test Result (30 failures / +30)api_v3_ContributionTest.testRepeatTransactionMembershipRenewContributionNotCompleted with data set #1api_v3_ContributionTest.testRepeatTransactionMembershipRenewContributionNotCompleted with data set #3api_v3_ContributionTest.testRepeatTransactionMembershipRenewContributionNotCompleted with data set #5api_v3_ContributionTest.testRepeatTransactionMembershipRenewContributionNotCompleted with data set #8api_v3_ContributionTest.testRepeatTransactionMembershipRenewContributionNotCompleted with data set #10api_v3_ContributionTest.testRepeatTransactionUpdateNextSchedContributionDate with data set "receive_date_includes_time_with_installments"api_v3_ContributionTest.testRepeatTransactionUpdateNextSchedContributionDate with data set "receive_date_includes_time_no_installments"api_v3_ContributionTest.testRepeatTransactionapi_v3_ContributionTest.testRepeatTransactionLineItemsapi_v3_ContributionTest.testRepeatTransactionIsTestShow all failed tests >>>

[eileenmcnaughton]

This is failing tests & has been for a while - but I salvaged the test & put up a passing patch #17972

[mattwire]

@eileenmcnaughton I'm going to re-open this. I think we need to work through these test failures as we're passing way too many "contribution" objects around and taking bits from here and there. This PR tries to resolve that early on and make sure we are working with a single, specific contribution.

It also "enforces" the following logic for parameters to Contribution.repeattransaction:

• If contribution_recur_id is set, find the template contribution based on that and ignore any contribution ID that is passed in.
• Otherwise use contribution_id as passed.

The rationale behind this is:

• Sometimes you only have one or the other of those parameters. If you really require that a specific contribution is used when you have a contribution_recur_id then just pre-process and remove the contribution_recur_id and specify the contribution_id that you desire. I've come across various situations where both contribution_id and contribution_recur_id are being passed and was confused as to which would take precedence. The current situation is a mix of "both" and "neither" which is pretty inconsistent.
• If you pass both, how can you be sure which one the code will use? It makes no sense to use both.
• Regarding renewal of related entities, that should get calculated either via the recurring contribution or via the existing membership_payment record on the contribution so should work in both cases.

[eileenmcnaughton]

@mattwire if you want to actively work on the test fails it's fine for it to be open and I'll close the ones I re-opened.

However, it's not really the way that I imagine tackling cleaning up this function - which is really to work towards repeatransaction only calling repeatransaction, and not the full completeOrder function - which was the approach I used (ie save the contribution only once, in repeattransaction and not again in the main function). Note that the repeattransaction already takes the line items from the templatecontribution - and that's locked in by tests - so I wouldn't want to change that without a gitlab analysis. Also note that it's not clear to me that we gain much by passing in $contribution to repeattransaction at all.

I had thought the way to proceed was to look at what happens after repeattransaction is called, ensure it's tested & simplify / remove it. I guess in short you want to clean up how we build some variables that I think we should look to eliminate the usage of

However, as with all WIP it should only be open when you are actively working on it since this is the review queue.

[eileenmcnaughton]

@mattwire note that the only time I think 'contribution id as passed' should be used is when it comes in the $input array. Currently we always set original_contribution_id in the $input when it comes from the api

That $input array is passed to the repeattransaction command - so the repeattransaction command could pass that into getTemplateContribution as one of the overrides & that could take precedence over the order already in there.

That leaves the problem that the api is calculating the 'original_contribution_id' when we don't pass it into the api - and a later PR could remove that - but for now it is coming up with the effective same result so it doesn't matter whether it follows sooner or later

[mattwire]

@eileenmcnaughton I see what you mean about original_contribution_id being calculated in the API. I've just pushed another commit that should fix at least some test fails.

[eileenmcnaughton]

hmm - it's worse

[eileenmcnaughton]

I still don't think we should be calculating templateContribution in 2 places - we have everything we need in the repeattransaction function

[eileenmcnaughton]

I removed the needs-work flag & put more specific flags on.

[mattwire]

Closing pending work on #17994