BUG When setting up a new site, the default pages will not inherit a review date from siteconfig #20

code/extensions/SiteTreeContentReview.php

@@ -18,6 +18,15 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 		'LastEditedByName' => 'Varchar(255)',
 		'OwnerNames' => 'Varchar(255)'
 	);
+
+	
+	/**
+	 *
+	 * @var array
+	 */
+	private static $defaults = array(
+		'ContentReviewType' => 'Inherit'
+	);
 	
 	/**
 	 *
@@ -383,45 +392,27 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 		$this->owner->LastEditedByName=$this->owner->getEditorName();
 		$this->owner->OwnerNames = $this->owner->getOwnerNames();
 		
-		// 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 = $this->owner->getOptions();
-		
 		// If the user changed the Type, we need to recalculate the
 		// Next review date
 		if($this->owner->isChanged('ContentReviewType', 2)) {
+			
 			// Changed to Disabled
 			if($this->owner->ContentReviewType == 'Disabled') {
-				$nextDate = null;
+				$this->setDefaultReviewDateForDisabled();
-			// Changed to Inherit
-			} elseif($this->owner->ContentReviewType == 'Inherit') {
-				// Take from Parent page
-				if($settings && $this->owner->parent()->exists()) {
-					$nextDate = $this->getReviewDate($this->owner->parent());
-				// Inherit from siteconfig
-				} elseif($settings instanceof SiteConfig) {
-					$nextDate = $this->getReviewDate();
-				// No setting, parent disabled
-				} else {
-					 $nextDate = null;
-				}
 			// Changed to Custom
+			} elseif($this->owner->ContentReviewType == 'Custom') {
+				$this->setDefaultReviewDateForCustom();
+			// Changed to Inherit
 			} else {
-				if($nextDate = $this->owner->NextReviewDate) {
+				$this->setDefaultReviewDateForInherited();
-					$nextDate = $this->owner->NextReviewDate;
-				// No review date provided, use today + period
-				} else {
-					$this->owner->NextReviewDate = null;
-					$nextDate = $this->getReviewDate();
-				}
-			}
-			if(is_object($nextDate)) {
-				$this->owner->NextReviewDate = $nextDate->getValue();
-			} else {
-				$this->owner->NextReviewDate = $nextDate;
 			}
 		}
 		
+		// Ensure that a inherited page always have a next review date
+		if($this->owner->ContentReviewType == 'Inherit' && !$this->owner->NextReviewDate) {
+			$this->setDefaultReviewDateForInherited();
+		}
+		
 		// Oh yey.. now we need to update all the child pages that inherit this setting
 		// We can only change children after this record has been created, otherwise the stageChildren
 		// method will grab all pages in the DB (this messes up unittesting)
@@ -429,11 +420,14 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 			return;
 		}
 		
-		if($this->owner->isChanged('ReviewPeriodDays', 2) && !$this->owner->isChanged('ContentReviewType', 2)) {
+		// parent page change it's review period 
+		// && !$this->owner->isChanged('ContentReviewType', 2)
+		if($this->owner->isChanged('ReviewPeriodDays', 2)) {
 			$nextReviewUnixSec = strtotime(' + '.$this->owner->ReviewPeriodDays . ' days', SS_Datetime::now()->format('U'));
-			$this->owner->NextReviewDate = date('Y-m-d');
+			$this->owner->NextReviewDate = date('Y-m-d', $nextReviewUnixSec);
 		}
 		
+		//
 		if($this->owner->isChanged('NextReviewDate', 2)) {
 			$children = $this->owner->stageChildren(true)->filter('ContentReviewType', 'Inherit');
 			foreach($children as $child) {
@@ -442,7 +436,53 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 			}
 		}
 	}
-
+	
+	/**
+	 * 
+	 */
+	private function setDefaultReviewDateForDisabled() {
+		$this->owner->NextReviewDate = null; 
+	}
+	
+	/**
+	 * 
+	 */
+	protected function setDefaultReviewDateForCustom() {
+		if($this->owner->NextReviewDate) {
+			return;
+		}
+		
+		$this->owner->NextReviewDate = null;
+		$nextDate = $this->getReviewDate();
+		
+		if(is_object($nextDate)) {
+			$this->owner->NextReviewDate = $nextDate->getValue(); 
+		} else {
+			$this->owner->NextReviewDate = $nextDate; 
+		}
+		
+	}
+	
+	/**
+	 * 
+	 */
+	protected function setDefaultReviewDateForInherited() {
+		$options = $this->getOptions();
+		$nextDate = null;
+		// Set the review date to be the same as the parents reviewdate
+		if($options && $this->owner->parent()->exists()) {
+			$nextDate = $this->getReviewDate($this->owner->parent());
+		// Use the defualt settings
+		} elseif($options instanceof SiteConfig) {
+			$nextDate = $this->getReviewDate();
+		} 
+		if(is_object($nextDate)) {
+			$this->owner->NextReviewDate = $nextDate->getValue(); 
+		} else {
+			$this->owner->NextReviewDate = $nextDate; 
+		}
+	}
+	
 	/**
 	 * Provide permissions to the CMS
 	 * 

tests/ContentReviewCMSPageEditControllerTest.php

@@ -4,6 +4,14 @@ class ContentReviewCMSPageEditControllerTest extends FunctionalTest {
 	
 	public static $fixture_file = 'contentreview/tests/ContentReviewTest.yml';
 	
+	protected $requiredExtensions = array(
+		"SiteTree" => array("SiteTreeContentReview"),
+		"Group" => array("ContentReviewOwner"),
+		"Member" => array("ContentReviewOwner"),
+		"CMSPageEditController" => array("ContentReviewCMSExtension"),
+		"SiteConfig" => array("ContentReviewDefaultSettings"),
+	);
+	
 	public function testReviewedThrowsExceptionWithNoRecordID() {
 		$this->setExpectedException('SS_HTTPResponse_Exception', 'No record ID', 404);
 		$controller = new CMSPageEditController();

tests/ContentReviewNotificationTest.php

@@ -4,6 +4,14 @@ class ContentReviewNotificationTest extends SapphireTest {
 	
 	public static $fixture_file = 'contentreview/tests/ContentReviewTest.yml';
 	
+	protected $requiredExtensions = array(
+		"SiteTree" => array("SiteTreeContentReview"),
+		"Group" => array("ContentReviewOwner"),
+		"Member" => array("ContentReviewOwner"),
+		"CMSPageEditController" => array("ContentReviewCMSExtension"),
+		"SiteConfig" => array("ContentReviewDefaultSettings"),
+	);
+	
 	public function testContentReviewEmails() {
 		SS_Datetime::set_mock_now('2010-02-24 12:00:00');
 		

tests/ContentReviewReportTest.php

@@ -4,6 +4,14 @@ class ContentReviewReportTest extends FunctionalTest {
 	
 	public static $fixture_file = 'contentreview/tests/ContentReviewTest.yml';
 	
+	protected $requiredExtensions = array(
+		"SiteTree" => array("SiteTreeContentReview"),
+		"Group" => array("ContentReviewOwner"),
+		"Member" => array("ContentReviewOwner"),
+		"CMSPageEditController" => array("ContentReviewCMSExtension"),
+		"SiteConfig" => array("ContentReviewDefaultSettings"),
+	);
+	
 	public function testReportContent() {
 		$editor = $this->objFromFixture('Member', 'editor');
 		$this->logInAs($editor);

tests/ContentReviewSettingsTest.php

@@ -7,6 +7,14 @@ class ContentReviewSettingsTest extends SapphireTest {
 
 	public static $fixture_file = 'contentreview/tests/ContentReviewSettingsTest.yml';
 	
+	protected $requiredExtensions = array(
+		"SiteTree" => array("SiteTreeContentReview"),
+		"Group" => array("ContentReviewOwner"),
+		"Member" => array("ContentReviewOwner"),
+		"CMSPageEditController" => array("ContentReviewCMSExtension"),
+		"SiteConfig" => array("ContentReviewDefaultSettings"),
+	);
+	
 	public function testAdvanceReviewDate10Days() {
 		$page = new Page();
 		$page->ContentReviewType = 'Custom';
@@ -167,15 +175,20 @@ class ContentReviewSettingsTest extends SapphireTest {
 		$parentPage = $this->objFromFixture('Page', 'page-1');
 		$childPage = $this->objFromFixture('Page', 'page-1-1');
 		
-		// AFTER: parent page have a period of five days, so childPage should have a 
+		// AFTER: parent page have a period of 10 days, so childPage should have a 
-		// review date LastEdited + 5 days
+		// review date now + 10 days
 		$this->assertNotEquals($oldChildDate, $childPage->NextReviewDate);
+		$this->assertEquals($this->addDaysToDate(date('Y-m-d'), 10), $childPage->NextReviewDate);
 		$this->assertEquals($parentPage->NextReviewDate, $childPage->NextReviewDate);
 	}
 	
 	// helper method for this test class
 	private function addDaysToDate($date, $days, $format='Y-m-d') {
-		$sec = strtotime('+ '. $days .' days', $date->format('U'));
+		if(is_object($date)) {
+			$sec = strtotime('+ '. $days .' days', $date->format('U'));
+		} else {
+			$sec = strtotime('+ '. $days .' days', strtotime($date));
+		}
 		return date($format, $sec);
 	}
 }

tests/SiteTreeContentReviewTest.php

@@ -4,6 +4,14 @@ class SiteTreeContentReviewTest extends FunctionalTest {
 	
 	public static $fixture_file = 'contentreview/tests/ContentReviewTest.yml';
 	
+	protected $requiredExtensions = array(
+		"SiteTree" => array("SiteTreeContentReview"),
+		"Group" => array("ContentReviewOwner"),
+		"Member" => array("ContentReviewOwner"),
+		"CMSPageEditController" => array("ContentReviewCMSExtension"),
+		"SiteConfig" => array("ContentReviewDefaultSettings"),
+	);
+	
 	public function testOwnerNames() {
 		$editor = $this->objFromFixture('Member', 'editor');
 		$this->logInAs($editor);