dev/financial#197 Revert freezing on total_amount field on recurring form (alt to #23796)

[mattwire]

Overview

dev/financial#197 Revert freezing on total_amount field on recurring form

Before

#21473 results in the amount field being frozen for recurring contributions - it was reviewed as "PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature." - but that is pretty clearly wrong as users are used to editing the amount, and are unfamiliar with the template contribution - which often does NOT permit editing the amount

After

The field is editable again

Technical Details

This returns the field to the previous state. On checking the code this might mean the recurring amount is out of sync with the template contribution. However, that just takes us back to the status quo which was fine for most users. For single line item contributions the contribution_recur.amount field takes precedence and it doesn't matter if the two are out of sync.

Ideally we need to ensure that any changes to the ContributionRecur.amount are blocked for non-single row or cause a corresponding update for single row templates and this needs to happen regardless of whether the form or the api or BAO makes the update. (It currently IS the case on this form)

Comments

We agreed to underlying schema changes in Barcelona - which have been implemented. Since then UI changes have also been made to add the contribution template to the UI. UI changes definitely need more scrutiny but unfortunately I merged the PR that made this change based on the review, rather than my own analysis, and did not realise it made an important UI change that should have gone through the dev list.

This implements a quick revert on the problematic part (as suggested by Matt). We can re-discuss a fix in master but after testing this out I don't think the freeze is acceptable while we do this

@KarinG @mattwire @adixon @seamuslee001

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

This is fine - but I don't actually see us going back to freezing it for single line items - rather I think we should update the template contribution when it is edited for single line items.

[adixon]

There are many things to like about the new template system for recurring contributions. Last week we identified two missing pieces that were causing problems:

1. The ability to edit the template (using the admin ui) was broken in the sense that you got funny errors when trying to save. Matt already had a PR for that (created way back Sep 2021) and so it was merged (on Jun 7 2022):

https://lab.civicrm.org/dev/financial/-/issues/194

#21471

2. That however revealed a complicated issue in that some recurring contributions now required the line item editor in order to modify them, and editing a recurring contribution template using the line item editor didn't update the amount field in the recurring contribution record amount field. That's this issue and fix:

https://lab.civicrm.org/dev/financial/-/issues/195

https://lab.civicrm.org/extensions/lineitemedit/-/merge_requests/70

So now the issue is how to handle these issues wrt the next releases, right?

In my view, 21471 should get backported to the next release if it's not already in there, and we should move forward.

It does leave the issue that you need a line item editor to modify recurring contributions, but I think that's better than having the ability to leave your system in an incoherent state where the total of the recurring template is different from the total of the recurring contribution schedule entity.

re: iATS - the most difficult part of the regression from a user point of view was that after 1. was in, but before 2. was, users could modify the template and that would not change the recurring contribution schedule. The iATS extension uses the recurring contribution schedule amount to take the money and then uses the (core) repeattransaction api to actually generate the contribution (which is already using the template amount, apparently!), resulting in CiviCRM recording a different amount than what transacted in iATS.

So the key points are:

1. Although we noticed with our iATS clients, it wasn't due to anything iATS specific, but could have affected other payment processors that manage the recurring schedule.
2. If we release a system that allows a user to generate a mismatch between the recurring contribution template total and the recurring contribution schedule amount, we'll have the same issue.

If you want me to test a PR against what I'm saying here, let me know, but whatever we end up with, keeping those two amounts in sync is pretty key.

If we can all confirm that the template is now the authority for the total amount, going forward, then I'll also update iATS so that it uses that amount as well.

[monishdeb]

Merging based on MoP tag

[adixon]

Okay, I'll assume that my previous comment was ignored for being too verbose?

Let me be more blunt:

However, that just takes us back to the status quo which was fine for most users. For single line item contributions the contribution_recur.amount field takes precedence and it doesn't matter if the two are out of sync.

I'm not sure this is true, this would need to be tested against reality.

"Status Quo" is not really a useful concept now that the recurring template is in core, which has lots of unexpected consequences.

[KarinG]

Monish why did you completely ignore everything Alan just posted to this PR? That’s highly inappropriate.

[eileenmcnaughton]

I think the idea that we need to do a quick revert & resolve in a later release was correct - so Monish's action was appropriate - the discussion is whether we should re-freeze in a later release - which is an issue for the dev-digest since it is a UI-opinion issue.

Re does this reverrt us to the status quo - prior to 5.49

• you could edit recurring contribution via the screen and it allowed the total amount to be edited for single-line-item contributions (only) - the repeattransaction api has been written to treat the civicrm_contribution_recur.amount value as authoritative and the unit tests cover that.
• the template contribution screen was in core but if you chose to use it then changes to the amount would not flow through to the contribution_recur.amount

In the release about to go out

• you can edit single line contributions' contribution recur.amount again as has always been possible. If you do this value is authoritative through repeattransaction - as has been the case for years - the status quo is returned here
• the template contribution screen was in core but if you chose to use it then changes to the amount will flow through to the contribution_recur.amount

Hence this reverts the change to the UI but not the fix from #21473

The benefits of the freeze are pretty doubtful since there is no retrospective cleanup of data and it doesn't address contribution_recur.amount being updated through the api/BAO (and if that was addressed there would be no need for the freeze) - however, that is a discussion for another forum since this was a UI change that I merged and should not have done without notifying the dev-list (one which I didn't realise was in the PR as I merged based on the review comments)

[eileenmcnaughton]

Note a quick revert and later fix is our standard policy -

[mattwire]

the repeattransaction api has been written to treat the civicrm_contribution_recur.amount value as authoritative and the unit tests cover that.

@eileenmcnaughton I'm pretty sure that's NOT the case. At least reading through the code. Contribution.repeattransaction will try to get the amount from the template contribution if it's not explicitly passed in.

[eileenmcnaughton]

Check the getTemplateContribution function

[eileenmcnaughton]

Argh it IS broken - there is a mistake in the code where it is referring to total_amount not amount - that sucks - it obviously isn't tested like I thought & regressed some time back.

But, I should not have merged the PR with the UI change as it doesn't address historical records so it is only a false sense of security and it doesn't address the actual issue (and isn't needed once those ARE addressed) - so we should stick with our standard policy of revert & fix in master

[adixon]

Okay, so your goal is to allow the recurring record to be edited, potentially putting it out of sync with the total in the recurring template, and ensure that the recurring record is "authoritative" for the amount (at least as far as core api functions go).

That would have the benefit of (potentially, to be tested) restoring self-serve updates of recurring amounts (without needing to edit the recurring template) and would make the recurring template more like the best-effort kind of templating that we had previously (i.e. before this, we used the most recent contribution that has the same total amount as a template, if available).

That all sounds like a reasonable compromise. I'd guess in either case, we have quite a few other potential places where things could go sideways, so probably best that we all bullet-proof any recurring contribution generation to make sure we don't get a mismatch via some other unexpected process.

Ensuring consistency between the recurring record total amount and the template really will require that total amount in the recurring record to be frozen in some circumstance, e.g. a contribution with a bunch of line items, but for simple donations with one line item, it does seem overkill to force the use of the template.

I'm game to test, or tell me if there's some code you want reviewed.

[KarinG]

Hi Alan @adixon when you get to testing - this seems like a good place for me to document a specific example of repeattransaction API no longer caring about the recurring series amount at all - it is **not** using the most recent contribution that has the same total amount as a template. And it's even overriding the cleverness that exists in the iATS Extension!

Feel free to move this example elsewhere if it needs to go elsewhere.

1. This was reported by a mutual client of ours during first monthly reconciliation period after upgrading to ESR 5.45.* (from previous ESR) -> so repeattransaction API behaviour changed between ESRs.
2. Their processes had not changed; iATS extension has not changed;
3. Process includes to record the NSF (this is a direct debit return) as a (-$5) and copying over all other details associated with the Contribution that was returned, including the recur_id
4. iATS Extension successfully charged $5 (amount defined in the recurring series) on May 08
5. Repeattransaction simply grabbed the last amount in the series (-$5) and recorded that (-$5) as the Contribution amount on May 08 even though the payment was $5
6. I did show Eileen this screenshot on May 18 - but we both weren't sure what to make of it at that time. Now it's clear it may well have been the first clue that the decision making process for repeattransaction API has changed to undesirable outcomes.

[eileenmcnaughton]

goal for this PR is simply to get back to where we were before I merged that PR that went into 5.49 - fix for the long-standing issue is for master

[adixon]

Regardless of that goal, a lot of other things have happened and we need to retest with the current code base for new unexpected side effects. https://brandondonnelly.com/2017/04/09/this-river-i-step-in-is-not-the-river-i-stand-in/