Permit additional (freeform) relative date filters - e.g 'last 18 months', 'last 45 days'

[eileenmcnaughton]

Overview

Genericise 'ending' date filter and add tests

Adds unit tests for date filters of the type 'ending' and adds more options - that are not exposed in the UI but could be exposed via an extension or the api

Before

The following 'ending' options work from date filters but have no tests

Last 60 days including today	ending_2.month
Last 2 years including today	ending_2.year
Last 3 years including today	ending_3.year
Last 30 days including today	ending.month
Last 90 days including today	ending.quarter
Last 7 days including today	ending.week
Last 12 months including today	ending.year

After

The above options still work and have unit tests

In addition it is possible to add additional 'ending' filters into the civicrm_option_value table and they will work in the UI - e.g

results in

The additional filters supported are

ending_n.month
ending_n.week
ending_n.year
ending_n.day
ending_n.quarter

Where n can be any number. The choice of label is up to the person adding them.

Technical Details

Currently we have hard coded variants of 'ending_2.year' for 'Last 2 years including today' and
for month, week and quarter we support last month ending today etc.

There is a complexity in that the current month entries are

ending_2.month = 'Last 60 days ending today'
ending.quarter = 'Last 90 days ending today'

I think it was done this way primarily to get the language right but as the period gets longer
(my interest is in supporting 18 months) the mismatch between 30 days & a month becomes more & more of an issue.

I actually feel a pretty good case could be made for replacing the default
entries so instead of

ending_2.month = Last 60 days including today

we would have

ending_60.day = Last 60 days including today

(I would probably do that on new installs only if we go that way)

Comments

I haven't focussed on any code rationalisation as I wanted to keep this to
additional code not changes to current & discuss / agree if this approach (ie. for the 'n'
entries we are respectful of actual month lengths & we leave it to future option value
enterers to worry about the wording.

@MegaphoneJon @magnolia61 @joannechester you have all had a role in previous work around these filters

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

@MegaphoneJon @magnolia61 any chance either of you can chime in on the appropriateness of my calculations etc

[eileenmcnaughton]

@MegaphoneJon I was hoping to discuss this with you....

[MegaphoneJon]

I was guiltily eyeing this today! I'll do my best to make time on this tomorrow.

[eileenmcnaughton]

thanks @MegaphoneJon it does need a little bit of thought (aka coffee) around the interpretations (given the 30 days assumption previously) so .... coffee

[MegaphoneJon]

My sense is that calling ending_2.month "Last 60 days ending today" is a clear bug. I also think your change is the correct one. I think it's fine to limit this to new installs, though I'd probably put an update message in for folks who use that filter.

I haven't reviewed this PR in depth (let me know if you'd like that, or you wanted to have this discussion first) - but a long while back I'd intended to add the ability to pass a datetime in to CRM_Utils_Date::relativeToAbsolute() and CRM_Utils_Date::getFromTo() with a default of now() rather than ALWAYS being now().

This would make them pure functions (when the datetime was defined), which would make it very easy to write robust tests. As a bonus, it would facilitate my on-again off-again "make your own relative date filter" extension by allowing me to display the from/to dates for an arbitrary date (e.g. "does my filter work correctly on leap days?")

[MegaphoneJon]

After a moment's reflection, I thought, "Of COURSE @eileenmcnaughton wants me to review this PR". So I did.

My primary concern here is that we're locking in the incorrect behavior for months with a test. I would feel better if we were NOT testing the ending.month and ending_2.month scenario. I would feel better still if we allowed passing in the datetime per my last comment, so we could actually test with absolute dates.

Tangentially, just yesterday I was writing some custom code to pull dates that amounted to "last 12 calendar months not including the one we're currently in" (e.g. on "2018-09-07" relative dates would be "2017-09-01 to 2018-08-31"). It was as simple as:

    $startDate = (new DateTime('first day of this month last year'))->format('Y-m-d');
    $endDate = (new DateTime('last day of last month'))->format('Y-m-d');

[eileenmcnaughton]

@MegaphoneJon I updated so that we are testing the day variants & adding the day variants on new installs and I altered the descriptor on new installs. I left a couple of things out of scope.

1. adding the day variants on new installs & renaming the month variants to say 'last one month". I think we could get away with that with an upgrade message. We couldn't remove the current month variant in case it is used in reports / smart groups already. I note that I only focussed on 'ending' & 'starting' still exists so I think the change can wait for another round.

2. relative dates seems easy enough to add but I didn't want to extend the scope to relevant tests right now

++ b/CRM/Utils/Date.php
@@ -1083,12 +1083,14 @@ class CRM_Utils_Date {
    *   Relative time frame: this, previous, previous_1.
    * @param int $unit
    *   Frequency unit like year, month, week etc.
+   * @param string $relativeToDate
+   *   Date for the range to be relative to, in a format understood by strtotime.
    *
    * @return array
    *   start date and end date for the relative time frame
    */
-  public static function relativeToAbsolute($relativeTerm, $unit) {
-    $now = getdate();
+  public static function relativeToAbsolute($relativeTerm, $unit, $relativeToDate = 'now') {
+    $now = getdate(strtotime($relativeToDate));
     $from = $to = $dateRange = array();
     $from['H'] = $from['i'] = $from['s'] = 0;
     $relativeTermParts = explode('_', $relativeTerm);
(END)

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@seamuslee001 @MegaphoneJon I think this is mergeable now?

[magnolia61]

Not sure if this is 100% related but some relative filters seem to got broken after some changes in May: https://lab.civicrm.org/dev/core/issues/438

[eileenmcnaughton]

@magnolia61 ok - this isn't actually merged yet.... but are you on the latest?

[MegaphoneJon]

I'm so sorry I haven't had a chance @eileenmcnaughton - but I've bumped this up in my priority. I'm busy so I haven't had time for pro-bono work outside of governance, but I know this has sat.

[eileenmcnaughton]

Just a note that the https://lab.civicrm.org/dev/core/issues/438 is unrelated

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@MegaphoneJon how is that list looking?

[MegaphoneJon]

OK, I got a chance to look at this tonight and I'm satisfied. I'll note that CRM_Utils_Date::relativeToAbsolute('ending', 'day') gives an unexpected result:

~dmaster(branch:master*) » cv ev 'return CRM_Utils_Date::relativeToAbsolute('ending', 'day');'
{
    "from": "20181108000000",
    "to": "20181107235959"
}

but that doesn't seem to affect anything else, and that's not an option someone would pick since "today" is an option.

[eileenmcnaughton]

Thanks @MegaphoneJon !

[sluc23]

Hi @eileenmcnaughton , will this PR allow to create custom time filters within extensions, or am I misunderstanding the scope of this?
In that case, how would it be created in the extension?
(In my case a customer asks us to add "Last 9 months including today" filter)

[eileenmcnaughton]

@sluc23 right so you add additional option values to get that to work - ie in value you would have

ending_9.month

[eileenmcnaughton]

(you can just do one off's in the db on the site)

[magnolia61]

late to the party. looked at this PR. Am I correct in the assumption ending_n.fiscal_year should work only fiscal_year is not in the unit test?

[eileenmcnaughton]

@magnolia61 I didn't tackle ending_n.fiscal_year - or a bunch of other possible alternatives

[MegaphoneJon]

I have a branch somewhere on my Github that adds the hook for defining your own relative date filters - but haven't had time for the review process. @sluc23 or @magnolia61 if someone wants to champion this, I can submit it.

[eileenmcnaughton]

@MegaphoneJon that would be nice - I would note I think the changes I made to support 'n' should be made to fiscal date filters etc - I just left it out of scope for manageability

[magnolia61]

I added a freeform relative date filter for "This Fiscal Year". (PR: #13440) After giving it some thoughts it does not seem desirable to have a ending_n.fiscal_year. It just does not make sense. What we needed was This and x fiscal years back. Hope I used to code logic well enough, it works perfectly for us. If someone could take a look at the logic I used, I could change the other filters for 'this' (for year, month, etc)