diff --git a/src/Reports/PagesDueForReviewReport.php b/src/Reports/PagesDueForReviewReport.php index 7fb3545..fc0bd02 100644 --- a/src/Reports/PagesDueForReviewReport.php +++ b/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; + } } } diff --git a/src/Reports/PagesWithoutReviewScheduleReport.php b/src/Reports/PagesWithoutReviewScheduleReport.php index 0b9ef9e..0f7b402 100644 --- a/src/Reports/PagesWithoutReviewScheduleReport.php +++ b/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; } diff --git a/src/Tasks/ContentReviewEmails.php b/src/Tasks/ContentReviewEmails.php index 28d58f9..c59bc56 100644 --- a/src/Tasks/ContentReviewEmails.php +++ b/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); } } diff --git a/tests/php/ContentReviewNotificationTest.php b/tests/php/ContentReviewNotificationTest.php index 09d32d7..019fa4a 100644 --- a/tests/php/ContentReviewNotificationTest.php +++ b/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(); + } + } }