From 7dea15886e317487e835dcec4727a368a357b8fc Mon Sep 17 00:00:00 2001 From: rahul Date: Fri, 5 Apr 2024 18:41:44 +0530 Subject: [PATCH 1/5] Added a new option in workflow notificationSettings to ensure that notifications are also sent to inactive users --- .../src/DependencyInjection/Configuration.php | 4 ++++ .../EventSubscriber/NotificationSubscriber.php | 15 +++++++++------ .../Notification/AbstractNotificationService.php | 8 ++++---- .../Notification/NotificationEmailService.php | 4 ++-- .../Notification/PimcoreNotificationService.php | 4 ++-- 5 files changed, 21 insertions(+), 14 deletions(-) diff --git a/bundles/CoreBundle/src/DependencyInjection/Configuration.php b/bundles/CoreBundle/src/DependencyInjection/Configuration.php index 507c5ce3a90..1b476c95f8d 100644 --- a/bundles/CoreBundle/src/DependencyInjection/Configuration.php +++ b/bundles/CoreBundle/src/DependencyInjection/Configuration.php @@ -1614,6 +1614,10 @@ private function addWorkflowNode(ArrayNodeDefinition $rootNode): void ->defaultValue(NotificationSubscriber::DEFAULT_MAIL_TEMPLATE_PATH) ->info('Path to mail source - either Symfony path to template or fullpath to Pimcore document. Optional use ' . NotificationEmailService::MAIL_PATH_LANGUAGE_PLACEHOLDER . ' as placeholder for language.') ->end() + ->booleanNode('includeAllUsers') + ->defaultFalse() + ->info('Set this to true to ensure that notifications are also sent to inactive users') + ->end() ->end() ->end() ->end() diff --git a/lib/Workflow/EventSubscriber/NotificationSubscriber.php b/lib/Workflow/EventSubscriber/NotificationSubscriber.php index abac5ba4c1a..8ff409c4ad4 100644 --- a/lib/Workflow/EventSubscriber/NotificationSubscriber.php +++ b/lib/Workflow/EventSubscriber/NotificationSubscriber.php @@ -81,19 +81,20 @@ public function onWorkflowCompleted(Event $event): void if (empty($condition) || $this->expressionService->evaluateExpression($workflow, $subject, $condition)) { $notifyUsers = $notificationSetting['notifyUsers'] ?? []; $notifyRoles = $notificationSetting['notifyRoles'] ?? []; + $includeAllUsers = $notificationSetting['includeAllUsers'] ?? false; if (in_array(self::NOTIFICATION_CHANNEL_MAIL, $notificationSetting['channelType'])) { - $this->handleNotifyPostWorkflowEmail($transition, $workflow, $subject, $notificationSetting['mailType'], $notificationSetting['mailPath'], $notifyUsers, $notifyRoles); + $this->handleNotifyPostWorkflowEmail($transition, $workflow, $subject, $notificationSetting['mailType'], $notificationSetting['mailPath'], $notifyUsers, $notifyRoles, $includeAllUsers); } if (in_array(self::NOTIFICATION_CHANNEL_PIMCORE_NOTIFICATION, $notificationSetting['channelType'])) { - $this->handleNotifyPostWorkflowPimcoreNotification($transition, $workflow, $subject, $notifyUsers, $notifyRoles); + $this->handleNotifyPostWorkflowPimcoreNotification($transition, $workflow, $subject, $notifyUsers, $notifyRoles, $includeAllUsers); } } } } - private function handleNotifyPostWorkflowEmail(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, string $mailType, string $mailPath, array $notifyUsers, array $notifyRoles): void + private function handleNotifyPostWorkflowEmail(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, string $mailType, string $mailPath, array $notifyUsers, array $notifyRoles, bool $includeAllUsers): void { //notify users $subjectType = ($subject instanceof Concrete ? $subject->getClassName() : Service::getElementType($subject)); @@ -106,11 +107,12 @@ private function handleNotifyPostWorkflowEmail(Transition $transition, \Symfony\ $subject, $transition->getLabel(), $mailType, - $mailPath + $mailPath, + $includeAllUsers ); } - private function handleNotifyPostWorkflowPimcoreNotification(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, array $notifyUsers, array $notifyRoles): void + private function handleNotifyPostWorkflowPimcoreNotification(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, array $notifyUsers, array $notifyRoles, bool $includeAllUsers): void { $subjectType = ($subject instanceof Concrete ? $subject->getClassName() : Service::getElementType($subject)); $this->pimcoreNotificationService->sendPimcoreNotification( @@ -119,7 +121,8 @@ private function handleNotifyPostWorkflowPimcoreNotification(Transition $transit $workflow, $subjectType, $subject, - $transition->getLabel() + $transition->getLabel(), + $includeAllUsers ); } diff --git a/lib/Workflow/Notification/AbstractNotificationService.php b/lib/Workflow/Notification/AbstractNotificationService.php index 5cd93d0643c..f935861f18f 100644 --- a/lib/Workflow/Notification/AbstractNotificationService.php +++ b/lib/Workflow/Notification/AbstractNotificationService.php @@ -46,7 +46,7 @@ protected function getNoteInfo(int $id): string * * @return User[][] */ - protected function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers = false): array + protected function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers): array { $notifyUsers = []; @@ -57,7 +57,7 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ foreach ($roleList->load() as $role) { $userList = new User\Listing(); - $userList->setCondition('FIND_IN_SET(?, roles) > 0', [$role->getId()]); + $userList->setCondition('FIND_IN_SET(?, roles) > 0 AND active = ?', [$role->getId(), $includeAllUsers]); foreach ($userList->load() as $user) { if ($includeAllUsers || $user->getEmail()) { @@ -71,10 +71,10 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ //get users $userList = new User\Listing(); if ($includeAllUsers) { - $userList->setCondition('name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).')'); + $userList->setCondition('name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') AND active = ?', [$includeAllUsers]); } else { $userList->setCondition( - 'name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and email is not null' + 'name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and email is not null AND active = ?', [$includeAllUsers] ); } diff --git a/lib/Workflow/Notification/NotificationEmailService.php b/lib/Workflow/Notification/NotificationEmailService.php index 1ce2ffc437e..f7b1b988c08 100644 --- a/lib/Workflow/Notification/NotificationEmailService.php +++ b/lib/Workflow/Notification/NotificationEmailService.php @@ -48,10 +48,10 @@ public function __construct(EngineInterface $template, RouterInterface $router, * Sends an Mail * */ - public function sendWorkflowEmailNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action, string $mailType, string $mailPath): void + public function sendWorkflowEmailNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action, string $mailType, string $mailPath, bool $includeAllUsers): void { try { - $recipients = $this->getNotificationUsersByName($users, $roles); + $recipients = $this->getNotificationUsersByName($users, $roles, $includeAllUsers); if (!count($recipients)) { return; } diff --git a/lib/Workflow/Notification/PimcoreNotificationService.php b/lib/Workflow/Notification/PimcoreNotificationService.php index a7a7093a989..ea6eca413eb 100644 --- a/lib/Workflow/Notification/PimcoreNotificationService.php +++ b/lib/Workflow/Notification/PimcoreNotificationService.php @@ -37,10 +37,10 @@ public function __construct(NotificationService $notificationService, Translator $this->translator = $translator; } - public function sendPimcoreNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action): void + public function sendPimcoreNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action, bool $includeAllUsers): void { try { - $recipients = $this->getNotificationUsersByName($users, $roles, true); + $recipients = $this->getNotificationUsersByName($users, $roles, $includeAllUsers); if (!count($recipients)) { return; } From d7950af16f64b74158830fba5d3c91ee394fe106 Mon Sep 17 00:00:00 2001 From: rahul Date: Fri, 5 Apr 2024 19:03:51 +0530 Subject: [PATCH 2/5] Condition improved --- .../Notification/AbstractNotificationService.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/Workflow/Notification/AbstractNotificationService.php b/lib/Workflow/Notification/AbstractNotificationService.php index f935861f18f..e531349edda 100644 --- a/lib/Workflow/Notification/AbstractNotificationService.php +++ b/lib/Workflow/Notification/AbstractNotificationService.php @@ -57,7 +57,12 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ foreach ($roleList->load() as $role) { $userList = new User\Listing(); - $userList->setCondition('FIND_IN_SET(?, roles) > 0 AND active = ?', [$role->getId(), $includeAllUsers]); + + if ($includeAllUsers) { + $userList->setCondition('FIND_IN_SET(?, roles) > 0', [$role->getId()]); + } else { + $userList->setCondition('FIND_IN_SET(?, roles) > 0 and email is not null AND active = ?', [$role->getId()]); + } foreach ($userList->load() as $user) { if ($includeAllUsers || $user->getEmail()) { @@ -71,7 +76,7 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ //get users $userList = new User\Listing(); if ($includeAllUsers) { - $userList->setCondition('name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') AND active = ?', [$includeAllUsers]); + $userList->setCondition('name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).')', [$includeAllUsers]); } else { $userList->setCondition( 'name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and email is not null AND active = ?', [$includeAllUsers] From 8b5d6c68d055eaf7145ff61af361d8a8d678130c Mon Sep 17 00:00:00 2001 From: rahul Date: Tue, 4 Jun 2024 17:13:23 +0530 Subject: [PATCH 3/5] worked on feedback point : removed the node of includeAllUsers configuration node --- .../src/DependencyInjection/Configuration.php | 4 --- .../NotificationSubscriber.php | 15 ++++------ .../AbstractNotificationService.php | 28 +++++-------------- .../Notification/NotificationEmailService.php | 4 +-- .../PimcoreNotificationService.php | 4 +-- 5 files changed, 17 insertions(+), 38 deletions(-) diff --git a/bundles/CoreBundle/src/DependencyInjection/Configuration.php b/bundles/CoreBundle/src/DependencyInjection/Configuration.php index 1b476c95f8d..507c5ce3a90 100644 --- a/bundles/CoreBundle/src/DependencyInjection/Configuration.php +++ b/bundles/CoreBundle/src/DependencyInjection/Configuration.php @@ -1614,10 +1614,6 @@ private function addWorkflowNode(ArrayNodeDefinition $rootNode): void ->defaultValue(NotificationSubscriber::DEFAULT_MAIL_TEMPLATE_PATH) ->info('Path to mail source - either Symfony path to template or fullpath to Pimcore document. Optional use ' . NotificationEmailService::MAIL_PATH_LANGUAGE_PLACEHOLDER . ' as placeholder for language.') ->end() - ->booleanNode('includeAllUsers') - ->defaultFalse() - ->info('Set this to true to ensure that notifications are also sent to inactive users') - ->end() ->end() ->end() ->end() diff --git a/lib/Workflow/EventSubscriber/NotificationSubscriber.php b/lib/Workflow/EventSubscriber/NotificationSubscriber.php index 8ff409c4ad4..abac5ba4c1a 100644 --- a/lib/Workflow/EventSubscriber/NotificationSubscriber.php +++ b/lib/Workflow/EventSubscriber/NotificationSubscriber.php @@ -81,20 +81,19 @@ public function onWorkflowCompleted(Event $event): void if (empty($condition) || $this->expressionService->evaluateExpression($workflow, $subject, $condition)) { $notifyUsers = $notificationSetting['notifyUsers'] ?? []; $notifyRoles = $notificationSetting['notifyRoles'] ?? []; - $includeAllUsers = $notificationSetting['includeAllUsers'] ?? false; if (in_array(self::NOTIFICATION_CHANNEL_MAIL, $notificationSetting['channelType'])) { - $this->handleNotifyPostWorkflowEmail($transition, $workflow, $subject, $notificationSetting['mailType'], $notificationSetting['mailPath'], $notifyUsers, $notifyRoles, $includeAllUsers); + $this->handleNotifyPostWorkflowEmail($transition, $workflow, $subject, $notificationSetting['mailType'], $notificationSetting['mailPath'], $notifyUsers, $notifyRoles); } if (in_array(self::NOTIFICATION_CHANNEL_PIMCORE_NOTIFICATION, $notificationSetting['channelType'])) { - $this->handleNotifyPostWorkflowPimcoreNotification($transition, $workflow, $subject, $notifyUsers, $notifyRoles, $includeAllUsers); + $this->handleNotifyPostWorkflowPimcoreNotification($transition, $workflow, $subject, $notifyUsers, $notifyRoles); } } } } - private function handleNotifyPostWorkflowEmail(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, string $mailType, string $mailPath, array $notifyUsers, array $notifyRoles, bool $includeAllUsers): void + private function handleNotifyPostWorkflowEmail(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, string $mailType, string $mailPath, array $notifyUsers, array $notifyRoles): void { //notify users $subjectType = ($subject instanceof Concrete ? $subject->getClassName() : Service::getElementType($subject)); @@ -107,12 +106,11 @@ private function handleNotifyPostWorkflowEmail(Transition $transition, \Symfony\ $subject, $transition->getLabel(), $mailType, - $mailPath, - $includeAllUsers + $mailPath ); } - private function handleNotifyPostWorkflowPimcoreNotification(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, array $notifyUsers, array $notifyRoles, bool $includeAllUsers): void + private function handleNotifyPostWorkflowPimcoreNotification(Transition $transition, \Symfony\Component\Workflow\Workflow $workflow, ElementInterface $subject, array $notifyUsers, array $notifyRoles): void { $subjectType = ($subject instanceof Concrete ? $subject->getClassName() : Service::getElementType($subject)); $this->pimcoreNotificationService->sendPimcoreNotification( @@ -121,8 +119,7 @@ private function handleNotifyPostWorkflowPimcoreNotification(Transition $transit $workflow, $subjectType, $subject, - $transition->getLabel(), - $includeAllUsers + $transition->getLabel() ); } diff --git a/lib/Workflow/Notification/AbstractNotificationService.php b/lib/Workflow/Notification/AbstractNotificationService.php index e531349edda..61258f16cbf 100644 --- a/lib/Workflow/Notification/AbstractNotificationService.php +++ b/lib/Workflow/Notification/AbstractNotificationService.php @@ -46,7 +46,7 @@ protected function getNoteInfo(int $id): string * * @return User[][] */ - protected function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers): array + protected function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers = true): array { $notifyUsers = []; @@ -57,17 +57,9 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ foreach ($roleList->load() as $role) { $userList = new User\Listing(); - - if ($includeAllUsers) { - $userList->setCondition('FIND_IN_SET(?, roles) > 0', [$role->getId()]); - } else { - $userList->setCondition('FIND_IN_SET(?, roles) > 0 and email is not null AND active = ?', [$role->getId()]); - } - + $userList->setCondition('FIND_IN_SET(?, roles) > 0 and email is not null AND active = ?', [$role->getId(), $includeAllUsers]); foreach ($userList->load() as $user) { - if ($includeAllUsers || $user->getEmail()) { - $notifyUsers[$user->getLanguage()][$user->getId()] = $user; - } + $notifyUsers[$user->getLanguage()][$user->getId()] = $user; } } } @@ -75,18 +67,12 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ if ($users) { //get users $userList = new User\Listing(); - if ($includeAllUsers) { - $userList->setCondition('name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).')', [$includeAllUsers]); - } else { - $userList->setCondition( - 'name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and email is not null AND active = ?', [$includeAllUsers] - ); - } + $userList->setCondition( + 'name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and email is not null AND active = ?', [$includeAllUsers] + ); foreach ($userList->load() as $user) { - if ($includeAllUsers || $user->getEmail()) { - $notifyUsers[$user->getLanguage()][$user->getId()] = $user; - } + $notifyUsers[$user->getLanguage()][$user->getId()] = $user; } } diff --git a/lib/Workflow/Notification/NotificationEmailService.php b/lib/Workflow/Notification/NotificationEmailService.php index f7b1b988c08..1ce2ffc437e 100644 --- a/lib/Workflow/Notification/NotificationEmailService.php +++ b/lib/Workflow/Notification/NotificationEmailService.php @@ -48,10 +48,10 @@ public function __construct(EngineInterface $template, RouterInterface $router, * Sends an Mail * */ - public function sendWorkflowEmailNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action, string $mailType, string $mailPath, bool $includeAllUsers): void + public function sendWorkflowEmailNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action, string $mailType, string $mailPath): void { try { - $recipients = $this->getNotificationUsersByName($users, $roles, $includeAllUsers); + $recipients = $this->getNotificationUsersByName($users, $roles); if (!count($recipients)) { return; } diff --git a/lib/Workflow/Notification/PimcoreNotificationService.php b/lib/Workflow/Notification/PimcoreNotificationService.php index ea6eca413eb..16d9b693825 100644 --- a/lib/Workflow/Notification/PimcoreNotificationService.php +++ b/lib/Workflow/Notification/PimcoreNotificationService.php @@ -37,10 +37,10 @@ public function __construct(NotificationService $notificationService, Translator $this->translator = $translator; } - public function sendPimcoreNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action, bool $includeAllUsers): void + public function sendPimcoreNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action): void { try { - $recipients = $this->getNotificationUsersByName($users, $roles, $includeAllUsers); + $recipients = $this->getNotificationUsersByName($users, $roles); if (!count($recipients)) { return; } From a60a8b2c11a867a3e7206624ea9407105b02c522 Mon Sep 17 00:00:00 2001 From: markus-moser Date: Fri, 16 Aug 2024 10:10:02 +0200 Subject: [PATCH 4/5] Improve empty email check and send pimcore notifications to users without email --- .../AbstractNotificationService.php | 19 +++++++++++++------ .../PimcoreNotificationService.php | 2 +- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/Workflow/Notification/AbstractNotificationService.php b/lib/Workflow/Notification/AbstractNotificationService.php index 61258f16cbf..7047cff0ad6 100644 --- a/lib/Workflow/Notification/AbstractNotificationService.php +++ b/lib/Workflow/Notification/AbstractNotificationService.php @@ -46,7 +46,7 @@ protected function getNoteInfo(int $id): string * * @return User[][] */ - protected function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers = true): array + public function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers = false): array { $notifyUsers = []; @@ -57,7 +57,12 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ foreach ($roleList->load() as $role) { $userList = new User\Listing(); - $userList->setCondition('FIND_IN_SET(?, roles) > 0 and email is not null AND active = ?', [$role->getId(), $includeAllUsers]); + $userList->setCondition('FIND_IN_SET(?, roles) > 0 AND active = 1', [$role->getId()]); + + if (!$includeAllUsers) { + $userList->addConditionParam('(email IS NOT NULL AND email != "")'); + } + foreach ($userList->load() as $user) { $notifyUsers[$user->getLanguage()][$user->getId()] = $user; } @@ -67,9 +72,11 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ if ($users) { //get users $userList = new User\Listing(); - $userList->setCondition( - 'name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and email is not null AND active = ?', [$includeAllUsers] - ); + $userList->setCondition('name IN ('.implode(',', array_map([Db::get(), 'quote'], $users)).') and active = 1'); + + if (!$includeAllUsers) { + $userList->addConditionParam('(email IS NOT NULL AND email != "")'); + } foreach ($userList->load() as $user) { $notifyUsers[$user->getLanguage()][$user->getId()] = $user; @@ -77,7 +84,7 @@ protected function getNotificationUsersByName(array $users, array $roles, bool $ } foreach ($notifyUsers as $language => $usersPerLanguage) { - $notifyUsers[$language] = array_values($notifyUsers[$language]); + $notifyUsers[$language] = array_values($usersPerLanguage); } return $notifyUsers; diff --git a/lib/Workflow/Notification/PimcoreNotificationService.php b/lib/Workflow/Notification/PimcoreNotificationService.php index 16d9b693825..a7a7093a989 100644 --- a/lib/Workflow/Notification/PimcoreNotificationService.php +++ b/lib/Workflow/Notification/PimcoreNotificationService.php @@ -40,7 +40,7 @@ public function __construct(NotificationService $notificationService, Translator public function sendPimcoreNotification(array $users, array $roles, Workflow $workflow, string $subjectType, ElementInterface $subject, string $action): void { try { - $recipients = $this->getNotificationUsersByName($users, $roles); + $recipients = $this->getNotificationUsersByName($users, $roles, true); if (!count($recipients)) { return; } From 34955e491604c522ccc9fe6d26a80606933e1ca1 Mon Sep 17 00:00:00 2001 From: markus-moser Date: Fri, 16 Aug 2024 10:10:45 +0200 Subject: [PATCH 5/5] Revert debugging change --- lib/Workflow/Notification/AbstractNotificationService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Workflow/Notification/AbstractNotificationService.php b/lib/Workflow/Notification/AbstractNotificationService.php index 7047cff0ad6..95e562427e9 100644 --- a/lib/Workflow/Notification/AbstractNotificationService.php +++ b/lib/Workflow/Notification/AbstractNotificationService.php @@ -46,7 +46,7 @@ protected function getNoteInfo(int $id): string * * @return User[][] */ - public function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers = false): array + protected function getNotificationUsersByName(array $users, array $roles, bool $includeAllUsers = false): array { $notifyUsers = [];