Merge branch '4.1'

src/Reports/PagesDueForReviewReport.php

@@ -217,9 +217,11 @@ class PagesDueForReviewReport extends Report
             $records = $records->filterByCallback(function ($page) use ($currentUser) {
                 $options = $page->getOptions();
 
-                foreach ($options->ContentReviewOwners() as $owner) {
-                    if ($currentUser->ID == $owner->ID) {
-                        return true;
+                if ($options) {
+                    foreach ($options->ContentReviewOwners() as $owner) {
+                        if ($currentUser->ID == $owner->ID) {
+                            return true;
+                        }
                     }
                 }
 

src/Reports/PagesWithoutReviewScheduleReport.php

@@ -154,7 +154,7 @@ class PagesWithoutReviewScheduleReport extends Report
 
         $options = $record->getOptions();
 
-        if ($options->OwnerGroups()->count() == 0 && $options->OwnerUsers()->count() == 0) {
+        if ($options && $options->OwnerGroups()->count() == 0 && $options->OwnerUsers()->count() == 0) {
             return false;
         }
 

src/Tasks/ContentReviewEmails.php

@@ -15,6 +15,7 @@ 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.
@@ -57,14 +58,26 @@ class ContentReviewEmails extends BuildTask
                 continue;
             }
 
-            $option = $page->getOptions();
+            // get most recent review log of current [age]
+            $contentReviewLog = $page->ReviewLogs()->sort('Created DESC')->first();
 
-            foreach ($option->ContentReviewOwners() as $owner) {
-                if (!isset($overduePages[$owner->ID])) {
-                    $overduePages[$owner->ID] = ArrayList::create();
+            // check log date vs NextReviewDate. If someone has left a content review
+            // after the review date, then we don't need to notify anybody
+            if ($contentReviewLog && $contentReviewLog->Created >= $page->NextReviewDate) {
+                $page->advanceReviewDate();
+                continue;
+            }
+
+            $options = $page->getOptions();
+
+            if ($options) {
+                foreach ($options->ContentReviewOwners() as $owner) {
+                    if (!isset($overduePages[$owner->ID])) {
+                        $overduePages[$owner->ID] = ArrayList::create();
+                    }
+
+                    $overduePages[$owner->ID]->push($page);
                 }
-
-                $overduePages[$owner->ID]->push($page);
             }
         }
 

tests/php/ContentReviewNotificationTest.php

@@ -16,6 +16,7 @@ use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\Security\Group;
 use SilverStripe\Security\Member;
 use SilverStripe\SiteConfig\SiteConfig;
+use SilverStripe\ContentReview\Models\ContentReviewLog;
 
 /**
  * @mixin PHPUnit_Framework_TestCase
@@ -81,4 +82,70 @@ class ContentReviewNotificationTest extends SapphireTest
 
         DBDatetime::clear_mock_now();
     }
+
+    /**
+     * When a content review is left after a review date, we want to ensure that
+     * overdue notifications aren't sent.
+     */
+    public function testContentReviewNeeded()
+    {
+        DBDatetime::set_mock_now('2018-08-10 12:00:00');
+
+        /** @var Page|SiteTreeContentReview $childParentPage */
+        $childParentPage = $this->objFromFixture(Page::class, 'no-review');
+        $childParentPage->NextReviewDate = '2018-08-10';
+        $childParentPage->write();
+
+        // we need to ensure only our test page is being ran. If we don't do this
+        // then it may notify for other pages which fails our test
+        $this->deleteAllPagesExcept([$childParentPage->ID]);
+
+        // Grabbing the 'author' from member class
+        $member = $this->objFromFixture(Member::class, 'author');
+
+        // Assigning member as contentreviewer to page
+        $childParentPage->ContentReviewUsers()->add($member);
+
+        // Assert that only one reviewer is assigned to page
+        $this->assertCount(1, $childParentPage->ContentReviewUsers());
+
+        // Create new log
+        $log = new ContentReviewLog();
+
+        // Assign log reviewer as current member
+        $log->ReviewerID = $member->ID;
+
+        // Assign log ID to page ID
+        $log->SiteTreeID = $childParentPage->ID;
+
+        // Write to DB
+        $log->write();
+
+        // assert that log was created day of review
+        $this->assertEquals('2018-08-10 12:00:00', $log->Created);
+        $this->assertCount(1, $childParentPage->ReviewLogs());
+
+        $task = new ContentReviewEmails();
+        $task->run(new HTTPRequest('GET', '/dev/tasks/ContentReviewEmails'));
+
+        // Expecting to not send the email as content review for page is done
+        $email = $this->findEmail($member->Email);
+        $this->assertNull($email);
+
+        DBDatetime::clear_mock_now();
+    }
+
+    /**
+     * Deletes all pages except those passes in to the $ids parameter
+     *
+     * @param  array  $ids Page IDs which will NOT be deleted
+     */
+    private function deleteAllPagesExcept(array $ids)
+    {
+        $pages = SiteTree::get()->exclude('ID', $ids);
+
+        foreach ($pages as $page) {
+            $page->delete();
+        }
+    }
 }