Merge pull request #36 from open-sausages/pulls/fix-inherited-date

BUG Show inherited date even when inheriting settings

code/extensions/SiteTreeContentReview.php

@@ -352,6 +352,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 
         $userField = ListboxField::create("OwnerUsers", _t("ContentReview.PAGEOWNERUSERS", "Users"), $usersMap)
             ->setMultiple(true)
+			->addExtraClass('custom-setting')
             ->setAttribute("data-placeholder", _t("ContentReview.ADDUSERS", "Add users"))
             ->setDescription(_t('ContentReview.OWNERUSERSDESCRIPTION', 'Page owners that are responsible for reviews'));
 
@@ -364,6 +365,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 
         $groupField = ListboxField::create("OwnerGroups", _t("ContentReview.PAGEOWNERGROUPS", "Groups"), $groupsMap)
             ->setMultiple(true)
+			->addExtraClass('custom-setting')
             ->setAttribute("data-placeholder", _t("ContentReview.ADDGROUP", "Add groups"))
             ->setDescription(_t("ContentReview.OWNERGROUPSDESCRIPTION", "Page owners that are responsible for reviews"));
 
@@ -373,7 +375,12 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
             ->setConfig("datavalueformat", "yyyy-MM-dd")
             ->setDescription(_t("ContentReview.NEXTREVIEWDATADESCRIPTION", "Leave blank for no review"));
 
-        $reviewFrequency = DropdownField::create("ReviewPeriodDays", _t("ContentReview.REVIEWFREQUENCY", "Review frequency"), self::get_schedule())
+        $reviewFrequency = DropdownField::create(
+			"ReviewPeriodDays",
+			_t("ContentReview.REVIEWFREQUENCY", "Review frequency"),
+			self::get_schedule()
+		)
+			->addExtraClass('custom-setting')
             ->setDescription(_t("ContentReview.REVIEWFREQUENCYDESCRIPTION", "The review date will be set to this far in the future whenever the page is published"));
 
         $notesField = GridField::create("ReviewNotes", "Review Notes", $this->owner->ReviewLogs(), GridFieldConfig_RecordEditor::create());
@@ -386,9 +393,8 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
                 $groupField,
                 $reviewDate,
                 $reviewFrequency
-            )->addExtraClass("custom-settings"),
+            )->addExtraClass("review-settings"),
             ReadonlyField::create("ROContentOwners", _t("ContentReview.CONTENTOWNERS", "Content Owners"), $this->getOwnerNames()),
-            ReadonlyField::create("RONextReviewDate", _t("ContentReview.NEXTREVIEWDATE", "Next review date"), $this->owner->NextReviewDate),
             $notesField,
         ));
     }
@@ -504,19 +510,6 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
             $nextReviewUnixSec = strtotime(" + " . $this->owner->ReviewPeriodDays . " days", SS_Datetime::now()->format("U"));
             $this->owner->NextReviewDate = date("Y-m-d", $nextReviewUnixSec);
         }
-
-        //
-        if ($this->owner->isChanged("NextReviewDate", 2)) {
-            $children = $this->owner->stageChildren(true)->filter("ContentReviewType", "Inherit");
-            $compatibility = ContentReviewCompatability::start();
-
-            foreach ($children as $child) {
-                $child->NextReviewDate = $this->owner->NextReviewDate;
-                $child->write();
-            }
-
-            ContentReviewCompatability::done($compatibility);
-        }
     }
 
     private function setDefaultReviewDateForDisabled()
@@ -526,6 +519,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 
     protected function setDefaultReviewDateForCustom()
     {
+		// Don't overwrite existing value
         if ($this->owner->NextReviewDate) {
             return;
         }
@@ -542,11 +536,16 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 
     protected function setDefaultReviewDateForInherited()
     {
+		// Don't overwrite existing value
+		if($this->owner->NextReviewDate) {
+			return;
+		}
+
         $options = $this->getOptions();
         $nextDate = null;
 
-        if ($options && $this->owner->parent()->exists()) {
-            $nextDate = $this->getReviewDate($this->owner->parent());
+        if ($options instanceof SiteTree) {
+            $nextDate = $this->getReviewDate($options);
         } elseif ($options instanceof SiteConfig) {
             $nextDate = $this->getReviewDate();
         }

javascript/contentreview.js

@@ -40,16 +40,15 @@ jQuery(function($) {
 			},
 			
 			_custom: function() {
-				$('.custom-settings').show();
-				$('.inherited-settings').hide();	
+				$('.review-settings').show();
+				$('.field.custom-setting').show();
 			}, 
 			_inherited: function() {
-				$('.inherited-settings').show();	
-				$('.custom-settings').hide();
+				$('.review-settings').show();
+				$('.field.custom-setting').hide();
 			},
 			_disabled: function() {
-				$('.inherited-settings').hide();	
-				$('.custom-settings').hide();
+				$('.review-settings').hide();
 			}
 		});	
 		

tests/ContentReviewSettingsTest.php

@@ -68,17 +68,19 @@ class ContentReviewSettingsTest extends SapphireTest
 
     public function testAdvanceReviewFromInheritedSettings()
     {
-        /** @var Page|SiteTreeContentReview $page */
-        $page = $this->objFromFixture("Page", "page-1-1");
-
-        /** @var Page|SiteTreeContentReview $parentPage */
+		// When a parent page is advanced, the next review date of the child is not automatically advanced
         $parentPage = $this->objFromFixture("Page", "page-1");
+		$this->assertTrue($parentPage->advanceReviewDate());
+		$parentPage->write();
 		
+        $page = $this->objFromFixture("Page", "page-1-1");
+		$this->assertEquals(date("Y-m-d", strtotime("now + 5 days")), $parentPage->NextReviewDate);
+		$this->assertEquals('2011-04-12', $page->NextReviewDate);
+
+		// When a sub page is advanced, the next review date is advanced by the number of days in the parent
         $this->assertTrue($page->advanceReviewDate());
-
         $page->write();
-
-        $this->assertEquals(date("Y-m-d", strtotime("now + " . $parentPage->ReviewPeriodDays . " days")), $page->NextReviewDate);
+        $this->assertEquals(date("Y-m-d", strtotime("now + 5 days")), $page->NextReviewDate);
     }
 
     public function testAdvanceReviewFromInheritedSiteConfigSettings()
@@ -183,19 +185,11 @@ class ContentReviewSettingsTest extends SapphireTest
 
     public function testGetNextReviewDateFromPageInheritedSetting()
     {
-        /** @var Page|SiteTreeContentReview $page */
+		// Although page-1-1 inherits from page-1, it has an independent review date
         $page = $this->objFromFixture("Page", "page-1-1");
-
         $nextReviewDate = $page->getReviewDate();
-
         $this->assertInstanceOf("Date", $nextReviewDate);
-
-        /** @var Page|SiteTreeContentReview $nextPage */
-        $nextPage = $this->objFromFixture("Page", "page-1");
-
-        $expected = $nextPage->NextReviewDate;
-
-        $this->assertEquals($expected, $nextReviewDate->format("Y-m-d"));
+        $this->assertEquals('2011-04-12', $nextReviewDate->format("Y-m-d"));
     }
 
     public function testUpdateNextReviewDateFromCustomToDisabled()
@@ -248,13 +242,10 @@ class ContentReviewSettingsTest extends SapphireTest
         /** @var Page|SiteTreeContentReview $childPage */
         $childPage = $this->objFromFixture("Page", "page-1-1");
 
-        // BEFORE: parent page have a period of five days, so childPage should have a 
-        // review date LastEdited + 5 days
-        $this->assertEquals($parentPage->NextReviewDate, $childPage->NextReviewDate);
+		// Parent and child pages have different review dates
+        $this->assertNotEquals($parentPage->NextReviewDate, $childPage->NextReviewDate);
 
-        $oldChildDate = $childPage->NextReviewDate;
-        // But if we change the parent page ReviewPeriodDays to 10, the childs should 
-        // change as well
+        // But if we change the parent page ReviewPeriodDays to 10, the childs stays the same
         $parentPage->ReviewPeriodDays = 10;
         $parentPage->write();
 
@@ -267,11 +258,14 @@ class ContentReviewSettingsTest extends SapphireTest
         /** @var Page|SiteTreeContentReview $page */
         $childPage = $this->objFromFixture("Page", "page-1-1");
 
-        // AFTER: parent page have a period of 10 days, so childPage should have
-        // a review date now + 10 days.
-        $this->assertNotEquals($oldChildDate, $childPage->NextReviewDate);
+        // The parent page's date advances, but not the child's
+        $this->assertEquals('2011-04-12', $childPage->NextReviewDate);
+        $this->assertEquals($this->addDaysToDate(date("Y-m-d"), 10), $parentPage->NextReviewDate);
+
+		// Reviewing the child page should, however, advance its review by 10 days
+		$childPage->advanceReviewDate();
+		$childPage->write();
         $this->assertEquals($this->addDaysToDate(date("Y-m-d"), 10), $childPage->NextReviewDate);
-        $this->assertEquals($parentPage->NextReviewDate, $childPage->NextReviewDate);
     }
 
     /**

tests/ContentReviewSettingsTest.yml

@@ -65,6 +65,8 @@ Page:
   page-1-1:
     Title: page 1 1
     ContentReviewType: Inherit
+    ReviewPeriodDays: 1
+    NextReviewDate: 2011-04-12
     ParentID: =>Page.page-1
   page-2:
     Title: page 2