Send review notifications to service contacts

[stephen-cox]

Adds a service contact entity that can be associated with nodes and sends notifications to service contact when content passes it's needs review date.

Fixes #12

[stephen-cox]

To test this PR:

Install and enable the localgov_workflows_notifications module:

composer require localgovdrupal/localgov_workflows:"dev-feature/1.x/12-review-emails as 1.2.3"
drush en localgov_workflows_notifications localgov_review_date

Create some service contacts at /admin/content/localgov-service-contact and then associate them with a node.

On the node, check the 'content reviewed' box and set the review date to yesterday.

Then run:

drush sset localgov_workflows_notifications.last_email_run 0
drush cron

[finnlewis]

@Adnan-cds is half way through reviewing this.... others encouraged to help review.

[Adnan-cds]

Results from testing so far:

• None of the user roles has any of the Service contact management permissions. Perhaps the "Editor" and "User manager" role should be given some permissions.
• I was adding a service contact at /admin/content/localgov-service-contact/add. It either accepts a Drupal user or an external user. The UX would be better if there were a visual clue to indicate it is either a Drupal user or an external user.
• Under Claro, the Service contact widget is overstretched:
• It would be good if the helptext of the Service contact widget in node edit forms briefly explains what it is for.
• After adding a Service contact to an unpublished Guide page and then setting its review date to yesterday, drush cron suffered a PHP error :(

  [21-Feb-2024 16:44:38 Europe/London] TypeError: Drupal\localgov_review_date\Entity\ReviewDate::getEntity(): Return value must be of type Drupal\Core\Entity\EntityInterface, null returned in /path/to/codebase/web/modules/contrib/localgov_workflows/modules/localgov_review_date/src/Entity/ReviewDate.php on line 158 #0 /path/to/codebase/web/modules/contrib/localgov_workflows/modules/localgov_workflows_notifications/localgov_workflows_notifications.module(144): Drupal\localgov_review_date\Entity\ReviewDate->getEntity()
  #1 /path/to/codebase/web/core/lib/Drupal/Core/Cron.php(335): localgov_workflows_notifications_cron()
  #2 /path/to/codebase/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(388): Drupal\Core\Cron->Drupal\Core\{closure}(Object(Closure), 'localgov_workfl...')
  #3 /path/to/codebase/web/core/lib/Drupal/Core/Cron.php(318): Drupal\Core\Extension\ModuleHandler->invokeAllWith('cron', Object(Closure))
  #4 /path/to/codebase/web/core/lib/Drupal/Core/Cron.php(159): Drupal\Core\Cron->invokeCronHandlers()
  #5 /path/to/codebase/web/core/lib/Drupal/Core/ProxyClass/Cron.php(75): Drupal\Core\Cron->run()
  #6 /path/to/codebase/vendor/drush/drush/src/Commands/core/DrupalCommands.php(63): Drupal\Core\ProxyClass\Cron->run()
  #7 [internal function]: Drush\Commands\core\DrupalCommands->cron(Array)
  #8 /path/to/codebase/vendor/consolidation/annotated-command/src/CommandProcessor.php(276): call_user_func_array(Array, Array)
  #9 /path/to/codebase/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
  #10 /path/to/codebase/vendor/consolidation/annotated-command/src/CommandProcessor.php(175): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
  #11 /path/to/codebase/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(387): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
  #12 /path/to/codebase/vendor/symfony/console/Command/Command.php(326): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
  #13 /path/to/codebase/vendor/symfony/console/Application.php(1081): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
  #14 /path/to/codebase/vendor/symfony/console/Application.php(320): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
  #15 /path/to/codebase/vendor/symfony/console/Application.php(174): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
  #16 /path/to/codebase/vendor/drush/drush/src/Runtime/Runtime.php(110): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
  #17 /path/to/codebase/vendor/drush/drush/src/Runtime/Runtime.php(40): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
  #18 /path/to/codebase/vendor/drush/drush/drush.php(139): Drush\Runtime\Runtime->run(Array)
  #19 /path/to/codebase/vendor/drush/drush/drush(4): require('/path/to/codebase/v...')
  #20 /path/to/codebase/bin/drush(120): include('/path/to/codebase/v...')
  #21 {main}
  TypeError: Drupal\localgov_review_date\Entity\ReviewDate::getEntity(): Return value must be of type Drupal\Core\Entity\EntityInterface, null returned in Drupal\localgov_review_date\Entity\ReviewDate->getEntity() (line 158 of /path/to/codebase/web/modules/contrib/localgov_workflows/modules/localgov_review_date/src/Entity/ReviewDate.php).

[stephen-cox]

Thanks for looking @Adnan-cds

I would have just used Drupal's user accounts as Service contacts instead of adding a new Entity type

This came out of the discussion early on. Some councils have all their users in the site, some don't. This should cover both setups.

None of the user roles has any of the Service contact management permissions. Perhaps the "Editor" and "User manager" role should be given some permissions.

Good catch. Will add permissions. Certainly to the editor role. Not sure about the user manager, as this user currently only has permissions to create users with certain roles.

Will need an install hook to add the permissions to editors.

The UX would be better if there were a visual clue to indicate it is either a Drupal user or an external user.

Do you have anything in mind for this? I have added some text to explain it and disabled the other fields once text is added to one field, but not sure what else can be added to make this clear.

I'll address the bugs this morning.

[stephen-cox]

@Adnan-cds

I have added a hook to add permissions to view and manage service contacts. Editors have permissions to manage them. Authors and contributors have permissions to view them so they can be added and removed from content.

Under Claro, the Service contact widget is overstretched

I'm not seeing this on a fresh LGD install. Can you provide more information on your install?

It would be good if the helptext of the Service contact widget in node edit forms briefly explains what it is for.

I've updated the message to 'Associate service contacts with this content by searching by name or email.'

After adding a Service contact to an unpublished Guide page and then setting its review date to yesterday, drush cron suffered a PHP error :(

Unfortunately, I'm not seeing this either with a fresh install and the demo module. I assume you're testing this on an existing site.

The error message indicates an issue with the Review Date entity. If there's no entity associated with the review date then the getEntity command fails as it returns NULL. As we're checking for this anyway, I have updated the code to allow NULL returns, which should fix the error you're seeing.

Can you test again with the latest changes?

[Adnan-cds]

I have added a hook to add permissions to view and manage service contacts. Editors have permissions to manage them. Authors and contributors have permissions to view them so they can be added and removed from content.

Thanks. Looks good.

Under Claro, the Service contact widget is overstretched I'm not seeing this on a fresh LGD install. Can you provide more information on your install?

This is no longer happening. Thanks again :)

It would be good if the helptext of the Service contact widget in node edit forms briefly explains what it is for. I've updated the message to 'Associate service contacts with this content by searching by name or email.'

Thanks. It still doesn't fully hint at what these Service contacts are for. Adding another sentence such as "They will receive review reminders" may bring more clarity.

After adding a Service contact to an unpublished Guide page and then setting its review date to yesterday, drush cron suffered a PHP error :( The error message indicates an issue with the Review Date entity. If there's no entity associated with the review date then the getEntity command fails as it returns NULL. As we're checking for this anyway, I have updated the code to allow NULL returns, which should fix the error you're seeing.

No fatal error any more. Thanks a lot :)

Can you test again with the latest changes?

It works!

[Adnan-cds]

The UX would be better if there were a visual clue to indicate it is either a Drupal user or an external user. Do you have anything in mind for this?

Something like the following would be good. But this is not a must-have. Once the question of whether or not to bundle Symfony mailer with this change is resolved, I am happy for it to be merged.

[finnlewis]

Sounds like https://www.drupal.org/project/symfony_mailer is the way... and allows people full flexibility over how they configure the transport.

I will test and have a think reflecting on the comments above.

Questions:

1. do we force people to use Symfony Mailer?
2. If so, do we include it as a part of the localgov profile or localgov_core for consistency?
3. Do we write some code to accommodate people who are using other emailing solutions?

[finnlewis]

Worth testing with

• https://www.drupal.org/project/smtp
• https://www.drupal.org/project/mailsystem

[Adnan-cds]

Worth testing with

Installed the new localgov_workflows_notifications submodule on an existing site that's been using the smtp module and symfony_mailer has indeed taken over from the smtp module.

On the upside, the Webform module whitelists symfony_mailer for sending out attachments. So there should not be any loss of functionality for Webforms at least.

[finnlewis]

Can we add a check / warning with

• hook_install
• hook_requirements
• incompatible modules ?

Such that: if any of these modules are installed: (smtp / mailsystem / phpmailer) - warn the user that we will install symfony_mailer and we are taking over.

Or, notify the user (developer) on install, regarding of the other modules installed?

[stephen-cox]

@finnlewis @Adnan-cds There's now a warning message if enabling this and a conflicting module is already enabled:

https://github.com/localgovdrupal/localgov_workflows/blob/f7b720b067db7576f8c43ee0fc1743c422573cd6/modules/localgov_workflows_notifications/localgov_workflows_notifications.install#L13-L25

And a warning added to the top of the README

https://github.com/localgovdrupal/localgov_workflows/blob/e6d90c1413e2ddc443ef0a9a07de03c5453c050b/modules/localgov_workflows_notifications/README.md?plain=1#L7-L17

Do you think this sufficient warning about the potential email changes?

[Adnan-cds]

Do you think this sufficient warning about the potential email changes?

I can live with this. Thanks for this. I will test again and see how it goes.