CheckEnv - Give new installs a grace period before 'Cron Not Running' msg

[colemanw]

Overview

Don't bug site admins with "Cron Not Running" error messages right after install. Instead, give them a day's grace period in which to set it up.

Inspired by discussion at https://lab.civicrm.org/dev/core/-/issues/1863#note_39918

Before

"Cron Not Running" error pops up immediately after installing CiviCRM.

After

The error is suppressed for 24 hrs.

Technical Details

The last cron run is recorded as a status preference. That's a little odd, but I'll go with it.
Now if a cron run isn't recorded, we create one with prefs = 'new' to distinguish it from normal cron runs.
I'm honestly not sure what the 'prefs' column is supposed to be used for; grepping turns up nothing. But it seems to work for this purpose.

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

[colemanw]

@kcristiano I thought you'd appreciate this.

Also, while I was working on this I noticed some oddities in the StatusPreference BAO and cleaned it up: #17801

[kcristiano]

@colemanw Thank You.

Great improvement.

r-run worked as expected:

pop up:

Status screen:

No red, just lists of what to do.

[demeritcowboy]

Just curious why this change? The JobManager still refers to it as 'checkLastCron', so while it's unlikely somebody will change this function name, that would then get out of sync. Should it be a class constant which could then be used in both places?

[colemanw]

@demeritcowboy the are actually the same thing, as the function name is "checkLastCron".

I had previously gotten confused about the relationship between the statusPreference and the function, and so I made this change to un-confuse myself. It's more consistent this way because the statusMessage generated below uses __FUNCTION__ and not the string literal.

[colemanw]

@kcristiano thanks for the review. I've just made one additional change which is to switch from INFO to NOTICE when cron has not been set up. That seems more fitting, and will still not generate a popup warning.

[agh1]

I like the direction of this. However, I am a little concerned that this creates a statusPreference, since if someone wanted to pre-hush the cron check (i.e. they know they'll never have cron running and they want to be sure the message doesn't pop up) they'd insert a statusPreference right after install. I think this would undo their work.

[colemanw]

@agh1 this confused me at first too, but creating a statusPreference is actually not new to this PR.

The way the system currently works, a statusPreference with the name "checkLastCron" is created every time cron runs (and if one already exists, it gets updated) and it stores the current timestamp.

So all this PR does in addition to that is create (or update) a statusPreference with the current timestamp and the designation "new" in its prefs field if a timestamp isn't found. The scenario you described where the install script creates one would work fine. This status check would update it with the "new" designation and everything would work normally from there.

[agh1]

One thought is to just make the message a "notice" if no cron has ever run and no scheduled jobs (besides version_check) are enabled. We can tell that they won't be missing out on much, so the severity is no longer "error: your mail is stuck and membership are eternal" but rather "notice: you haven't enabled something most people want".

[agh1]

@colemanw gotcha--I haven't looked at this in years and I think that used to be stored in a setting.

[agh1]

Rereading this my only thought is that the message during that grace period is a "notice", which is appropriate but means that nobody will see it unless they open up the status page. We set up this check in the first place to replace a more ad hoc popup that appeared upon install. My concern is that we will really need to emphasize in some other way that an admin should configure cron.

Otherwise, we're just deferring the shock and confusion of seeing "cron not running" until a day later.

[colemanw]

@agh1 ok I've downgraded the message from ERROR to WARNING in cases where cron has not ever run.

[agh1]

@colemanw yeah I think that's fair: it's more severe if you thought things were running than if you never thought it would.

Re: my prior comment about increasing visibility, I think a natural thing to do would be to have a message on the scheduled jobs page if cron isn't running, similar to what's there in a non-production environment:

civicrm-core/CRM/Admin/Page/Job.php

Lines 142 to 144 in 64aa560

	if (CRM_Core_Config::environment() != 'Production') {
	CRM_Core_Session::setStatus(ts('Execution of scheduled jobs has been turned off by default since this is a non-production environment. You can override this for particular jobs by adding runInNonProductionEnvironment=TRUE as a parameter.'), ts("Non-production Environment"), "warning", array('expires' => 0));
	}

I'd write it now, but I'd end up duplicating a lot of the code here in terms of logic. Do you think it would make sense to abstract out some of this code so it's available as a sanity check elsewhere in CiviCRM?

[kcristiano]

@agh1 Regarding

However, I am a little concerned that this creates a statusPreference, since if someone wanted to pre-hush the cron check (i.e. they know they'll never have cron running and they want to be sure the message doesn't pop up) they'd insert a statusPreference right after install. I think this would undo their work.

I tested using this snippet:

wp civicrm api StatusPreference.create ignore_severity=critical name=checkLastCron

This hides the cron notice after the patch is applied.

I agree adding a notice to the scheduled jobs page would be a big improvement.

[eileenmcnaughton]

@agh1 are your concerns address (ie do you consider this mergeable now)

[agh1]

@eileenmcnaughton Yeah I think this is good to merge. In reading the code again I think there's not too much that would be duplicated in producing a warning for the scheduled jobs page, so you can ignore that last question in my previous comment.

[eileenmcnaughton]

OK - I read @kcristiano as being in favour too - thanks

[agh1]

See #17819 which adds a message at the top of the Scheduled Jobs page.