From a8b602279553aff77c6e41c3afb4fe39de294701 Mon Sep 17 00:00:00 2001
From: DeckChairs <DeckChairs@users.noreply.github.com>
Date: Tue, 16 Jul 2019 14:40:38 +0000
Subject: [PATCH 01/28] Enrich and personalise the role request emails.

* Add the entity the role was requested on into the message
* Add context and personalise role request emails

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 56 +++++++++++++++-------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index dd7d8c7a7..56ee69bf9 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -6,6 +6,7 @@
  */
 require_once __DIR__ . '/AbstractEntityService.php';
 require_once __DIR__ . '/Factory.php';
+require_once __DIR__ . '/User.php';
 
 /**
  * A service layer to aid the sending of notification emails
@@ -27,10 +28,11 @@ class NotificationService extends AbstractEntityService {
      *
      * @param Site/ServiceGroup/NGI/Project $entity
      */
-    public function roleRequest($entity) {
+    public function roleRequest ($role_requested, $requesting_user, $entity) {
         $project = null;
-        $emails = null;
+        $authorising_user_ids = null;
         $projectIds = null;
+        $entity_name = $entity->getName();
 
         // Get the roles from the entity
         foreach ( $entity->getRoles () as $role ) {
@@ -45,7 +47,7 @@ public function roleRequest($entity) {
 
                 // If the site has no site adminstrators to approve the role request then send an email to the parent NGI users to approve the request
                 if ($roles == null) {
-                    $this->roleRequest ( $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
+                    $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
                 }
             } else if ($entity instanceof \ServiceGroup) {
                 //$enablingRoles = \Factory::getServiceGroupService ()->authorize Action ( \Action::GRANT_ROLE, $entity, $role->getUser () );
@@ -70,18 +72,18 @@ public function roleRequest($entity) {
             if ($position != null) {
                 unset ( $enablingRoles [$position] );
             }*/
-            // Get the users email and add it to the array if they have an enabling role
+            // Get the user id and add it to the array if they have an enabling role
             if (count ( $enablingRoles ) > 0) {
-                $emails [] = $role->getUser ()->getEmail ();
+                $authorising_user_ids [] = $role->getUser ()->getId ();
             }
         }
 
         /*
          * No users are able to grant the role or there are no users over this entity. In this case we will email the parent entity for approval
          */
-        if ($emails == null || count($emails) == 0) {
+        if ($authorising_user_ids == null || count($authorising_user_ids) == 0) {
             if ($entity instanceof \Site) {
-                $this->roleRequest ( $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
+                $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
             } else if ($entity instanceof \NGI) {
                 /*
                  * It is important to remove duplicate projects here otherwise we will spam the same addresses as we recursively call this method.
@@ -97,8 +99,8 @@ public function roleRequest($entity) {
         } else {
             // If the entity has valid users who can approve the role then send the email notification.
 
-            // Remove duplicate emails from array
-            $emails = array_unique ( $emails );
+            // Remove duplicate user ids from array
+            $authorising_user_ids = array_unique ( $authorising_user_ids );
 
             // Get the PortalURL to create an accurate link to the role approval view
             $localInfoLocation = __DIR__ . "/../../config/local_info.xml";
@@ -107,16 +109,34 @@ public function roleRequest($entity) {
 
             // Email content
             $headers = "From: no-reply@goc.egi.eu";
-            $subject = "GocDB: A Role request requires attention";
-
-            $body = "Dear GOCDB User,\n\n" . "A user has requested a role that requires attention.\n\n" .
-                    "You can approve or deny this request here:\n\n" . $webPortalURL . "/index.php?Page_Type=Role_Requests\n\n" .
-                    "Note: This role may already have been approved or denied by another GocDB User";
-
             $sendMail = TRUE;
             // Send email to all users who can approve this role request
-            if ($emails != null) {
-                foreach ( $emails as $email ) {
+            if ($authorising_user_ids != null) {
+                foreach ( $authorising_user_ids as $user_id ) {
+                    $approving_user = \Factory::getUserService()->getUser($user_id);
+                    $email = $approving_user->getEmail();
+                    $subject = sprintf(
+                        'GocDB: A Role request from %1$s over %2$s requires your attention',
+                        $requesting_user->getForename(),
+                        $entity_name
+                    );
+                    $body = sprintf(
+                        implode("\n", array(
+                            'Dear %1$s,',
+                            '%2$s requested %3$s on %4$s which requires your attention.',
+                            '',
+                            'You can approve or deny the request here:',
+                            '    %3$s/index.php?Page_Type=Role_Requests',
+                            '',
+                            'Note: This role could already have been approved or denied by another GocDB User',
+                        ),
+                        $approving_user->getForename(),
+                        $requesting_user->getForename(),
+                        $role_requested->getRoleType()->getName(),
+                        $role_requested->getOwnedEntity()->getName(),
+                        $webPortalURL
+                    ));
+
                     if($sendMail){
                        mail($email, $subject, $body, $headers);
                     } else {
@@ -137,7 +157,7 @@ public function roleRequest($entity) {
             foreach ( $projectIds as $pid ) {
                 $project = \Factory::getOwnedEntityService ()->getOwnedEntityById ( $pid );
                 if(sendMail){
-                    $this->roleRequest ( $project );
+                    $this->roleRequest ( $role_requested, $requesting_user, $project );
                 } else {
                     echo $project->getName () . "<br>";
                 }

From e1ef4cd8c1860b977d10ef939ba3c84525b3f171 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 10:42:37 +0100
Subject: [PATCH 02/28] Factor out preparation and sending of email to a
 seperate function

This somewhat improves readability.
---
 lib/Gocdb_Services/NotificationService.php | 82 ++++++++++++----------
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 56ee69bf9..6f84f7965 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -102,48 +102,11 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             // Remove duplicate user ids from array
             $authorising_user_ids = array_unique ( $authorising_user_ids );
 
-            // Get the PortalURL to create an accurate link to the role approval view
-            $localInfoLocation = __DIR__ . "/../../config/local_info.xml";
-            $localInfoXML = simplexml_load_file ( $localInfoLocation );
-            $webPortalURL = $localInfoXML->local_info->web_portal_url;
-
-            // Email content
-            $headers = "From: no-reply@goc.egi.eu";
-            $sendMail = TRUE;
             // Send email to all users who can approve this role request
             if ($authorising_user_ids != null) {
                 foreach ( $authorising_user_ids as $user_id ) {
                     $approving_user = \Factory::getUserService()->getUser($user_id);
-                    $email = $approving_user->getEmail();
-                    $subject = sprintf(
-                        'GocDB: A Role request from %1$s over %2$s requires your attention',
-                        $requesting_user->getForename(),
-                        $entity_name
-                    );
-                    $body = sprintf(
-                        implode("\n", array(
-                            'Dear %1$s,',
-                            '%2$s requested %3$s on %4$s which requires your attention.',
-                            '',
-                            'You can approve or deny the request here:',
-                            '    %3$s/index.php?Page_Type=Role_Requests',
-                            '',
-                            'Note: This role could already have been approved or denied by another GocDB User',
-                        ),
-                        $approving_user->getForename(),
-                        $requesting_user->getForename(),
-                        $role_requested->getRoleType()->getName(),
-                        $role_requested->getOwnedEntity()->getName(),
-                        $webPortalURL
-                    ));
-
-                    if($sendMail){
-                       mail($email, $subject, $body, $headers);
-                    } else {
-                       echo "Email: " . $email . "<br>";
-                       echo "Subject: " . $subject . "<br>";
-                       echo "Body: " . $body . "<br>";
-                    }
+                    $this->send_email($role_requested, $requesting_user, $entity_name, $requesting_user);
                 }
             }
         }
@@ -164,4 +127,47 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             }
         }
     }
+
+    private function send_email($role_requested, $requesting_user, $entity_name, $approving_user) {
+        $sendMail = TRUE;
+        $headers = "From: no-reply@goc.egi.eu";
+
+        // Get the PortalURL to create an accurate link to the role approval view
+        $localInfoLocation = __DIR__ . "/../../config/local_info.xml";
+        $localInfoXML = simplexml_load_file ( $localInfoLocation );
+        $webPortalURL = $localInfoXML->local_info->web_portal_url;
+
+        $subject = sprintf(
+            'GocDB: A Role request from %1$s over %2$s requires your attention',
+            $requesting_user->getForename(),
+            $entity_name
+        );
+
+        $body = sprintf(
+            implode("\n", array(
+                'Dear %1$s,',
+                '%2$s requested %3$s on %4$s which requires your attention.',
+                '',
+                'You can approve or deny the request here:',
+                '    %5$s/index.php?Page_Type=Role_Requests',
+                '',
+                'Note: This role could already have been approved or denied by another GocDB User',
+            ),
+            $approving_user->getForename(),
+            $requesting_user->getForename(),
+            $role_requested->getRoleType()->getName(),
+            $role_requested->getOwnedEntity()->getName(),
+            $webPortalURL
+        ));
+
+        $email = $approving_user->getEmail();
+
+        if ($sendMail) {
+            mail($email, $subject, $body, $headers);
+        } else {
+            echo "Email: $email<br>";
+            echo "Subject: $subject<br>";
+            echo "Body: $body<br>";
+        }
+    }
 }

From 08b7a7a7afd42dd2500414444ed72d06f1929fc1 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 10:44:01 +0100
Subject: [PATCH 03/28] Remove commented out code cruft

---
 lib/Gocdb_Services/NotificationService.php | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 6f84f7965..0c366253b 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -67,11 +67,6 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
                 }
             }
 
-            // remove admin from enabling roles
-            /*$position = array_search ( 'GOCDB_ADMIN', $enablingRoles );
-            if ($position != null) {
-                unset ( $enablingRoles [$position] );
-            }*/
             // Get the user id and add it to the array if they have an enabling role
             if (count ( $enablingRoles ) > 0) {
                 $authorising_user_ids [] = $role->getUser ()->getId ();

From 3ee8160168dbaeac7ca0b8c357080f39185188fa Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Fri, 1 Oct 2021 12:43:33 +0000
Subject: [PATCH 04/28] Remove if statement from NotificationService call

- it's not up to this piece of code to determine if
  emails should be sent to the used.
- that is currently done by the NotificationService itself.
---
 htdocs/web_portal/controllers/political_role/request_role.php | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/htdocs/web_portal/controllers/political_role/request_role.php b/htdocs/web_portal/controllers/political_role/request_role.php
index 1d7b268d6..afe9bf1c9 100644
--- a/htdocs/web_portal/controllers/political_role/request_role.php
+++ b/htdocs/web_portal/controllers/political_role/request_role.php
@@ -95,9 +95,7 @@ function submitRoleRequest($roleName, $entityId, \User $user =null) {
    // perfoms role validation and throws exceptios accordingly.
    $newRole = \Factory::getRoleService()->addRole($roleName, $user, $entity);
 
-   if(\Factory::getConfigService()->getSendEmails()){
-       \Factory::getNotificationService()->roleRequest($entity);
-   }
+   \Factory::getNotificationService()->roleRequest($entity);
 
    show_view('political_role/new_request.php');
 }

From a262b85c9c0439dd8b382435b0ef8ad3f4596fde Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:00:47 +0100
Subject: [PATCH 05/28] Pass newRole and user to initial call to roleRequest

---
 htdocs/web_portal/controllers/political_role/request_role.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/htdocs/web_portal/controllers/political_role/request_role.php b/htdocs/web_portal/controllers/political_role/request_role.php
index afe9bf1c9..4061d9e38 100644
--- a/htdocs/web_portal/controllers/political_role/request_role.php
+++ b/htdocs/web_portal/controllers/political_role/request_role.php
@@ -95,7 +95,7 @@ function submitRoleRequest($roleName, $entityId, \User $user =null) {
    // perfoms role validation and throws exceptios accordingly.
    $newRole = \Factory::getRoleService()->addRole($roleName, $user, $entity);
 
-   \Factory::getNotificationService()->roleRequest($entity);
+   \Factory::getNotificationService()->roleRequest($newRole, $user, $entity);
 
    show_view('political_role/new_request.php');
 }

From e69c8bb35ffd100a6549b57c09e1b1b4020bea11 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:05:42 +0100
Subject: [PATCH 06/28] Actually pass approving user to send_email

---
 lib/Gocdb_Services/NotificationService.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 0c366253b..e68b522a2 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -101,7 +101,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             if ($authorising_user_ids != null) {
                 foreach ( $authorising_user_ids as $user_id ) {
                     $approving_user = \Factory::getUserService()->getUser($user_id);
-                    $this->send_email($role_requested, $requesting_user, $entity_name, $requesting_user);
+                    $this->send_email($role_requested, $requesting_user, $entity_name, $approving_user);
                 }
             }
         }

From 6db804d23257cb4c9c5c93119f54118f29293da8 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:13:53 +0100
Subject: [PATCH 07/28] Factor out getting of web PortalURL to a private
 function

---
 lib/Gocdb_Services/NotificationService.php | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index e68b522a2..cd2669070 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -123,15 +123,20 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         }
     }
 
+
+    /**
+    * Return the PortalURL to enable an accurate link to the role approval view to be created
+    */
+    private function get_webPortalURL() {
+        $localInfoXML = simplexml_load_file(__DIR__ . "/../../config/local_info.xml");
+        return $localInfoXML->local_info->web_portal_url;
+    }
+
+
     private function send_email($role_requested, $requesting_user, $entity_name, $approving_user) {
         $sendMail = TRUE;
         $headers = "From: no-reply@goc.egi.eu";
 
-        // Get the PortalURL to create an accurate link to the role approval view
-        $localInfoLocation = __DIR__ . "/../../config/local_info.xml";
-        $localInfoXML = simplexml_load_file ( $localInfoLocation );
-        $webPortalURL = $localInfoXML->local_info->web_portal_url;
-
         $subject = sprintf(
             'GocDB: A Role request from %1$s over %2$s requires your attention',
             $requesting_user->getForename(),
@@ -152,7 +157,7 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
             $requesting_user->getForename(),
             $role_requested->getRoleType()->getName(),
             $role_requested->getOwnedEntity()->getName(),
-            $webPortalURL
+            $this->get_webPortalURL()
         ));
 
         $email = $approving_user->getEmail();

From 3382f89ec35eca64d1a3b3e0b0f05bd8169a7ae3 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:23:19 +0100
Subject: [PATCH 08/28] Add private mock mail function for debugging

---
 lib/Gocdb_Services/NotificationService.php | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index cd2669070..cfc13678a 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -133,6 +133,19 @@ private function get_webPortalURL() {
     }
 
 
+    private function mock_mail($to, $subject, $message, $additional_headers = "", $additional_parameters = "") {
+        echo "<!--\n";
+        echo "Sending mail disabled, but would have sent:\n";
+        echo "$additional_headers\n";
+        echo "To: $to\n";
+        echo "Subject: $subject\n";
+        echo "\n$message\n";
+        echo "\nAdditional Parameters: $additional_parameters\n";
+        echo "-->\n";
+        return True;
+    }
+
+
     private function send_email($role_requested, $requesting_user, $entity_name, $approving_user) {
         $sendMail = TRUE;
         $headers = "From: no-reply@goc.egi.eu";
@@ -152,22 +165,20 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
                 '    %5$s/index.php?Page_Type=Role_Requests',
                 '',
                 'Note: This role could already have been approved or denied by another GocDB User',
-            ),
+            )),
             $approving_user->getForename(),
             $requesting_user->getForename(),
             $role_requested->getRoleType()->getName(),
             $role_requested->getOwnedEntity()->getName(),
             $this->get_webPortalURL()
-        ));
+        );
 
         $email = $approving_user->getEmail();
 
         if ($sendMail) {
             mail($email, $subject, $body, $headers);
         } else {
-            echo "Email: $email<br>";
-            echo "Subject: $subject<br>";
-            echo "Body: $body<br>";
+            $this->mock_mail($email, $subject, $body, $headers);
         }
     }
 }

From 4462025a650a323dbb2822118819d2cda4eb4c46 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:30:38 +0100
Subject: [PATCH 09/28]  Read send_email from config file and act appropriately

Remove hardcoded sendmail "test"
---
 lib/Gocdb_Services/NotificationService.php | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index cfc13678a..f55808c24 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -133,6 +133,15 @@ private function get_webPortalURL() {
     }
 
 
+    /**
+    * Return whether send_email is enabled in the config file
+    */
+    private function get_config_send_email() {
+        $localInfoXML = simplexml_load_file(__DIR__ . "/../../config/local_info.xml");
+        return strtolower($localInfoXML->local_info->send_email) === 'true';
+    }
+
+
     private function mock_mail($to, $subject, $message, $additional_headers = "", $additional_parameters = "") {
         echo "<!--\n";
         echo "Sending mail disabled, but would have sent:\n";
@@ -147,7 +156,6 @@ private function mock_mail($to, $subject, $message, $additional_headers = "", $a
 
 
     private function send_email($role_requested, $requesting_user, $entity_name, $approving_user) {
-        $sendMail = TRUE;
         $headers = "From: no-reply@goc.egi.eu";
 
         $subject = sprintf(
@@ -175,7 +183,7 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
 
         $email = $approving_user->getEmail();
 
-        if ($sendMail) {
+        if ($this->get_config_send_email()) {
             mail($email, $subject, $body, $headers);
         } else {
             $this->mock_mail($email, $subject, $body, $headers);

From f2f02acd3b55063137fc6f447c4c72e2baccf5b1 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:31:06 +0100
Subject: [PATCH 10/28] Move headers next to where it is used

---
 lib/Gocdb_Services/NotificationService.php | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index f55808c24..dcc7e5502 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -156,8 +156,6 @@ private function mock_mail($to, $subject, $message, $additional_headers = "", $a
 
 
     private function send_email($role_requested, $requesting_user, $entity_name, $approving_user) {
-        $headers = "From: no-reply@goc.egi.eu";
-
         $subject = sprintf(
             'GocDB: A Role request from %1$s over %2$s requires your attention',
             $requesting_user->getForename(),
@@ -182,6 +180,7 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
         );
 
         $email = $approving_user->getEmail();
+        $headers = "From: no-reply@goc.egi.eu";
 
         if ($this->get_config_send_email()) {
             mail($email, $subject, $body, $headers);

From 3c9bbd9970db2b08ebd28ca3de7806baa848d6cf Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 11:38:23 +0100
Subject: [PATCH 11/28] Remove redundant code paths

This seems to have originally been factored out as part of 229a0e847523270a3f62b1ad9a1a359dbdcb4482
and has been sitting around since then.
---
 lib/Gocdb_Services/NotificationService.php | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index dcc7e5502..02a13e7f4 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -43,18 +43,11 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         foreach ( $roles as $role ) {
             $enablingRoles = \Factory::getRoleActionAuthorisationService()->authoriseAction(\Action::GRANT_ROLE, $entity, $role->getUser())->getGrantingRoles();
             if ($entity instanceof \Site) {
-                //$enablingRoles = \Factory::getSiteService ()->authorize Action ( \Action::GRANT_ROLE, $entity, $role->getUser () );
-
                 // If the site has no site adminstrators to approve the role request then send an email to the parent NGI users to approve the request
                 if ($roles == null) {
                     $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
                 }
-            } else if ($entity instanceof \ServiceGroup) {
-                //$enablingRoles = \Factory::getServiceGroupService ()->authorize Action ( \Action::GRANT_ROLE, $entity, $role->getUser () );
-            } else if ($entity instanceof \Project) {
-                //$enablingRoles = \Factory::getProjectService ()->authorize Action ( \Action::GRANT_ROLE, $entity, $role->getUser () );
             } else if ($entity instanceof \NGI) {
-                //$enablingRoles = \Factory::getNgiService ()->authorize Action ( \Action::GRANT_ROLE, $entity, $role->getUser () );
                 $projects = $entity->getProjects (); // set project with the NGI's parent project and later recurse with this
 
                 // Only send emails to Project users if there are no users with grant_roles over the NGI

From d755b31dfc14270ee2ab0d2c29a7802972b145fb Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Thu, 15 Aug 2019 12:32:31 +0100
Subject: [PATCH 12/28] Don't create single-use local variable for entity name

---
 lib/Gocdb_Services/NotificationService.php | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 02a13e7f4..758862fbc 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -32,7 +32,6 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         $project = null;
         $authorising_user_ids = null;
         $projectIds = null;
-        $entity_name = $entity->getName();
 
         // Get the roles from the entity
         foreach ( $entity->getRoles () as $role ) {
@@ -94,7 +93,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             if ($authorising_user_ids != null) {
                 foreach ( $authorising_user_ids as $user_id ) {
                     $approving_user = \Factory::getUserService()->getUser($user_id);
-                    $this->send_email($role_requested, $requesting_user, $entity_name, $approving_user);
+                    $this->send_email($role_requested, $requesting_user, $entity->getName(), $approving_user);
                 }
             }
         }

From 7df9d51cca459003af73c5639515e2c769ce2051 Mon Sep 17 00:00:00 2001
From: James Adams <james.adams@stfc.ac.uk>
Date: Wed, 26 Aug 2020 11:41:26 +0100
Subject: [PATCH 13/28] Fix from header and set email address to admin list

Fixes #168 (emails "from GOCDB" should come from gocdb-admins@mailman.egi.eu).
---
 lib/Gocdb_Services/NotificationService.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 758862fbc..889999e26 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -172,7 +172,7 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
         );
 
         $email = $approving_user->getEmail();
-        $headers = "From: no-reply@goc.egi.eu";
+        $headers = "From: GOCDB <gocdb-admins@mailman.egi.eu>";
 
         if ($this->get_config_send_email()) {
             mail($email, $subject, $body, $headers);

From b156c28cae335cd2afd040e7836276244c4a7008 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:44:55 +0100
Subject: [PATCH 14/28] Handle case where no user has a role over entity

Co-Authored-By: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 889999e26..d98027f51 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -33,7 +33,9 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         $authorising_user_ids = null;
         $projectIds = null;
 
-        // Get the roles from the entity
+        // Get the roles from the entity, if any.
+        $roles = [];
+
         foreach ( $entity->getRoles () as $role ) {
             $roles [] = $role;
         }

From 9b8b9f2726c931f955c101d4a6c4d674a888f6f8 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:44:56 +0100
Subject: [PATCH 15/28] Use the role requested object to populate email

- this way, if a site role request gets passed up to an NGI level
  user, they will see the site name in the request (not the NGI
  name).
---
 lib/Gocdb_Services/NotificationService.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index d98027f51..4c74e3282 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -153,7 +153,7 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
         $subject = sprintf(
             'GocDB: A Role request from %1$s over %2$s requires your attention',
             $requesting_user->getForename(),
-            $entity_name
+            $role_requested->getOwnedEntity()->getName()
         );
 
         $body = sprintf(

From 443211ec19bd2d2a4648de832b5f116d667266b5 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:44:57 +0100
Subject: [PATCH 16/28] Always recurse roleRequest when project role needed

- I guess this logic was somewhat there to test /stop recursion
  if you didn't want to send an email.
- The logic of whether to send an email or just print what would
  have been sent has been factored out already, so we don't need
  this if statement anymore.
---
 lib/Gocdb_Services/NotificationService.php | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 4c74e3282..58ae24513 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -108,11 +108,8 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         if ($projectIds != null) {
             foreach ( $projectIds as $pid ) {
                 $project = \Factory::getOwnedEntityService ()->getOwnedEntityById ( $pid );
-                if(sendMail){
-                    $this->roleRequest ( $role_requested, $requesting_user, $project );
-                } else {
-                    echo $project->getName () . "<br>";
-                }
+                $this->roleRequest ( $role_requested, $requesting_user, $project );
+
             }
         }
     }

From 845a0272778fa29b4bcfb618ed3e8963bfcf293e Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:44:58 +0100
Subject: [PATCH 17/28] Move enablingRoles initialisation closer to use

---
 lib/Gocdb_Services/NotificationService.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 58ae24513..675cf3c55 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -42,7 +42,6 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
 
         // Now for each role get the user
         foreach ( $roles as $role ) {
-            $enablingRoles = \Factory::getRoleActionAuthorisationService()->authoriseAction(\Action::GRANT_ROLE, $entity, $role->getUser())->getGrantingRoles();
             if ($entity instanceof \Site) {
                 // If the site has no site adminstrators to approve the role request then send an email to the parent NGI users to approve the request
                 if ($roles == null) {
@@ -61,6 +60,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
                 }
             }
 
+            $enablingRoles = \Factory::getRoleActionAuthorisationService()->authoriseAction(\Action::GRANT_ROLE, $entity, $role->getUser())->getGrantingRoles();
             // Get the user id and add it to the array if they have an enabling role
             if (count ( $enablingRoles ) > 0) {
                 $authorising_user_ids [] = $role->getUser ()->getId ();

From 0deddb53a6aa4f49cb9cf0fac9b21c4db5dcb182 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 26 Aug 2021 14:35:10 +0100
Subject: [PATCH 18/28] Use the Factory to access local config

- to support different views of GOCDB being served by the same instance.

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 675cf3c55..c7cadfcae 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -119,8 +119,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
     * Return the PortalURL to enable an accurate link to the role approval view to be created
     */
     private function get_webPortalURL() {
-        $localInfoXML = simplexml_load_file(__DIR__ . "/../../config/local_info.xml");
-        return $localInfoXML->local_info->web_portal_url;
+        return \Factory::getConfigService()->GetPortalURL();
     }
 
 
@@ -128,8 +127,7 @@ private function get_webPortalURL() {
     * Return whether send_email is enabled in the config file
     */
     private function get_config_send_email() {
-        $localInfoXML = simplexml_load_file(__DIR__ . "/../../config/local_info.xml");
-        return strtolower($localInfoXML->local_info->send_email) === 'true';
+        return \Factory::getConfigService()->getSendEmails();
     }
 
 

From eb54ec494107967279f614e6593f440b79ef6a44 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:44:59 +0100
Subject: [PATCH 19/28] Remove un-needed code.

- the behaviour of recursing this function is repeated further
  down.
---
 lib/Gocdb_Services/NotificationService.php | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index c7cadfcae..1d0ecf317 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -42,24 +42,6 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
 
         // Now for each role get the user
         foreach ( $roles as $role ) {
-            if ($entity instanceof \Site) {
-                // If the site has no site adminstrators to approve the role request then send an email to the parent NGI users to approve the request
-                if ($roles == null) {
-                    $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
-                }
-            } else if ($entity instanceof \NGI) {
-                $projects = $entity->getProjects (); // set project with the NGI's parent project and later recurse with this
-
-                // Only send emails to Project users if there are no users with grant_roles over the NGI
-                if ($roles == null) {
-                    // Get the ID's of each project so we can remove duplicates
-                    foreach ( $projects as $project ) {
-                        $projectIds [] = $project->getId ();
-                    }
-                    $projectIds = array_unique ( $projectIds );
-                }
-            }
-
             $enablingRoles = \Factory::getRoleActionAuthorisationService()->authoriseAction(\Action::GRANT_ROLE, $entity, $role->getUser())->getGrantingRoles();
             // Get the user id and add it to the array if they have an enabling role
             if (count ( $enablingRoles ) > 0) {

From c49b00cc4151de4cce1cc88deca70faec243d73d Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:45:00 +0100
Subject: [PATCH 20/28] Check array is empty rather than null

- `if ( empty( $array_name ) )` is more readable. I think we
  previously got away with it because `[] == null` returns true,
  or because the code interchanged empty arrays and `null`.
---
 lib/Gocdb_Services/NotificationService.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 1d0ecf317..d9837498e 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -74,7 +74,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             $authorising_user_ids = array_unique ( $authorising_user_ids );
 
             // Send email to all users who can approve this role request
-            if ($authorising_user_ids != null) {
+            if (!empty($authorising_user_ids)) {
                 foreach ( $authorising_user_ids as $user_id ) {
                     $approving_user = \Factory::getUserService()->getUser($user_id);
                     $this->send_email($role_requested, $requesting_user, $entity->getName(), $approving_user);

From 6efb29bd7cac84d99560ee5632c73d0f8b6dcea6 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:45:01 +0100
Subject: [PATCH 21/28] Simplify determination of users with GRANT_ROLE

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 32 +++++++++++-----------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index d9837498e..e5727c6c6 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -30,29 +30,29 @@ class NotificationService extends AbstractEntityService {
      */
     public function roleRequest ($role_requested, $requesting_user, $entity) {
         $project = null;
-        $authorising_user_ids = null;
+        $authorising_user_ids = [];
         $projectIds = null;
 
-        // Get the roles from the entity, if any.
-        $roles = [];
+        // For each role the entity has.
+        foreach ($entity->getRoles() as $role) {
+            // Get the corresponding user.
+            $user = $role->getUser();
 
-        foreach ( $entity->getRoles () as $role ) {
-            $roles [] = $role;
-        }
+            // Determine if that user has roles that can approve role requests,
+            // this role may not be the same as the one currently in the $role
+            // variable.
+            $enablingRoles = \Factory::getRoleActionAuthorisationService()->authoriseAction(\Action::GRANT_ROLE, $entity, $user)->getGrantingRoles();
 
-        // Now for each role get the user
-        foreach ( $roles as $role ) {
-            $enablingRoles = \Factory::getRoleActionAuthorisationService()->authoriseAction(\Action::GRANT_ROLE, $entity, $role->getUser())->getGrantingRoles();
-            // Get the user id and add it to the array if they have an enabling role
-            if (count ( $enablingRoles ) > 0) {
-                $authorising_user_ids [] = $role->getUser ()->getId ();
+            // If they can, add their user id to the list of authorising user
+            // ids.
+            if (count($enablingRoles) > 0) {
+                $authorising_user_ids [] = $user->getId();
             }
         }
 
-        /*
-         * No users are able to grant the role or there are no users over this entity. In this case we will email the parent entity for approval
-         */
-        if ($authorising_user_ids == null || count($authorising_user_ids) == 0) {
+        // If there are no users that are able to grant the role over this entity,
+        // we will email the parent entity for approval.
+        if (count($authorising_user_ids) == 0) {
             if ($entity instanceof \Site) {
                 $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
             } else if ($entity instanceof \NGI) {

From eac6b741f23357f69c55188b28c85cddf5203b45 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:45:02 +0100
Subject: [PATCH 22/28] Improve comments around recursion

---
 lib/Gocdb_Services/NotificationService.php | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index e5727c6c6..1062fcbd7 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -54,9 +54,11 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         // we will email the parent entity for approval.
         if (count($authorising_user_ids) == 0) {
             if ($entity instanceof \Site) {
+                // Sites can only have a single parent NGI.
                 $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
             } else if ($entity instanceof \NGI) {
                 /*
+                 * NGIs can belong to multiple Projects.
                  * It is important to remove duplicate projects here otherwise we will spam the same addresses as we recursively call this method.
                  */
                 $projects = $entity->getProjects (); // set project with the NGI's parent project and later recurse with this

From 34bf0a80a73be1ec7ab3c0f299f3148706e4bf0a Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:45:02 +0100
Subject: [PATCH 23/28] Use camelCase throughout

---
 lib/Gocdb_Services/NotificationService.php | 52 +++++++++++-----------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 1062fcbd7..6c62711fa 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -28,9 +28,9 @@ class NotificationService extends AbstractEntityService {
      *
      * @param Site/ServiceGroup/NGI/Project $entity
      */
-    public function roleRequest ($role_requested, $requesting_user, $entity) {
+    public function roleRequest ($roleRequested, $requestingUser, $entity) {
         $project = null;
-        $authorising_user_ids = [];
+        $authorisingUserIds = [];
         $projectIds = null;
 
         // For each role the entity has.
@@ -46,16 +46,16 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             // If they can, add their user id to the list of authorising user
             // ids.
             if (count($enablingRoles) > 0) {
-                $authorising_user_ids [] = $user->getId();
+                $authorisingUserIds [] = $user->getId();
             }
         }
 
         // If there are no users that are able to grant the role over this entity,
         // we will email the parent entity for approval.
-        if (count($authorising_user_ids) == 0) {
+        if (count($authorisingUserIds) == 0) {
             if ($entity instanceof \Site) {
                 // Sites can only have a single parent NGI.
-                $this->roleRequest ( $role_requested, $requesting_user, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
+                $this->roleRequest ( $roleRequested, $requestingUser, $entity->getNgi () ); // Recursivly call this function to send email to the NGI users
             } else if ($entity instanceof \NGI) {
                 /*
                  * NGIs can belong to multiple Projects.
@@ -73,13 +73,13 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
             // If the entity has valid users who can approve the role then send the email notification.
 
             // Remove duplicate user ids from array
-            $authorising_user_ids = array_unique ( $authorising_user_ids );
+            $authorisingUserIds = array_unique ( $authorisingUserIds );
 
             // Send email to all users who can approve this role request
-            if (!empty($authorising_user_ids)) {
-                foreach ( $authorising_user_ids as $user_id ) {
-                    $approving_user = \Factory::getUserService()->getUser($user_id);
-                    $this->send_email($role_requested, $requesting_user, $entity->getName(), $approving_user);
+            if (!empty($authorisingUserIds)) {
+                foreach ( $authorisingUserIds as $userId ) {
+                    $approvingUser = \Factory::getUserService()->getUser($userId);
+                    $this->sendEmail($roleRequested, $requestingUser, $entity->getName(), $approvingUser);
                 }
             }
         }
@@ -92,7 +92,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
         if ($projectIds != null) {
             foreach ( $projectIds as $pid ) {
                 $project = \Factory::getOwnedEntityService ()->getOwnedEntityById ( $pid );
-                $this->roleRequest ( $role_requested, $requesting_user, $project );
+                $this->roleRequest ( $roleRequested, $requestingUser, $project );
 
             }
         }
@@ -102,7 +102,7 @@ public function roleRequest ($role_requested, $requesting_user, $entity) {
     /**
     * Return the PortalURL to enable an accurate link to the role approval view to be created
     */
-    private function get_webPortalURL() {
+    private function getWebPortalURL() {
         return \Factory::getConfigService()->GetPortalURL();
     }
 
@@ -110,29 +110,29 @@ private function get_webPortalURL() {
     /**
     * Return whether send_email is enabled in the config file
     */
-    private function get_config_send_email() {
+    private function getConfigSendEmail() {
         return \Factory::getConfigService()->getSendEmails();
     }
 
 
-    private function mock_mail($to, $subject, $message, $additional_headers = "", $additional_parameters = "") {
+    private function mockMail($to, $subject, $message, $additionalHeaders = "", $additionalParameters = "") {
         echo "<!--\n";
         echo "Sending mail disabled, but would have sent:\n";
-        echo "$additional_headers\n";
+        echo "$additionalHeaders\n";
         echo "To: $to\n";
         echo "Subject: $subject\n";
         echo "\n$message\n";
-        echo "\nAdditional Parameters: $additional_parameters\n";
+        echo "\nAdditional Parameters: $additionalParameters\n";
         echo "-->\n";
         return True;
     }
 
 
-    private function send_email($role_requested, $requesting_user, $entity_name, $approving_user) {
+    private function sendEmail($roleRequested, $requestingUser, $entityName, $approvingUser) {
         $subject = sprintf(
             'GocDB: A Role request from %1$s over %2$s requires your attention',
-            $requesting_user->getForename(),
-            $role_requested->getOwnedEntity()->getName()
+            $requestingUser->getForename(),
+            $roleRequested->getOwnedEntity()->getName()
         );
 
         $body = sprintf(
@@ -145,20 +145,20 @@ private function send_email($role_requested, $requesting_user, $entity_name, $ap
                 '',
                 'Note: This role could already have been approved or denied by another GocDB User',
             )),
-            $approving_user->getForename(),
-            $requesting_user->getForename(),
-            $role_requested->getRoleType()->getName(),
-            $role_requested->getOwnedEntity()->getName(),
-            $this->get_webPortalURL()
+            $approvingUser->getForename(),
+            $requestingUser->getForename(),
+            $roleRequested->getRoleType()->getName(),
+            $roleRequested->getOwnedEntity()->getName(),
+            $this->getWebPortalURL()
         );
 
         $email = $approving_user->getEmail();
         $headers = "From: GOCDB <gocdb-admins@mailman.egi.eu>";
 
-        if ($this->get_config_send_email()) {
+        if ($this->getConfigSendEmail()) {
             mail($email, $subject, $body, $headers);
         } else {
-            $this->mock_mail($email, $subject, $body, $headers);
+            $this->mockMail($email, $subject, $body, $headers);
         }
     }
 }

From 569fcde84a0a42759324a9de2f23295732edd8b4 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:41:47 +0100
Subject: [PATCH 24/28] Factor out email sending into its own service

- for later reuse.
---
 lib/Gocdb_Services/EmailService.php        | 47 ++++++++++++++++++++++
 lib/Gocdb_Services/Factory.php             | 14 +++++++
 lib/Gocdb_Services/NotificationService.php | 30 +-------------
 3 files changed, 63 insertions(+), 28 deletions(-)
 create mode 100644 lib/Gocdb_Services/EmailService.php

diff --git a/lib/Gocdb_Services/EmailService.php b/lib/Gocdb_Services/EmailService.php
new file mode 100644
index 000000000..113a035bd
--- /dev/null
+++ b/lib/Gocdb_Services/EmailService.php
@@ -0,0 +1,47 @@
+<?php
+
+namespace org\gocdb\services;
+
+require_once __DIR__ . '/AbstractEntityService.php';
+require_once __DIR__ . '/Factory.php';
+
+/**
+
+ */
+class EmailService extends AbstractEntityService {
+    /**
+     * Depending on the configuration, either send an email or print what would
+     * have been sent.
+     * @param string $emailAddress    A single email address to send to
+     * @param string $subject         The subject of the email
+     * @param string $body            The body of the email
+     * @param string $headers         The headers of the email
+
+     */
+    public function send($emailAddress, $subject, $body, $headers) {
+        if ($this->getConfigSendEmail()) {
+            mail($emailAddress, $subject, $body, $headers);
+        } else {
+            $this->mockMail($emailAddress, $subject, $body, $headers);
+      }
+    }
+
+    /**
+    * Return whether send_email is enabled in the config file
+    */
+    private function getConfigSendEmail() {
+        return \Factory::getConfigService()->getSendEmails();
+    }
+
+    private function mockMail($to, $subject, $message, $additionalHeaders = "", $additionalParameters = "") {
+        echo "<!--\n";
+        echo "Sending mail disabled, but would have sent:\n";
+        echo "$additionalHeaders\n";
+        echo "To: $to\n";
+        echo "Subject: $subject\n";
+        echo "\n$message\n";
+        echo "\nAdditional Parameters: $additionalParameters\n";
+        echo "-->\n";
+        return True;
+    }
+}
diff --git a/lib/Gocdb_Services/Factory.php b/lib/Gocdb_Services/Factory.php
index 7c9019610..356ee835f 100644
--- a/lib/Gocdb_Services/Factory.php
+++ b/lib/Gocdb_Services/Factory.php
@@ -48,6 +48,7 @@ class Factory {
     private static $OwnedEntityService = null;
     private static $exService = null;
     private static $notificationService = null;
+    private static $emailService = null;
 
     public static $properties = array();
     //private static $properties = null;
@@ -385,6 +386,19 @@ public static function getNotificationService() {
         }
         return self::$notificationService;
     }
+
+    /**
+     * Singleton EmailService
+     * @return org\gocdb\services\EmailService
+     */
+    public static function getEmailService() {
+        if (self::$emailService == null) {
+            require_once __DIR__ . '/EmailService.php';
+            self::$emailService = new org\gocdb\services\EmailService();
+            self::$emailService->setEntityManager(self::getEntityManager());
+        }
+        return self::$emailService;
+    }
 }
 
 ?>
diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 6c62711fa..6dae365f9 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -106,28 +106,6 @@ private function getWebPortalURL() {
         return \Factory::getConfigService()->GetPortalURL();
     }
 
-
-    /**
-    * Return whether send_email is enabled in the config file
-    */
-    private function getConfigSendEmail() {
-        return \Factory::getConfigService()->getSendEmails();
-    }
-
-
-    private function mockMail($to, $subject, $message, $additionalHeaders = "", $additionalParameters = "") {
-        echo "<!--\n";
-        echo "Sending mail disabled, but would have sent:\n";
-        echo "$additionalHeaders\n";
-        echo "To: $to\n";
-        echo "Subject: $subject\n";
-        echo "\n$message\n";
-        echo "\nAdditional Parameters: $additionalParameters\n";
-        echo "-->\n";
-        return True;
-    }
-
-
     private function sendEmail($roleRequested, $requestingUser, $entityName, $approvingUser) {
         $subject = sprintf(
             'GocDB: A Role request from %1$s over %2$s requires your attention',
@@ -152,13 +130,9 @@ private function sendEmail($roleRequested, $requestingUser, $entityName, $approv
             $this->getWebPortalURL()
         );
 
-        $email = $approving_user->getEmail();
+        $emailAddress = $approvingUser->getEmail();
         $headers = "From: GOCDB <gocdb-admins@mailman.egi.eu>";
 
-        if ($this->getConfigSendEmail()) {
-            mail($email, $subject, $body, $headers);
-        } else {
-            $this->mockMail($email, $subject, $body, $headers);
-        }
+        \Factory::getEmailService()->send($emailAddress, $subject, $body, $headers);
     }
 }

From 1e0ad984e4c1df16733ce734a0820d3c20c5afef Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 15:09:52 +0100
Subject: [PATCH 25/28] Update documentation of NotificationService class

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 6dae365f9..d17866a92 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -26,7 +26,8 @@ class NotificationService extends AbstractEntityService {
      * send an email to those users to approve. It does this by passing the parent entity back into this method recursively.
      *
      *
-     * @param Site/ServiceGroup/NGI/Project $entity
+     * @param OwnedEntity $entity An instance of Site,Service,NGI,Project or other OwnedEntity.
+     * @return void
      */
     public function roleRequest ($roleRequested, $requestingUser, $entity) {
         $project = null;

From 82abbedebaa461f2f1ce64e9c2ce8a72b57eaffe Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Thu, 30 Sep 2021 16:35:08 +0100
Subject: [PATCH 26/28] Capitalise "GocDB" in
 lib/Gocdb_Services/NotificationService.php

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index d17866a92..d7d8fd71d 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -109,7 +109,7 @@ private function getWebPortalURL() {
 
     private function sendEmail($roleRequested, $requestingUser, $entityName, $approvingUser) {
         $subject = sprintf(
-            'GocDB: A Role request from %1$s over %2$s requires your attention',
+            'GOCDB: A Role request from %1$s over %2$s requires your attention',
             $requestingUser->getForename(),
             $roleRequested->getOwnedEntity()->getName()
         );
@@ -122,7 +122,7 @@ private function sendEmail($roleRequested, $requestingUser, $entityName, $approv
                 'You can approve or deny the request here:',
                 '    %5$s/index.php?Page_Type=Role_Requests',
                 '',
-                'Note: This role could already have been approved or denied by another GocDB User',
+                'Note: This role could already have been approved or denied by another GOCDB User',
             )),
             $approvingUser->getForename(),
             $requestingUser->getForename(),

From 6f6d3427616fc82888e8a9a4adc5e1201a5d42cd Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Fri, 1 Oct 2021 12:27:10 +0000
Subject: [PATCH 27/28] Add requesting User's surname to emails

- for clarity, as the email could be someone outside their entity.
- i.e, a Site role request email going to someone with a role
  over an NGI.
---
 lib/Gocdb_Services/NotificationService.php | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index d7d8fd71d..53e794383 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -109,23 +109,26 @@ private function getWebPortalURL() {
 
     private function sendEmail($roleRequested, $requestingUser, $entityName, $approvingUser) {
         $subject = sprintf(
-            'GOCDB: A Role request from %1$s over %2$s requires your attention',
+            'GOCDB: A Role request from %1$s %2$s over %3$s requires your attention',
             $requestingUser->getForename(),
+            $requestingUser->getSurname(),
             $roleRequested->getOwnedEntity()->getName()
         );
 
         $body = sprintf(
             implode("\n", array(
                 'Dear %1$s,',
-                '%2$s requested %3$s on %4$s which requires your attention.',
+                '',
+                '%2$s %3$s requested the "%4$s" role over %5$s which requires your attention.',
                 '',
                 'You can approve or deny the request here:',
-                '    %5$s/index.php?Page_Type=Role_Requests',
+                '    %6$s/index.php?Page_Type=Role_Requests',
                 '',
                 'Note: This role could already have been approved or denied by another GOCDB User',
             )),
             $approvingUser->getForename(),
             $requestingUser->getForename(),
+            $requestingUser->getSurname(),
             $roleRequested->getRoleType()->getName(),
             $roleRequested->getOwnedEntity()->getName(),
             $this->getWebPortalURL()

From 1dce39858d7f10293468bf4b859d9da30123ee25 Mon Sep 17 00:00:00 2001
From: gregcorbett <gregcorbett@users.noreply.github.com>
Date: Mon, 4 Oct 2021 13:42:50 +0100
Subject: [PATCH 28/28] Remove unneeded check for emptiness

- this check is part of the `else` of an
  `if (count($authorisingUserIds) == 0)` statement.
- `array_unique` won't empty `$authorisingUserIds`.
- hence, the check here isn't needed.

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
---
 lib/Gocdb_Services/NotificationService.php | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/Gocdb_Services/NotificationService.php b/lib/Gocdb_Services/NotificationService.php
index 53e794383..20ad162e0 100644
--- a/lib/Gocdb_Services/NotificationService.php
+++ b/lib/Gocdb_Services/NotificationService.php
@@ -77,11 +77,9 @@ public function roleRequest ($roleRequested, $requestingUser, $entity) {
             $authorisingUserIds = array_unique ( $authorisingUserIds );
 
             // Send email to all users who can approve this role request
-            if (!empty($authorisingUserIds)) {
-                foreach ( $authorisingUserIds as $userId ) {
-                    $approvingUser = \Factory::getUserService()->getUser($userId);
-                    $this->sendEmail($roleRequested, $requestingUser, $entity->getName(), $approvingUser);
-                }
+           foreach ( $authorisingUserIds as $userId ) {
+                $approvingUser = \Factory::getUserService()->getUser($userId);
+                $this->sendEmail($roleRequested, $requestingUser, $entity->getName(), $approvingUser);
             }
         }