First recurring payment (paypal ipn) - remove redundant status set, start_date change

[eileenmcnaughton]

Overview

First recurring payment (paypal ipn) - remove redundant status set, set start_date centrally

Before

The paypal IPN code updates the contribution status id from 'Pending' to 'In Progress' on the first payment. This is redundant as it is later done in BAO_ContributionRecur::updateOnNewPayment and incorrect as it is using the wrong option group (contribution vs contribution recur status options) - and the test shows it is done with these lines removed

On removing this the only 'reason' for the $recur->save() here is to update the start date to the current date when updating from 'Pending' to 'In Progress' - I feel like if this is worth doing it should be done in the BAO_ContributionRecur::updateOnNewPayment such that it is consistent

After

update to recurring removed from the PaypalIPN

Per discussion I originally wanted to centralise this but settled on removal as ... it's complicated

Technical Details

@mattwire @KarinG @adixon - I'm inclined to think it would ALSO be OK not to update the start date in paypal since it barely changes from the initial date if you disagree with always updating the civicrm_contribution_recur.start_date field when the first contribution in the series is completed

Comments

[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).

[mattwire]

@eileenmcnaughton A quick look at this PR it looks like you're updating the recur start_date every time a payment comes in - that's wrong.

[adixon]

I'm nervous about any kind of side effect like this, it seems reasonable but payment processing isn't always reasonable. Even if it did try and figure out if it was the 'first' of a sequence, that concept might not match other ideas of what first means (e.g. do you count failed ones?).

[eileenmcnaughton]

@mattwire - no - only if the recurring contribution is being changed from 'Pending' to 'In progress'

[adixon]

@eileenmcnaughton - great example of what I wrote above - a recurring contribution being changed from pending to "in progress" might normally be the first one, but may not be (e.g. currently, ACH/EFT iATS contributions, though I know that's a whole other ball of wax ...).

[eileenmcnaughton]

@adixon so how does that work - you are saying that they date it starts (a payment is first received) is different to the date it goes from 'pending' (no payments received) to 'In progress' - one or more payments received?

[eileenmcnaughton]

test this please

[adixon]

Yes, an ACH/EFT contribution is initially pending just like every other responsible contrribution. A request for the payment goes to iats and generally, if it's syntactically good enough, gets a transaction id back that really just says, "accepted for processing". In a (business) day or two, CiviCRM gets notice back from iATS (via a report that gets pulled hourly, not a callback) that it has been accepted by payers the merchant bank (or not), at which point CiviCRM/iATS extension will convert the contribution to complete or failed.

https://github.com/iATSPayments/com.iatspayments.civicrm/wiki/Verification-and-the-Journal

[eileenmcnaughton]

@adixon ok - but how does that relate to the civicrm_contribution_recur.start_date field - ie this proposal is that the start date is consistently updated to reflect when the first payment is received - ie when the first contribution is updated to completed.

[adixon]

My main point is that recurring payments are a very mixed bag and making and enforcing any assumptions across the board is likely to create new unexpected problems.

My answer above was trying to point out that your logic is assuming a cc type of sequence of predictable payments, but it's also a good example of a continuing confusion with recurring payments - i.e. that even amongst developers, we get confused between a recurring schedule and a contribution that is part of a recurring schedule, because we call them both recurring contributions at different times.

But maybe it would help if you describe what problem this automation is intended to solve?

[mattwire]

@eileenmcnaughton I think this really needs a lab issue first as it's a proposal that needs discussing. This issue and related comments there by @adixon explain very well some of the reasons why start_date != first payment received https://lab.civicrm.org/dev/financial/-/issues/139#note_39942

[eileenmcnaughton]

@adixon @mattwire so the logic in core is that

1. a civicrm_contribution_recur record is updated
2. the first time a contribution against that record is completed then the status of the civicrm_contribution_recur record is updated from Pending to In Progress

Both Authorize.net and Paypal at the same time update the civicrm_contribution_recur.start_date to be the date of that first completed contribution.

In the case of Paypal it actually does 3 things

1. updates the contribution_recur.status_id to In Progress - this is done referencing the wrong option group & also duplicated in ContributionRecur::updateOnNewPayment` - so it can be removed
2. updates the contribution_recur.modified date - this is done in the BAO & can be removed
3. updates the contribution_recur.start_date - this last one is not done anywhere else - but if it is (as the schema says) - 'The date the first scheduled recurring contribution occurs.' - it makes sense that this be done at the same place in the code that updates the civicrm_contribution_recur record to 'In Progress' - which would also remove code from Authorize.net & get us closer to cleaning up the whole completeOrder / repeatTransaction as getting those 2 classes to call the right api is part of that chunk.

I couldn't see anything in the linked gitlab that discussed start_date - it all seemed to be about the contribution record.

Haven't said all that - I'm also OK with dropping this & just removing the 1 line from the paypal class & not also trying to leverage this to remove some of the obstacles to cleaning up completeOrder

[mattwire]

I think paypal (and core authorize.net) are doing the wrong thing if they're updating start_date on the recur. It creates inconsistency and seems limited to those two processors.
The ticket I linked to discusses contribution.receive_date but the discussion/issue/solutions are very similar to contribution_recur.start_date because they are both set at the same time and usually are set to either "now" or to some defined time in the future.
An example with Stripe credit card and delayed start_date enabled: Complete contribution page and setup a monthly recur to start in 5 days time. In 5 days time the contribution fails. It is then retried 2 days later and succeeds. The intention was still that the payment was taken on day 5 and that's what will happen the following month. So we don't want to change the start_date on the recur or the receive_date on the contribution. But we do record the trxn_date on the financial_trxn record on day 7. Similar examples apply to most recurring payment processors.

[eileenmcnaughton]

@mattwire I'm going to go with your version & rip out changing the start_date on paypal then - purely because it is the easiest :-) and I was already marginal on the benefit of updating the date in paypal because paypal recurrings happen in real time anyway

[eileenmcnaughton]

@mattwire updated to just remove the code

[eileenmcnaughton]

thanks @mattwire