CRM-21854 - Contribution start date and end dates are not respected #11881
Conversation
|
@yashodha whenever we touch a datefield in a form we should change it to use datepicker - I'm guessing these fields aren't since CRM_Utils_Date::processDate shouldn't be needed for a datepicker field (also you need to strtotime before comparing) |
| $endDate = CRM_Utils_Date::processDate(CRM_Utils_Array::value('end_date', $this->_values)); | ||
| $now = date('YmdHis'); | ||
| if ($endDate && $endDate < $now) { | ||
| throw new Exception(ts('The page you requested has past its end date on '. CRM_Utils_Date::customFormat($endDate) ), $this->_id); |
There was a problem hiding this comment.
should we be chucking Exception or CRM_Contribute_Exception_InactiveContributionPageException exception? @eileenmcnaughton
There was a problem hiding this comment.
ohh clever spotting!
|
@eileenmcnaughton : used datepicker instead. Should we do a grep and replace in the code base for consistency. |
|
@yashodha - it would be great to clean them all up in one fell swoop- - but a bit risky since we need to test each one :-( |
| @@ -0,0 +1,25 @@ | |||
| <?php | |||
|
|
|||
| class CRM_Contribute_Exception_FutureContributionPageException extends Exception { | |||
| @@ -313,6 +313,17 @@ public function preProcess() { | |||
| throw new CRM_Contribute_Exception_InactiveContributionPageException(ts('The page you requested is currently unavailable.'), $this->_id); | |||
| } | |||
|
|
|||
| $endDate = CRM_Utils_Date::processDate(CRM_Utils_Array::value('end_date', $this->_values)); | |||
There was a problem hiding this comment.
so I wonder why $this->_values is not in iso format then? -
There was a problem hiding this comment.
@eileenmcnaughton : it is in iso format as well but needs to be compared to $now
$this->_values['end_date'] = 2021-04-27 00:00:00
$now = 20180523123026
There was a problem hiding this comment.
OK - so I guess the readability thing is that instead of using standard php functions we have to dig into a custom function to realise it's basically not doing anything a std function doesn't.
To make it more readable we should make it clear we are comparing like with like - ie preferably compare timestamps
if (!empty[ $this->_values['end_date']) {
$timestamp = strtotime(CRM_Utils_Array::value('end_date', $this->_values));
if ( $timestamp < time())
....
But if for some reason that is going to be a problem then at least format $this->_values['end_date'] the same way as $now ie.
if (!empty[ $this->_values['end_date']) {
$timestamp = strtotime(CRM_Utils_Array::value('end_date', $this->_values));
if ( date('YmdHis', $timestamp) < date('YmdHis'))
....
There was a problem hiding this comment.
@eileenmcnaughton
how about using CRM_Utils_Date::isoToMysql which makes it clearer?
There was a problem hiding this comment.
Hmm - I feel like for most devs using straight php functions makes is clearer since they are highly used to those & the Civi date functions can be a bit weird & wonderful - but I do agree that is a lot clearer than 'processDate'
Is it really ISO format ? I mean there would never timezone info on the date?
|
I'm just looking through PRs & assessing this one. It seems to be that it was just about merge-ready but the last comment wasn't replied to so it dropped off my radar. I'm not loving the user experience of this -
I think a bounce + a message would be better and I think the processDate function is unnecessary & hampers readability. However, it's the user experience worse than it is without the patch (given that the field is ignored), the Exception structure is nice & it removes technical debt on the datepicker field. So I'm opting to merge it. |
Unreleased regression from civicrm#11881 see https://lab.civicrm.org/dev/core/issues/263
Unreleased regression from civicrm#11881 see https://lab.civicrm.org/dev/core/issues/263 Change-Id: Ida8a07cf0d641008a099dfdbe87acf6ebfab3e51
Unreleased regression from civicrm#11881 see https://lab.civicrm.org/dev/core/issues/263
Overview
Steps to replicate :
Before
able to access the contribution page
After
not able to access the contribution page
Comments
Users are getting confused as to why the contribution continued to accept payments after the end date. There is no way to set a contribution to end without manually going in to inactivate.