[jfilter]

Fixes #823.

• FRST-STEP 3b in Batching::updateRCUR: a pending first installment in an open FRST group of the same creditor is re-dated to the new collection date and reused, so syncGroups() moves it and the emptied group is cleaned up. Several candidates → warning, mandate skipped for this run. On-hold installments are not re-dated.
• hook_civicrm_check warning for open, non-empty groups past their submission deadline.
• Tests for the overdue-group case, the ambiguous case and the status check; one paragraph in docs/usage.md.

Re-dating is consistent with Group::received() and STEP 4 and keeps the date-based lookup idempotent. RCUR is unchanged: a missed RCUR rate may legitimately be collected later.

[dontub]

Thanks for the PR @jfilter! Can you have a look at my comments and questions?

[dontub]

The line $existingContributionsByRecurId[$recurId] = $installment; below would prevent new FRST contributions being created, if there's already one pending FRST contribution, and the existing contribution would be moved to a new transaction group. So count($installments) > 1 can actually only be true in cases before the code was added. In such a case the filtering of $mandates below would result in no new FRST contribution being created and the existing FRST contributions might be removed from there current groups, depending on whether a mandate with the same collection date remains in $mandatesByCollectionDateAndFinancialTypeId. Is my understanding correct or have I missed something?

[jfilter]

Yes, that's how I understand it too. With the fix the batching doesn't create a second FRST installment anymore, so this can only happen with data from before (or contributions created some other way). I didn't want to guess which one is the right one, so the mandate is just skipped and there's a warning in the log.

The existing contributions are then only handled by the normal syncGroups(), same as before: if one of them is in an open group whose collection date also comes up in this run, it gets removed from that group. Overdue groups (date in the past) aren't touched. Without the check the batching would just have added another installment.

[dontub]

Well, instead of having a lot of code that it's only used a short time for situations from the previous implementation, I think I'd prefer to just keep the previous behavior in cases where there's already more than one FRST contribution:

if (count($installments) !== 1) {
  continue;
}
// Code to prevent second FRST contribution.

Another thing that I could imagine is to add an upgrade step to \CRM_Sepa_Upgrader that removes pending FRST contributions, but the last one.

[jfilter]

@dontub I can change that if you like, or do you want to do it yourself?

[jfilter]

Thanks @dontub for the review! I rebased onto master, which should also fix the earlier CI failures (testBug629 and the Smarty errors were fixed on master in the meantime) once the workflows are approved to run. Answered your comments inline.

[dontub]

@jensschuppe: Do you think we can use this as new default or might there be users who want a different behavior?

[jensschuppe]

@jensschuppe: Do you think we can use this as new default or might there be users who want a different behavior?

Summarizing from my understanding:

• Currently, FRST installments get recreated for new runs if no FRST installment had been processed by then (i. e. installment date being the latest run, given that old groups have not been collected)
• Now, FRST installments in open groups past their collection date are being moved to the new group (with the installment date changed to the new group's date) - only if just one FRST installment exists for the mandate
• Now, if there's more than one FRST installment in old groups, no new installment will be collected for that mandate and a warning be logged instead, i. e. the user will have to remove the corresponding contributions manually before proceeding or else the mandate won't collect anything, forever.

If that's how it works now, we should add complete instructions or a link to the docs what this actually means and how to solve it. I can imagine having to a) find and b) delete the correct contributions can be challenging for some users. The new code just logs to the CiviCRM log and not to the UI though. So with this approach, the user won't notice that/why a mandate is not being processed.

That's an edge case, but (known) edge cases are the ones that need the most explanation, I'd say. Especially when it comes to time-critical things with money involved.

[jfilter]

@jensschuppe Thanks for the summary. One correction to the first point: with the current behavior, the FRST installment is not recreated instead of the old one but in addition to it. The old pending contribution stays in the overdue open group, and the new one goes into the new group. Closing both groups collects the first payment twice. That is what happened to us in production (#823; #634 already noted that "legitimate FRST contributions still will be collected as duplicates in this case").

So the current default is a double debit unless someone notices and deletes the old group by hand. I don't see a use case that depends on it.

[jensschuppe]

I got that the issue is multiple FRST installments being created currently. This PR might actually lead to the opposite: no FRST installment (and thus no subsequent installments either) being created for affected mandates.

That's not saying we shouldn't do this, but document and report to the user what to do in that case (actively in the CiviSEPA UI and when it occurs, not just logging or adding to the status report). Everything else seems fine concept-wise.

Users who rely on FRST installments currently being created for new groups regardless of old open groups (which those users probably forget about/delete anyway) would be affected; and those users should get a clear notice that this behavior changed - that's basically my answer to @dontub's question 😉

[jfilter]

@jensschuppe @dontub Thanks! I went with dontub's suggestion in 35ead1c: with several pending FRST installments none is reused and a new one is created as before, so no mandate is left without a first collection. The single-installment case still moves the pending one. docs/usage.md now states the behaviour change and what to do with several pending first collections. Happy to squash the commits if you prefer.

[dontub]

Actually I'd prefer to have an upgrade step that reduces the FRST installments to each last one so there's a consistent state and no different behavior in batching. WDYT @jensschuppe @jfilter?