Merge pull request #195 from creative-commoners/pulls/4.7/fix-broken-job-on-wrong-email

FIX Notification job marked as broken

src/Tasks/ContentReviewEmails.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\ContentReview\Tasks;
 
 use Page;
+use RuntimeException;
 use SilverStripe\ContentReview\Compatibility\ContentReviewCompatability;
 use SilverStripe\Control\Email\Email;
 use SilverStripe\Control\HTTPRequest;
@@ -15,18 +16,29 @@ use SilverStripe\Security\Member;
 use SilverStripe\SiteConfig\SiteConfig;
 use SilverStripe\View\ArrayData;
 use SilverStripe\View\SSViewer;
-use SilverStripe\ContentReview\Models\ContentReviewLog;
 
 /**
  * Daily task to send emails to the owners of content items when the review date rolls around.
  */
 class ContentReviewEmails extends BuildTask
 {
+    private array $invalid_emails = [];
+
     /**
      * @param HTTPRequest $request
+     * @throws RuntimeException
      */
     public function run($request)
     {
+        if (!$this->isValidEmail($senderEmail = SiteConfig::current_site_config()->ReviewFrom)) {
+            throw new RuntimeException(
+                sprintf(
+                    'Provided sender email address is invalid: "%s".',
+                    $senderEmail
+                )
+            );
+        }
+
         $compatibility = ContentReviewCompatability::start();
 
         // First grab all the pages with a custom setting
@@ -42,6 +54,16 @@ class ContentReviewEmails extends BuildTask
         }
 
         ContentReviewCompatability::done($compatibility);
+
+        if (is_array($this->invalid_emails) && count($this->invalid_emails) > 0) {
+            $plural = count($this->invalid_emails) > 1 ? 's are' : ' is';
+            throw new RuntimeException(
+                sprintf(
+                    'Provided email' . $plural . ' invalid: "%s".',
+                    implode(', ', $this->invalid_emails)
+                )
+            );
+        }
     }
 
     /**
@@ -93,6 +115,13 @@ class ContentReviewEmails extends BuildTask
         // Prepare variables
         $siteConfig = SiteConfig::current_site_config();
         $owner = Member::get()->byID($ownerID);
+
+        if (!$this->isValidEmail($owner->Email)) {
+            $this->invalid_emails[] = $owner->Name . ': ' . $owner->Email;
+
+            return;
+        }
+
         $templateVariables = $this->getTemplateVariables($owner, $siteConfig, $pages);
 
         // Build email
@@ -159,4 +188,12 @@ class ContentReviewEmails extends BuildTask
             'ToEmail' => $recipient->Email,
         ];
     }
+
+    /**
+     * Check validity of email
+     */
+    protected function isValidEmail(?string $email): bool
+    {
+        return (bool) filter_var($email, FILTER_VALIDATE_EMAIL);
+    }
 }

tests/php/ContentReviewNotificationTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\ContentReview\Tests;
 
 use Page;
+use ReflectionClass;
 use SilverStripe\CMS\Model\SiteTree;
 use SilverStripe\CMS\Controllers\CMSPageEditController;
 use SilverStripe\ContentReview\Extensions\ContentReviewCMSExtension;
@@ -132,6 +133,26 @@ class ContentReviewNotificationTest extends SapphireTest
         DBDatetime::clear_mock_now();
     }
 
+    /**
+     * Test that provided email is valid
+     */
+    public function testIsValidEmail()
+    {
+        $class = new ReflectionClass(ContentReviewEmails::class);
+        $method = $class->getMethod('isValidEmail');
+        $method->setAccessible(true);
+
+        $member = $this->objFromFixture(Member::class, 'author');
+        $task = new ContentReviewEmails();
+
+        $this->assertTrue($method->invokeArgs($task, [$member->Email]));
+        $this->assertTrue($method->invokeArgs($task, ['correct.email@example.com']));
+
+        $this->assertFalse($method->invokeArgs($task, [null]));
+        $this->assertFalse($method->invokeArgs($task, ['broken.email']));
+        $this->assertFalse($method->invokeArgs($task, ['broken@email']));
+    }
+
     /**
      * Deletes all pages except those passes in to the $ids parameter
      *