From 638d2fc331e70d498bafbc21cd9316ef4e5eac67 Mon Sep 17 00:00:00 2001 From: Stig Lindqvist Date: Mon, 24 Feb 2014 21:10:10 +1300 Subject: [PATCH] Refactorings --- code/ContentReviewEmails.php | 8 +-- code/extensions/SiteTreeContentReview.php | 66 ++++++++--------------- code/reports/PagesDueForReviewReport.php | 8 ++- tests/ContentReviewSettingsTest.php | 27 +++------- 4 files changed, 34 insertions(+), 75 deletions(-) diff --git a/code/ContentReviewEmails.php b/code/ContentReviewEmails.php index a902034..81e8a92 100644 --- a/code/ContentReviewEmails.php +++ b/code/ContentReviewEmails.php @@ -72,16 +72,12 @@ class ContentReviewEmails extends BuildTask { */ protected function getOverduePagesForOwners(SS_list $pages, array &$overduePages) { foreach($pages as $page) { - - // Update the NextReviewDate cache for this page - //$page->updateNextReviewDate($forceWrite = true); - if(!$page->isContentReviewOverdue()) { continue; } - $settings = SiteTreeContentReview::get_options($page); - foreach($settings->ContentReviewOwners() as $owner) { + $option = $page->getOptions(); + foreach($option->ContentReviewOwners() as $owner) { if(!isset(self::$member_cache[$owner->ID])) { self::$member_cache[$owner->ID] = $owner; } diff --git a/code/extensions/SiteTreeContentReview.php b/code/extensions/SiteTreeContentReview.php index 55a79ec..d5bf92b 100644 --- a/code/extensions/SiteTreeContentReview.php +++ b/code/extensions/SiteTreeContentReview.php @@ -62,24 +62,22 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider /** * - * @param DataObject $setting + * @param DataObject $options * @param SiteTree $page * @return Date | false - returns false if the content review have disabled */ - public static function get_next_review_date(DataObject $setting, SiteTree $page) { + public function getReviewDate(DataObject $options, SiteTree $page) { if($page->obj('NextReviewDate')->exists()) { return $page->obj('NextReviewDate'); } - - if(!$setting) { + if(!$options) { return false; } - - if(!$setting->ReviewPeriodDays) { + if(!$options->ReviewPeriodDays) { return false; } // Failover to check on ReviewPeriodDays + LastEdited - $nextReviewUnixSec = strtotime(' + '.$setting->ReviewPeriodDays . ' days', SS_Datetime::now()->format('U')); + $nextReviewUnixSec = strtotime(' + '.$options->ReviewPeriodDays . ' days', SS_Datetime::now()->format('U')); $date = Date::create('NextReviewDate'); $date->setValue(date('Y-m-d H:i:s', $nextReviewUnixSec)); return $date; @@ -87,22 +85,25 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider /** * Get the object that have the information about the content - * review settings + * review settings. Either + * - a SiteTreeContentReview decorated object + * - the default SiteTree config + * - false if this page have it's content review disabled * * Will go through parents and root pages will use the siteconfig * if their setting is Inherit. * - * @param SiteTree $page * @return DataObject or false if no settings found */ - public static function get_options($page) { - if($page->ContentReviewType == 'Custom') { - return $page; + public function getOptions() { + if($this->owner->ContentReviewType == 'Custom') { + return $this->owner; } - if($page->ContentReviewType == 'Disabled') { + if($this->owner->ContentReviewType == 'Disabled') { return false; } + $page = $this->owner; // $page is inheriting it's settings from it's parent, find // the first valid parent with a valid setting while($parent = $page->Parent()) { @@ -142,10 +143,11 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider * @return string */ public function getEditorName() { - if($member = Member::currentUser()) { + $member = Member::currentUser(); + if($member) { return $member->FirstName .' '. $member->Surname; } - return NULL; + return null; } /** @@ -316,32 +318,6 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider return $hasNextReview; } - /** - * Update the NextReviewDate and save the dataobject, - * this is typically done after the ContentReviewType has changed - * - * @param DataObject $settings - * @param bool $forceWrite - */ - public function updateNextReviewDate($forceWrite = false) { - $settings = self::get_options($this->owner); - - if(!$settings && $this->owner->NextReviewDate) { - $this->owner->NextReviewDate = null; - } - - if($settings) { - $nextDate = self::get_next_review_date($settings, $this->owner); - if($nextDate && $nextDate) { - $this->owner->NextReviewDate = $nextDate->getValue(); - } - } - - if($forceWrite) { - $this->owner->write(); - } - } - /** * * @param \FieldList $actions @@ -395,7 +371,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider // This contains the DataObject that have the content review settings // for this object, it might be this page, one of its parent or SiteConfig - $settings = self::get_options($this->owner); + $settings = $this->owner->getOptions(); // If the user changed the Type, we need to recalculate the // Next review date @@ -409,10 +385,10 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider $this->owner->NextReviewDate = null; // Take from Parent page if($settings && $this->owner->parent()->exists()) { - $nextDate = self::get_next_review_date($settings, $this->owner->parent()); + $nextDate = $this->getReviewDate($settings, $this->owner->parent()); // Inherit from siteconfig } elseif($settings instanceof SiteConfig) { - $nextDate = self::get_next_review_date($settings, $this->owner); + $nextDate = $this->getReviewDate($settings, $this->owner); // No setting, parent disabled } else { $nextDate = null; @@ -424,7 +400,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider // No review date provided, use today + period } else { $this->owner->NextReviewDate = null; - $nextDate = self::get_next_review_date($settings, $this->owner); + $nextDate = $this->getReviewDate($settings, $this->owner); } } if(is_object($nextDate)) { diff --git a/code/reports/PagesDueForReviewReport.php b/code/reports/PagesDueForReviewReport.php index 053738c..0e39ca0 100644 --- a/code/reports/PagesDueForReviewReport.php +++ b/code/reports/PagesDueForReviewReport.php @@ -101,7 +101,7 @@ class PagesDueForReviewReport extends SS_Report { return 'disabled'; } if($item->ContentReviewType == 'Inherit') { - $setting = $item->getContentReviewSetting($item); + $setting = SiteTreeContentReview::getOptions($item); if(!$setting) { return 'disabled'; } @@ -117,7 +117,7 @@ class PagesDueForReviewReport extends SS_Report { return 'disabled'; } if($item->ContentReviewType == 'Inherit') { - $setting = $item->getContentReviewSetting($item); + $setting = SiteTreeContentReview::getOptions($item); if(!$setting) { return 'disabled'; } @@ -158,9 +158,7 @@ class PagesDueForReviewReport extends SS_Report { // If there's no review dates set, default to all pages due for review now $reviewDate = new Zend_Date(SS_Datetime::now()->Format('U')); $reviewDate->add(1, Zend_Date::DAY); - $records = $records->where('"ContentReviewType" != \'Disabled\''); - //$records = $records->where(sprintf('"NextReviewDate" < \'%s\'', $reviewDate->toString('YYYY-MM-dd'))); - + $records = $records->where(sprintf('"NextReviewDate" < \'%s\'', $reviewDate->toString('YYYY-MM-dd'))); } else { // Review date before if(!empty($params['ReviewDateBefore'])) { diff --git a/tests/ContentReviewSettingsTest.php b/tests/ContentReviewSettingsTest.php index 4a8ec93..66463b3 100644 --- a/tests/ContentReviewSettingsTest.php +++ b/tests/ContentReviewSettingsTest.php @@ -4,10 +4,9 @@ * This class tests that settings are inherited correctly based on the inherited, custom or disabled settings */ class ContentReviewSettingsTest extends SapphireTest { - + public static $fixture_file = 'contentreview/tests/ContentReviewSettingsTest.yml'; - public function testAdvanceReviewFromCustomSettings() { $page = $this->objFromFixture('Page', 'custom'); $this->assertTrue($page->advanceReviewDate()); @@ -34,42 +33,36 @@ class ContentReviewSettingsTest extends SapphireTest { public function testGetSettingsObjectFromCustom() { $page = $this->objFromFixture('Page', 'custom'); $this->assertEquals('Custom', $page->ContentReviewType); - $setting = SiteTreeContentReview::get_options($page); - $this->assertEquals($page, $setting); + $this->assertEquals($page, $page->getOptions()); } public function testGetSettingsObjectFromDisabled() { $page = $this->objFromFixture('Page', 'disabled'); $this->assertEquals('Disabled', $page->ContentReviewType); - $setting = SiteTreeContentReview::get_options($page); - $this->assertFalse($setting); + $this->assertFalse($page->getOptions()); } public function testGetSettingsObjectFromInheritPage() { $page = $this->objFromFixture('Page', 'page-1-1'); $this->assertEquals('Inherit', $page->ContentReviewType); - $settings = SiteTreeContentReview::get_options($page); - $this->assertEquals($this->objFromFixture('Page', 'page-1'), $settings); + $this->assertEquals($this->objFromFixture('Page', 'page-1'), $page->getOptions()); } public function testGetSettingsObjectFromInheritedRootPage() { $page = $this->objFromFixture('Page', 'inherit'); $this->assertEquals('Inherit', $page->ContentReviewType); - $settings = SiteTreeContentReview::get_options($page); - $this->assertEquals($this->objFromFixture('SiteConfig', 'default'), $settings); + $this->assertEquals($this->objFromFixture('SiteConfig', 'default'), $page->getOptions()); } public function testGetNextReviewDateFromCustomSettings() { $page = $this->objFromFixture('Page', 'custom'); - $settings = SiteTreeContentReview::get_options($page); - $date = SiteTreeContentReview::get_next_review_date($settings, $page); + $date = $page->getReviewDate($page->getOptions(), $page); $this->assertEquals('2010-02-01', $date->format('Y-m-d')); } public function testGetNextReviewDateFromSiteConfigInheritedSetting() { $page = $this->objFromFixture('Page', 'inherit'); - $settings = SiteTreeContentReview::get_options($page); - $nextReviewDate = SiteTreeContentReview::get_next_review_date($settings, $page); + $nextReviewDate = $page->getReviewDate($page->getOptions(), $page); $this->assertInstanceOf('Date', $nextReviewDate); $expected = $this->addDaysToDate(SS_Datetime::now(), $this->objFromFixture('SiteConfig', 'default')->ReviewPeriodDays); @@ -78,9 +71,8 @@ class ContentReviewSettingsTest extends SapphireTest { public function testGetNextReviewDateFromPageInheritedSetting() { $page = $this->objFromFixture('Page', 'page-1-1'); - $settings = SiteTreeContentReview::get_options($page); + $nextReviewDate = $page->getReviewDate($page->getOptions(), $page); - $nextReviewDate = SiteTreeContentReview::get_next_review_date($settings, $page); $this->assertInstanceOf('Date', $nextReviewDate); // It should be the same as the parents reviewdate $expected = $this->objFromFixture('Page', 'page-1')->NextReviewDate; @@ -133,7 +125,6 @@ class ContentReviewSettingsTest extends SapphireTest { $expected = $this->addDaysToDate($childPage->obj('LastEdited'), $parentPage->ReviewPeriodDays); $this->assertEquals($parentPage->NextReviewDate, $childPage->NextReviewDate); - $oldChildDate = $childPage->NextReviewDate; // But if we change the parent page ReviewPeriodDays to 10, the childs should // change as well @@ -149,7 +140,6 @@ class ContentReviewSettingsTest extends SapphireTest { // AFTER: parent page have a period of five days, so childPage should have a // review date LastEdited + 5 days $this->assertNotEquals($oldChildDate, $childPage->NextReviewDate); - $this->assertEquals($parentPage->NextReviewDate, $childPage->NextReviewDate); } @@ -158,5 +148,4 @@ class ContentReviewSettingsTest extends SapphireTest { $sec = strtotime('+ '. $days .' days', $date->format('U')); return date($format, $sec); } - } \ No newline at end of file