Problem/Motivation

The configuration option to notify users by role doesn't work for the 'authenticated user' role. This because users can't be fetched with the current lookup implementation as the 'authenticated user' role is an implicit role:

// Roles.
foreach ($notification->getRoleIds() as $role) {
  /** @var \Drupal\user\UserInterface[] $role_users */
  $role_users = $this->entityTypeManager
    ->getStorage('user')
    ->loadByProperties(['roles' => $role]);
  foreach ($role_users as $role_user) {
    $data['to'][] = $role_user->getEmail();
  }
}

Proposed resolution

Either discard the option in the configuration form or alter the lookup to query the authenticated users as recipients.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Baik Ho created an issue. See original summary.

Rob Holmes’s picture

Nice catch, thanks. I think we should support the 'Authenticated user option' so we need to add some extra code to handle this situation.

baikho’s picture

Status: Active » Needs review
FileSize
1.44 KB

See added patch to allow for authenticated user processing.

jhedstrom’s picture

Status: Needs review » Needs work
+++ b/src/Notification.php
@@ -131,10 +132,17 @@ class Notification implements NotificationInterface {
+          $uids = array_filter(\Drupal::entityQuery('user')->execute());

We should filter out blocked users here I think (->condition('status', 1). Also, I think we want to turn off access checking on the entity query since this could be triggered by users that don't have access to view other users (->accessCheck(FALSE)). Otherwise this looks great!

baikho’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
1.3 KB

Thanks for the review @jhedstrom, see updated patch with addressed remarks.

  • jhedstrom committed 362217c on 8.x-3.x authored by baikho
    Issue #3035410 by baikho: Notify 'authenticated user' role is not...
jhedstrom’s picture

Status: Needs review » Fixed

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.