Add default receive_date for contributions at BAO level

[eileenmcnaughton]

Overview

Fixes test bug by setting a default contribution receive_date in the Contribution.create BAO

Before

test fails in isolation. Can create contributions with no receive date by api

After

test passes. All contributions have a receive date.

Technical Details

This PR assumes

1. receive_date is required
2. if nothing is provided by the time it hits the BAO::create then 'now' is an appropriate default

We know 1 to be true because contributions with no receive_date cannot be edited - I can't find the gitlab right now (maybe @seamuslee001 can but there was a related regression fixed by setting this on the membership renewal form here #14316 - which is fine to do on forms but if it's 'never ok' we should 'never do it'

I found that CRM_Contribute_Page_AjaxTest actually fails if run in isolation - obviously 'something' is
helping it out when run together but in isolation the receive_date is missing on the contribution
which causes an error when it goes to save financial item. (There was a similar test issue recently
that turned out to impact form users - this solves the issue deeper down. The default is only applied
if id is not present

Comments

There is 'something' happening in test cleanup at the moment. 2 recent PRs have triggered seemingly unrelated tests to fail. In this case it's the opposite - a failure was hidden

@JoeMurray @monishdeb @pradpnayak @mattwire

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

[JoeMurray]

1. A pay later contribution should have a receive date of null I expect, going back to v1.3.
2. I suspect there is a low risk that some code doesn't do something if receive_date IS NULL, eg might an update of a 'default' for a next recurring check first before setting a value??

If this is to be a default, just want to reconfirm that it is not possible to use something like now() as a default in xml schema def: https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Contribute/Contribution.xml#L143.

I don't have an objection in principle, just worries that this might break something by changing after 14 years.

[eileenmcnaughton]

@JoeMurray I tested event page & contribution page & pay later DOES get a receive date - I couldn't find a way to enter a pay later contribution with no receive date - now that we fixed the place where a form let it through - but it just feels dodgey to rely on us never breaking the form layer again

[eileenmcnaughton]

My gut says this probably is right - but it is a change with some risk or perception there of & I'm gonna close as I think that is pretty hard to resolve - just fixing the test by adding the param to the test instead

[JoeMurray]

Hmm. I'm now thinking if I was wrong on Pay Later then I'm less sure this is very risky. @Monish what do you think?

[monishdeb]

@eileenmcnaughton @JoeMurray I think there's no harm in setting default receive date (RD) to the current day. When I did a pay-later contribution via live donation page, on submitting, it does set the receive date to the current day. Similar behavior on backoffice contribution as RD is a required field. And so does at API level https://github.com/civicrm/civicrm-core/blob/master/api/v3/Contribution.php#L108

[sunilpawar]

@eileenmcnaughton i don't know how useful for this task, but we have one case where we are getting DB error ( tested on v ESR 5.7.6 same thing is working without error on dmaster site)

1. Go to contact, click on 'Events' Tab.
2. Click on 'Add Event Registration'
   • choose the event and other fee details etc.
3. Check 'Record Payment?' Checkbox
4. un-set 'Received' Date field (both date and time) under 'Payment Information' section
5. save

Event and Contribution record get created but the 'civicrm_financial_item' table entry get failed because date is not provided.

INSERT INTO civicrm_financial_item (transaction_date , contact_id , description , amount , currency , financial_account_id , status_id , entity_table , entity_id )
VALUES ( 0 ,  114003 , 'Commercial Booths' ,  1800 , 'USD' ,  5 ,  1 , 'civicrm_line_item' ,  99244 ) 
[nativecode=1292 ** Incorrect datetime value: '0' for column 'transaction_date' at row 1]

Mysql Version : 5.7.26

[eileenmcnaughton]

@sunilpawar thanks - yes thats the scenario I'm concerned about - I have a suspicion we have a few cases where this would happen in the test suite except that some clean up issue hides it. When we accidentally made it possible to create a contribution with a receive date from membership renewal (back office) we saw this symptom there too.

To my mind the reason to do this is because we are obviously prone to flows where we fail to set it & cause weird errors without ensuring it's set at the BAO level - as evidenced by @sunilpawar finding another instance.

@pradpnayak @mattwire any thoughts - I feel like we are close to agreeing to merge this

[eileenmcnaughton]

Just a note that a deep dig into a failing test yesterday revealed that contamination of the CRM_Core_DAO::_nullArray was behind some cases where tests failed when run in isolation & passed when run together & vice versa. I haven't got fully through ferriting those out but I think there are tests that without that bug 'providing' a date for saving the financial item we will need this fix to provide a legit one (or to make more significant test changes but I lean now to merging this)

[MegaphoneJon]

I'm not blocking this merge - but when someone selects "Pay Later" then pays later, there's no way to search by the actual received date. I've nudged my client away from removing the receive_date value on pending contributions, but the truth is there's very little alternative ATM.

[eileenmcnaughton]

@MegaphoneJon yeah that's tricky - because it could be updated when received but generally there has been a multitude of uses for receive_date so messing with it is... dangerous.

How does your client remove the receive date?

[MegaphoneJon]

I had an extension that removes them, but we never rolled it out to production because, as you noted, this feels dangerous.

[eileenmcnaughton]

I guess that would still work on the pre-hook - as long as you alter receive_date to 'null' this would not change them

[MegaphoneJon]

@eileenmcnaughton I'm actually not sure that it would. I opened (then closed) financial#48 which explains my concerns in more detail. I'm not blocking this merge because, like financial#48, wiser minds (@mattwire) point out that we need a longer-term solution.

[colemanw]

Sounds like we have general agreement to merge this, and tests are passing.

[magnolia61]

Just for reference: the receive_date is still not required in the record payment section of the event registration form. This way contributions without a receive_date will slip through. Should this form field be required? https://lab.civicrm.org/dev/core/-/issues/1228