Add previous before and previous 2 fiscal year options

[larssandergreen]

Overview

Having the Year prior to previous fiscal year and Previous 2 fiscal years relative date options would be convenient and enable useful SearchKit searches without manually entering and updating dates (e.g. people who donated two fiscal years ago and last fiscal year or people who donated in the last 2 fiscal years, but not this fiscal year).

Luckily, these were already present and working properly in code, so the options just needed to be added to enable them.

Before

Year prior to previous fiscal year and Previous 2 fiscal years relative date options not present.

After

Year prior to previous fiscal year and Previous 2 fiscal years relative date options present.

Comments

Have tested the upgrade, but not the fresh install. Trying to use civibuild with --patch, but I get "ERROR: Missing main script (/download.sh)".

Is there any way to test civicrm_data.tpl without a new install?

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

[demeritcowboy]

Is there any way to test civicrm_data.tpl without a new install?

Not really. But you'll need to generate a sql/civicrm_generated.mysql which indirectly tests it because the test site here will use that. Somebody wrote this cool script that allows you to do that online in your fork/branch: https://github.com/civicrm/civicrm-core/blob/master/.github/workflows/regen.yml. Or you can run bin/regen.sh locally just note it wipes your db.

[demeritcowboy]

INSERT IGNORE INTO
in case somebody reruns the upgrade, or has already made this customization which believe it or not is supported/documented: https://docs.civicrm.org/user/en/latest/searching/relative-date-formats/#adding-new-filters

[larssandergreen]

Done, thanks.

[demeritcowboy]

I'm not really familiar with {localize}, but it looks like you'd need to also localize the INSERT list, e.g.

INSERT IGNORE INTO civicrm_option_value (option_group_id, {localize field='label'}label{/localize}

[larssandergreen]

Done.

[larssandergreen]

Somebody wrote this cool script that allows you to do that online in your fork/branch: https://github.com/civicrm/civicrm-core/blob/master/.github/workflows/regen.yml. Or you can run bin/regen.sh locally just note it wipes your db.

Great, I added the script option to the dev guide.

[demeritcowboy]

I was wrong about the insert ignore because this table allows two with the same name, so if someone reruns the upgrade you get two (or more) copies of the rows. Also these update statements are going to keep shifting the weight each time the upgrade is run. Or if someone has manually rearranged then the insert might not be in the "right" place.

So after all that wrangling:
(a) While nice-to-have, given that someone can just add their own option values and it's documented, how much more time do you want to spend fighting with it?
(b) You could do this in php using ensureOptionValueExists, and this multilingual technique and maybe just leave the weight updating out and insert at max(weight)+1.

[larssandergreen]

Yes, I see this could get messy. I did wonder a bit about the order shifting. Thanks for helping out with this even though it's pretty unimportant (but we've come this far...)

ensureOptionValueExists does seem like a much better way to go.

For the order, I've done some experimenting and if you specify a weight in API4, it cleverly increments all the higher weight options, so nothing else needs to be changed. I can update ensureOptionValueExists to use API4. I could also add a param called insert_after, which looks up the name of the option (in the specified option group) we want to insert after and then sets the weight to one higher (or does not set a weight if we don't find the option, in which case it gets added as max weight + 1.

That leaves the multilingual part. How about adding that multilingual logic to ensureOptionValueExists, so this is easier in the future?
If we just created an option value and we're multilingual, for each locale, update the row with the translated label and description.

[larssandergreen]

Or probably better just to create a new function that uses API4 and does localization and leave this one as is to ensure compatibility.

[demeritcowboy]

If you want to split this into 2 PRs - one that only adds it on new installs and then a separate for the upgrade, I think it would be an easy merge for the first one.

Also FYI there's some open questions about how much api should be used in upgrade steps - see here and here.

[larssandergreen]

OK, keep it low-level. So it turns out there's actually a readme that specifically covers how to add option values (I've added a link to this in the docs).

But the SQL in there still doesn't deal with the case where the option already exists, so I've done that in SQL as well. If that makes sense, I can update the readme along the lines of this example.

I can still split this — or maybe it's getting there now?

[yashodha]

@demeritcowboy @eileenmcnaughton @larssandergreen I suggest we take this opportunity to add hook to relativeToAbsolute method so that developers can add their own definition of range and what it translates to in an extension without having to add any more options here :

https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Date.php#L1911

[magnolia61]

I think the name 'year before previous fiscal year' is ambiguous. Might be better to call it 'fiscal year before previous fiscal year'. But this might be also a change in core of it is already in there.

[demeritcowboy]

add hook to relativeToAbsolute method so that developers can add their own definition of range and what it translates to in an extension without having to add any more options here

@yashodha It's an interesting idea but seems out-of-scope here. Do you want to make a lab ticket?

[larssandergreen]

@magnolia61 Fair enough. I was trying to keep it short, but I agree it is more clear and have updated to "Fiscal year before prior to previous fiscal year".

[yashodha]

@demeritcowboy sure will do. Thanks!

[demeritcowboy]

It seems previous_2.fiscal_year is not consistent with previous_2.year. But it's not because of the PR so is a separate issue, but could lead to confusion. Note the difference in the QILL (2021 vs 2022):

Previous 2 calendar years (between January 1st, 2020 12:00 AM and December 31st, 2021 11:59 PM)

Previous 2 fiscal years (between January 1st, 2020 12:00 AM and December 31st, 2022 11:59 PM)

Will finish testing the rest of this later today.

[demeritcowboy]

Lab ticket for the other issue: https://lab.civicrm.org/dev/core/-/issues/3922

[demeritcowboy]

So the scope of the problems are smaller than I thought, so since upgrade PR's go stale fast and because I've tested this otherwise in terms of its mechanics, I think it's mergeable. Install, upgrade, multilingual all work. Then can look at the additional PR.

[demeritcowboy]

It might be slightly cleaner to squash the commits so have put merge-ready.

[larssandergreen]

Thanks @demeritcowboy. I added the SQL for the upgrade as an example for handling values that might already exist to the readme for SQL upgrades and squashed.

[eileenmcnaughton]

Nice - @larssandergreen can you check the docs https://docs.civicrm.org/user/en/latest/searching/relative-date-formats/ & see if this triggers an update?

@yashodha I think to your point there are 2 things

• options that follow an established patterns (ie those that can be added like this by just adding more option values)
• options that follow a custom pattern that require a hook. At some point we probably need to figure out what this would look like

In terms of adding more options - at some point it makes sense to add them as managed entities in an extension - ie if they seem obscure / overloading the UI. I don't know when we hit that point & will take @demeritcowboy & @larssandergreen judgement that this PR was not that point

[larssandergreen]

@eileenmcnaughton Yes, have already done the edits to add these two options to the docs.

[eileenmcnaughton]

thanks @larssandergreen

[demeritcowboy]

I'm not sure what's different now but I'm getting an error on upgrade where it doesn't like the WHERE, i.e. this part is fine:

SELECT @option_group_id_date_filter, {localize}'{ts escape="sql"}Previous 2 fiscal years{/ts}'{/localize}, 'previous_2.fiscal_year', 'previous_2.fiscal_year', NULL, NULL, 0, (SELECT @max_wt := @max_wt+1), {localize}NULL{/localize}, 0, 0, 1, NULL, NULL, NULL

but it doesn't like the WHERE that comes after, even if I change it to something simple like WHERE 1=1. It says syntax error. Does anyone else get this?

[demeritcowboy]

Might be a mariadb thing. If I add FROM DUAL before the WHERE it's happy. Going to check some more things first.