search

localgovdrupal / localgov_workflows

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

Ensure last_email_run gets initialized #93

Closed stephen-cox closed 1 month ago

stephen-cox commented 1 month ago

Fixes #92

graham-web commented 1 month ago

Minor thought... shouldn't it also reset the last_email_run state when the "Enable email notifications" box has become ticked? Else the user would receive a flood of old notifications as soon as they [re-]enable e-mails!

Or perhaps simpler, for the same effect, just update state every time within hook_cron during the early-return?

ekes commented 1 month ago

Else the user would receive a flood of old notifications as soon as they [re-]enable e-mails!

Reading the code what gets sent by the module is base on querying using the REQUEST_TIME https://github.com/localgovdrupal/localgov_workflows/blob/94dad592dd71b1496db7f1fa585c15cbf752c610/modules/localgov_workflows_notifications/localgov_workflows_notifications.module#L133 and https://github.com/localgovdrupal/localgov_workflows/blob/94dad592dd71b1496db7f1fa585c15cbf752c610/modules/localgov_workflows_notifications/localgov_workflows_notifications.module#L143 iff the run should happen based on the last run value https://github.com/localgovdrupal/localgov_workflows/blob/94dad592dd71b1496db7f1fa585c15cbf752c610/modules/localgov_workflows_notifications/localgov_workflows_notifications.module#L136

So the number of mails should be the same no matter what?

Ahh https://github.com/localgovdrupal/localgov_workflows/blob/94dad592dd71b1496db7f1fa585c15cbf752c610/modules/localgov_workflows_notifications/localgov_workflows_notifications.module#L142

OK I'd say another issue for that.

stephen-cox commented 1 month ago

It's probably a good idea to reset the last_email_run state var when turning email sending on.

This would bring in quite a bit of duplication of code when resetting the the var, so I'm thinking of abstracting this into a service to make it easier to test.

Given this is quite critical, I suggest merging this and creating a new issue for reseting the var when enabling email sending.

finnlewis commented 1 month ago

Discussed at Merge Tuesday. Thanks for your comment @graham-web - we've created a separate issue to deal with that. https://github.com/localgovdrupal/localgov_workflows/issues/94

Otherwise, we're happy to get this out to councils who are testing.