dev/core#4000 Fix (old) regression - ensure only template contribution updates update recur

[eileenmcnaughton]

Overview

This revives #25023 which stalled

To address the stalling of the last PR I have

• Fixed test issues that were unrelated but made that un-mergeable (already merged, not in this PR)
• Documented how recurrings, template contributions & repeattransation work - https://docs.civicrm.org/dev/en/latest/financial/recurring-contributions/
• Created an extension for those who want the behaviour being removed https://docs.civicrm.org/dev/en/latest/financial/recurring-contributions
• Added an upgrade message pointing to the above
  

The issue is that in 5.49 / #21473 a change was added to update ContributionRecur records to match the amount and currency of any contributions attached to them if those contribution amounts or currencies were edited. This is expected behaviour for TemplateContributions as any change to a template contribution represents a change in expected future behaviour (and possibilty should be extended to create for template contributions).

However, for other contributions we do not know if this is a one-off change or a change in expectations. If the former the change should be ring-fenced to the Contribution in question. In the latter the ContributionRecur should be updated. The post hook is not in position to know which of these is the case so the responsibility of determining whether to change the ContributionRecur should be on the code that edits the contribution.

Users are not able to edit contributions linked to RecurringContributions under normal core usage and core recurring PaymentProcessors do their own updating - so this scenario never happens without custom code in play. So the issue is we have a behaviour where a change initiated by custom code triggers another change that often does not make sense, and which the custom code could make for itself.

@mattwire suggested that rather than limit the code to only make a change if the TemplateContribution changes we also include pseudo-TemplateContributions - ie the latest contribution if no TemplateContribution exists. However, this is not always correct as payment processors and sites support a range of behaviours such as ad hoc payments of variable amounts against a RecurringContribution.

In order to preserve the core-for-a-bit behaviour for those who do want it I have put up an extension that people can install if they do https://docs.civicrm.org/dev/en/latest/financial/recurring-contributions (I was unable to explain when someone would want it but there was resisitance to just removing the behaviour when I last worked on this.)

I have documented how recurrings and template contributions work here

https://docs.civicrm.org/dev/en/latest/financial/recurring-contributions/

Before

This is pretty hard to replicate in standard core because with no extensions installed users cannot edit contributions attached to payment processors. It is more likely in code - e.g if a payment processor created a one-off pending contribution against the record & then becomes aware the amount actually processed differs for some reason. It is an edge case but what we know about payment processors is that they are full of edge cases & hence we should not assume that if the code for some reason adds & then edits a contribution linked to a recurring contribution what it REALLY means is that the recurring record should change.

So to replicate on master, create a recurring payment & then add a contribution to it & update it using the UI

CRM.api4('Contribution', 'create', {
  values: {"financial_type_id":1, "contribution_recur_id":1, "total_amount":90, "contact_id":203},
  chain: {"name_me_0":["Contribution", "update", {"values":{"total_amount":777, "id":"$id"}}]}
})

After

In the above scenario there would be no change UNLESS we are dealing with a TemplateContribution

Technical Details

Comments

[civibot]

No issue was found matching the number given in the pull request title. Please check the issue number.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

Is the recur updating when any linked contribution is updated or just the latest?

[eileenmcnaughton]

Currently any

[mattwire]

ok, please update the message to make it clear

[eileenmcnaughton]

So you just want the word 'any' added in here "the amount of any contribution"

[mattwire]

In recent releases also what was the specific release - 5.49 - let's include it in the message to make it clear.

[mattwire]

Have posted this as a suggested code edit below

[eileenmcnaughton]

thanks for checking the version - committed - I can rebase when we are happy

[mattwire]

I think we definitely need input from @adixon here because payment processors do work in different ways. All the ones I'm involved in developing are processor initiated for recurring payments - ie. the processor manages the recurring schedule/amount and we update CiviCRM based on what it tells us happened - that can/does work the other way in that CiviCRM can tell the processor to make changes (eg. Smartdebit uses the CiviCRM UI to adjust the next amount/frequency via the Smartdebit API).

[seamuslee001]

Most of the payment processors I have worked with (eWAY, iATS, Moneris) have worked on the basis of CiviCRM triggering a charge to the credit card.

Also as far as I was aware, only if you had 1 line item in your linked contribution could you ever edit the amount of a recurring contribution and you did that by editing the recurring contribution there wasn't any concept until very recently of any contribution having its amount changed affecting the amount on the recurring contribution.

The 2 payment processors I have had experience with where it is more the payment processor controlling the amount (Auth.Net and PayPal) haven't ever tried to update the amount in the contribution_recur just used the inbounce params to override what was set in civicrm_contribution_reur.amount` see https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/AuthorizeNetIPN.php#L154 and https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/PayPalIPN.php#L121 https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/PayPalProIPN.php#L297

[eileenmcnaughton]

@mattwire yes - I think the problem arises in particular for Processors where they initiate a charge based on the recurring contribution - because if that changes unexpectedly they charge the wrong amount. So, some process edits an existing contribution linked to the recurring and the recurring contribution record changes - resulting in the wrong amount being charged.

By contrast if the processor 'knows' about a change then it should (& most do) update the recurring amount. Or, a user can update the recurring amount by editing the contribution record (single line item only) or editing the template contribution (which will edit the recurring).

Can you explain the flow in which updating the RecurringContribution record based on an edit to the amount of a contribution in the series other than the template contribution makes sense?

[adixon]

This change makes sense to me, in principle. I'm also surprised that editing an existing, non-template contribution related to a recurring sequence would impact the amount in the recurring contribution, that seems like a huge mistake (though rarely encountered).

[mattwire]

Can you explain the flow in which updating the RecurringContribution record based on an edit to the amount of a contribution in the series other than the template contribution makes sense?

No, that sounds fine. But I wanted to be clear that we wouldn't be introducing unintended side-effects for popular processors eg. IATs here. If we can get the upgrader message updated per comments above I'm happy for this to be merged.

[eileenmcnaughton]

I just rebased & squashed

[eileenmcnaughton]

@mattwire is this mergeable now?

[seamuslee001]

ok This seems to have general agreement and if we need to improve the wording we can do that as a follow up PR