localgovdrupal / localgov_workflows

Default editorial workflow for LocalGov Drupal content.
GNU General Public License v2.0
0 stars 1 forks source link

[Workflow notifications] Notifications not processed after clean install #92

Closed graham-web closed 1 month ago

graham-web commented 1 month ago

Testing a clean install of the localgov_workflows_notifications module, we noticed that the hook_cron logic to enqueue entities won't ever run, due to the state variable localgov_workflows_notifications.last_email_run not having been initialised.

In the below code, the default value (current request time) will always be before the calculated "next run" time, making the if-condition fail:

 $request_time = \Drupal::time()->getRequestTime();
  $last_run = \Drupal::state()->get('localgov_workflows_notifications.last_email_run', $request_time);
  $next_run = $last_run + 86400 * $settings->get('email_frequency');
  if ($request_time >= $next_run) {

Resolutions

90 suggests that the state would be initialised to the install time of the module - should it do this?

Else, amending the if condition to check for a default value would fix the issue. e.g.:

if ($request_time >= $next_run || $request_time == $last_run) {

stephen-cox commented 1 month ago

Hi @graham-web, thanks for reporting this rather serious bug.

I had initially just set the $last_run to 0 if it wasn't set, but this triggers alerts for all content that is currently in needs review.

Initializing last_email_run when the module is enabled to the current time makes sense for now, although this might change with #90. I'll also add an update hook to set it for current installs.

stephen-cox commented 1 month ago

This should be fixed with #93