Improving test coverage

code/extensions/ContentReviewCMSPageEditController.php

@@ -66,16 +66,17 @@ class ContentReviewCMSPageEditController extends LeftAndMainExtension {
 			throw new SS_HTTPResponse_Exception("No record ID", 404);
 		}
 		$SQL_id = Convert::raw2sql($data['ID']);
-		$record = SiteTree::get()->byID($SQL_id);
+		$page = SiteTree::get()->byID($SQL_id);
-		if($record && !$record->canEdit()) {
+		if($page && !$page->canEdit()) {
 			return Security::permissionFailure($this);
 		}
-		if(!$record || !$record->ID) {
+		if(!$page || !$page->ID) {
 			throw new SS_HTTPResponse_Exception("Bad record ID #$SQL_id", 404);
 		}
 		
-		$record->ReviewNotes = $data['ReviewNotes'];
+		$page->addReviewNote(Member::currentUser(), $data['ReviewNotes']);
-		$record->write();
+		$page->advanceReviewDate();
-		return $this->owner->redirect($this->owner->Link('show/'.$SQL_id));
+		
+		return $this->owner->redirect($this->owner->Link('show/'.$page->ID));
 	}
 }

code/extensions/SiteTreeContentReview.php

@@ -14,11 +14,18 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 	private static $db = array(
 		"ReviewPeriodDays" => "Int",
 		"NextReviewDate" => "Date",
-		'ReviewNotes' => 'Text',
 		'LastEditedByName' => 'Varchar(255)',
 		'OwnerNames' => 'Varchar(255)'
 	);
 	
+	/**
+	 *
+	 * @var array
+	 */
+	private static $has_many = array(
+		'ReviewLogs' => 'ContentReviewLog' 
+	);
+
 	/**
 	 *
 	 * @var array
@@ -34,11 +41,11 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 	 */
 	public function getOwnerNames() {
 		$names = array();
-		foreach($this->DirectGroups() as $group) {
+		foreach($this->OwnerGroups() as $group) {
 			$names[] = $group->Title;
 		}
 		
-		foreach($this->DirectUsers() as $group) {
+		foreach($this->OwnerUsers() as $group) {
 			$names[] = $group->getName();
 		}
 		return implode(', ', $names);
@@ -63,11 +70,9 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 	 * @return \ArrayList
 	 */
 	public function ContentReviewOwners() {
-		
 		$contentReviewOwners = new ArrayList();
-		
+		$toplevelGroups = $this->OwnerGroups();
-		$toplevelGroups = $this->DirectGroups();
+		if($toplevelGroups->count()) {
-		if($toplevelGroups) {
 			$groupIDs = array();
 			foreach($toplevelGroups as $group) {
 				$familyIDs = $group->collateFamilyIDs();
@@ -75,15 +80,15 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 					$groupIDs = array_merge($groupIDs, array_values($familyIDs));
 				}
 			}
+			array_unique($groupIDs);
 			if(count($groupIDs)) {
 				$groupMembers = DataObject::get('Member')->where("\"Group\".\"ID\" IN (" . implode(",",$groupIDs) . ")")
 				->leftJoin("Group_Members", "\"Member\".\"ID\" = \"Group_Members\".\"MemberID\"")
 				->leftJoin("Group", "\"Group_Members\".\"GroupID\" = \"Group\".\"ID\"");
 				$contentReviewOwners->merge($groupMembers);
 			}
-			
 		}
-		$contentReviewOwners->merge($this->DirectUsers());
+		$contentReviewOwners->merge($this->OwnerUsers());
 		$contentReviewOwners->removeDuplicates();
 		return $contentReviewOwners;
 	}
@@ -91,14 +96,14 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 	/**
 	 * @return ManyManyList
 	 */
-	public function DirectGroups() {
+	public function OwnerGroups() {
 		return $this->owner->getManyManyComponents('ContentReviewGroups');
 	}
 	
 	/**
 	 * @return ManyManyList
 	 */
-	public function DirectUsers() {
+	public function OwnerUsers() {
 		return $this->owner->getManyManyComponents('ContentReviewUsers');
 	}
 
@@ -120,7 +125,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 		}
 		asort($usersMap);
 		
-		$userField = ListboxField::create('DirectUsers', _t("ContentReview.PAGEOWNERUSERS", "Users"))
+		$userField = ListboxField::create('OwnerUsers', _t("ContentReview.PAGEOWNERUSERS", "Users"))
 			->setMultiple(true)
 			->setSource($usersMap)
 			->setAttribute('data-placeholder', _t('ContentReview.ADDUSERS', 'Add users'))
@@ -132,7 +137,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 			$groupsMap[$group->ID] = $group->getBreadcrumbs(' > ');
 		}
 		asort($groupsMap);
-		$groupField = ListboxField::create('DirectGroups', _t("ContentReview.PAGEOWNERGROUPS", "Groups"))
+		$groupField = ListboxField::create('OwnerGroups', _t("ContentReview.PAGEOWNERGROUPS", "Groups"))
 			->setMultiple(true)
 			->setSource($groupsMap)
 			->setAttribute('data-placeholder', _t('ContentReview.ADDGROUP', 'Add groups'))
@@ -163,7 +168,7 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 			)
 		)->setDescription(_t('ContentReview.REVIEWFREQUENCYDESCRIPTION', 'The review date will be set to this far in the future whenever the page is published'));
 		
-		$notesField = TextareaField::create('ReviewNotes', 'Review Notes');
+		$notesField = GridField::create('ReviewNotes', 'Review Notes', $this->owner->ReviewLogs());
 		
 		$fields->addFieldsToTab("Root.Review", array(
 			new HeaderField(_t('ContentReview.REVIEWHEADER', "Content review"), 2),
@@ -175,6 +180,37 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 		));
 	}
 	
+	/**
+	 * Creates a ContentReviewLog and connects it to this Page
+	 * 
+	 * @param Member $reviewer
+	 * @param string $message
+	 */
+	public function addReviewNote(Member $reviewer, $message) {
+		$reviewLog = ContentReviewLog::create();
+		$reviewLog->Note = $message;
+		$reviewLog->MemberID = $reviewer->ID;
+		$this->owner->ReviewLogs()->add($reviewLog);
+	}
+	
+	/**
+	 * Advance review date to the next date based on review period or set it to null
+	 * if there is no schedule
+	 * 
+	 * @return bool - returns true if date was set and false is content review is 'off'
+	 */
+	public function advanceReviewDate() {
+		$hasNextReview = true;
+		if($this->owner->ReviewPeriodDays) {
+			$this->owner->NextReviewDate = date('Y-m-d', strtotime('+' . $this->owner->ReviewPeriodDays . ' days'));
+		} else {
+			$hasNextReview = false;
+			$this->owner->NextReviewDate = null;
+		}
+		$this->owner->write();
+		return $hasNextReview;
+	}
+	
 	/**
 	 * 
 	 * @param \FieldList $actions
@@ -201,13 +237,13 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 		if($this->owner->obj('NextReviewDate')->InFuture()) {
 			return false;
 		}
-		if($this->DirectGroups()->count() == 0 && $this->DirectUsers()->count() == 0) {
+		if($this->OwnerGroups()->count() == 0 && $this->OwnerUsers()->count() == 0) {
 			return false;
 		}
-		if($member->inGroups($this->DirectGroups())) {
+		if($member->inGroups($this->OwnerGroups())) {
 			return true;
 		}
-		if($this->DirectUsers()->find('ID', $member->ID)) {
+		if($this->OwnerUsers()->find('ID', $member->ID)) {
 			return true;
 		}
 		return false;
@@ -217,9 +253,6 @@ class SiteTreeContentReview extends DataExtension implements PermissionProvider
 	 * Set the review data from the review period, if set.
 	 */
 	public function onBeforeWrite() {
-		if($this->owner->ReviewPeriodDays && !$this->owner->NextReviewDate) {
-			$this->owner->NextReviewDate = date('Y-m-d', strtotime('+' . $this->owner->ReviewPeriodDays . ' days'));
-		}
 		$this->owner->LastEditedByName=$this->owner->getEditorName();
 		$this->owner->OwnerNames = $this->owner->getOwnerNames();
 	}

code/models/ContentReviewLog.php

@@ -0,0 +1,30 @@
+<?php
+
+class ContentReviewLog extends DataObject {
+	
+	/**
+	 *
+	 * @var array
+	 */
+	private static $db = array(
+		'Note' => 'Text'
+	);
+	
+	/**
+	 *
+	 * @var array
+	 */
+	private static $has_one = array(
+		'Reviewer' => 'Member',
+		'SiteTree' => 'SiteTree'
+	);
+	
+	/**
+	 *
+	 * @var array
+	 */
+	private static $summary_fields = array(
+		'Note',
+		'Created'
+	);
+}

tests/ContentReviewTest.php

@@ -2,33 +2,8 @@
 
 class ContentReviewTest extends FunctionalTest {
 
-	/**
-	 *
-	 * @var string
-	 */
 	public static $fixture_file = 'contentreview/tests/ContentReviewTest.yml';
 	
-	public function testPermissionsExists() {
-		$perms = singleton('SiteTreeContentReview')->providePermissions();
-		$this->assertTrue(isset($perms['EDIT_CONTENT_REVIEW_FIELDS']));
-	}
-	
-	public function testUserWithPermissionCanEdit() {
-		$editor = $this->objFromFixture('Member', 'editor');
-		$this->logInAs($editor);
-		$page = new Page();
-		$fields = $page->getCMSFields();
-		$this->assertNotNull($fields->fieldByName('Root.Review'));
-	}
-	
-	public function testUserWithoutPermissionCannotEdit() {
-		$author = $this->objFromFixture('Member', 'author');
-		$this->logInAs($author);
-		$page = new Page();
-		$fields = $page->getCMSFields();
-		$this->assertNull($fields->fieldByName('Root.Review'));
-	}
-	
 	public function testContentReviewEmails() {
 		SS_Datetime::set_mock_now('2010-02-14 12:00:00');
 		
@@ -40,17 +15,6 @@ class ContentReviewTest extends FunctionalTest {
 		SS_Datetime::clear_mock_now();
 	}
 	
-	public function testAutomaticallySettingReviewDate() {
-		$editor = $this->objFromFixture('Member', 'editor');
-		$this->logInAs($editor);
-		
-		$page = new Page();
-		$page->ReviewPeriodDays = 10;
-		$page->write();
-		$this->assertTrue($page->doPublish());		
-		$this->assertEquals(date('Y-m-d', strtotime('now + 10 days')), $page->NextReviewDate);
-	}
-	
 	public function testReportContent() {
 		$editor = $this->objFromFixture('Member', 'editor');
 		$this->logInAs($editor);
@@ -102,52 +66,4 @@ class ContentReviewTest extends FunctionalTest {
 		$this->assertTrue($page->doPublish());
 		$this->assertEquals('', $page->OwnerNames);
 	}
-	
-	public function testCanNotBeReviewBecauseNoReviewDate() {
-		SS_Datetime::set_mock_now('2010-01-01 12:00:00');
-		$author = $this->objFromFixture('Member', 'author');
-		$page = $this->objFromFixture('Page', 'no-review');
-		// page 'no-review' is owned by author, but there is no review date
-		$this->assertFalse($page->canBeReviewedBy($author));
-	}
-	
-	public function testCanNotBeReviewedBecauseInFuture() {
-		SS_Datetime::set_mock_now('2010-01-01 12:00:00');
-		$author = $this->objFromFixture('Member', 'author');
-		$page = $this->objFromFixture('Page', 'staff');
-		// page 'staff' is owned by author, but the review date is in the future
-		$this->assertFalse($page->canBeReviewedBy($author));
-	}
-	
-	public function testCanNotBeReviewedByUser() {
-		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
-		$author = $this->objFromFixture('Member', 'author');
-		$page = $this->objFromFixture('Page', 'home');
-		// page 'home' doesnt have any owners
-		$this->assertFalse($page->canBeReviewedBy($author));
-	}
-	
-	public function testCanBeReviewedByUser() {
-		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
-		$author = $this->objFromFixture('Member', 'author');
-		$page = $this->objFromFixture('Page', 'staff');
-		// page 'staff' is owned by author
-		$this->assertTrue($page->canBeReviewedBy($author));
-	}
-	
-	public function testCanNotBeReviewedByGroup() {
-		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
-		$author = $this->objFromFixture('Member', 'editor');
-		$page = $this->objFromFixture('Page', 'contact');
-		// page 'contact' is owned by the authorgroup
-		$this->assertFalse($page->canBeReviewedBy($author));
-	}
-	
-	public function testCanBeReviewedByGroup() {
-		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
-		$author = $this->objFromFixture('Member', 'author');
-		$page = $this->objFromFixture('Page', 'contact');
-		// page 'contact' is owned by the authorgroup
-		$this->assertTrue($page->canBeReviewedBy($author));
-	}
 }

tests/ContentReviewTest.yml

@@ -50,3 +50,6 @@ Page:
     no-review:
         Title: Page without review date
         ContentReviewUsers: =>Member.author
+    group-owned:
+        Title: Page owned by group
+        ContentReviewGroups: =>Group.authorgroup

tests/SiteTreeContentReviewTest.php

@@ -0,0 +1,140 @@
+<?php
+
+class SiteTreeContentReviewTest extends FunctionalTest {
+	
+	public static $fixture_file = 'contentreview/tests/ContentReviewTest.yml';
+	
+	public function testPermissionsExists() {
+		$perms = singleton('SiteTreeContentReview')->providePermissions();
+		$this->assertTrue(isset($perms['EDIT_CONTENT_REVIEW_FIELDS']));
+	}
+	
+	public function testUserWithPermissionCanEdit() {
+		$editor = $this->objFromFixture('Member', 'editor');
+		$this->logInAs($editor);
+		$page = new Page();
+		$fields = $page->getCMSFields();
+		$this->assertNotNull($fields->fieldByName('Root.Review'));
+	}
+	
+	public function testUserWithoutPermissionCannotEdit() {
+		$author = $this->objFromFixture('Member', 'author');
+		$this->logInAs($author);
+		$page = new Page();
+		$fields = $page->getCMSFields();
+		$this->assertNull($fields->fieldByName('Root.Review'));
+	}
+	
+	public function testAutomaticallyToNotSetReviewDate() {
+		$editor = $this->objFromFixture('Member', 'editor');
+		$this->logInAs($editor);
+		
+		$page = new Page();
+		$page->ReviewPeriodDays = 10;
+		$page->write();
+		$this->assertTrue($page->doPublish());		
+		$this->assertEquals(null, $page->NextReviewDate);
+	}
+	
+	public function testSetReviewDate10Days() {
+		$page = new Page();
+		$page->ReviewPeriodDays = 10;
+		$this->assertTrue($page->advanceReviewDate());
+		$page->write();
+		$this->assertEquals(date('Y-m-d', strtotime('now + 10 days')), $page->NextReviewDate);
+	}
+	
+	public function testSetReviewDateNull() {
+		$page = new Page();
+		$page->ReviewPeriodDays = 0;
+		$this->assertFalse($page->advanceReviewDate());
+		$page->write();
+		$this->assertEquals(null, $page->NextReviewDate);
+	}
+	
+	public function testAddReviewNote() {
+		$author = $this->objFromFixture('Member', 'author');
+		$page = $this->objFromFixture('Page', 'home');
+		$page->addReviewNote($author, 'This is a message');
+		
+		// Get the page again to make sure it's not only cached in memory
+		$homepage = $this->objFromFixture('Page', 'home');
+		$this->assertEquals(1, $homepage->ReviewLogs()->count());
+		$this->assertEquals('This is a message', $homepage->ReviewLogs()->first()->Note);
+	}
+	
+	public function testGetContentReviewOwners() {
+		$page = $this->objFromFixture('Page', 'group-owned');
+		$owners = $page->ContentReviewOwners();
+		$this->assertEquals(1, $owners->count());
+		$this->assertEquals('author@example.com', $owners->first()->Email);
+	}
+	
+	public function testCanNotBeReviewBecauseNoReviewDate() {
+		SS_Datetime::set_mock_now('2010-01-01 12:00:00');
+		$author = $this->objFromFixture('Member', 'author');
+		$page = $this->objFromFixture('Page', 'no-review');
+		// page 'no-review' is owned by author, but there is no review date
+		$this->assertFalse($page->canBeReviewedBy($author));
+	}
+	
+	public function testCanNotBeReviewedBecauseInFuture() {
+		SS_Datetime::set_mock_now('2010-01-01 12:00:00');
+		$author = $this->objFromFixture('Member', 'author');
+		$page = $this->objFromFixture('Page', 'staff');
+		// page 'staff' is owned by author, but the review date is in the future
+		$this->assertFalse($page->canBeReviewedBy($author));
+	}
+	
+	public function testCanNotBeReviewedByUser() {
+		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
+		$author = $this->objFromFixture('Member', 'author');
+		$page = $this->objFromFixture('Page', 'home');
+		// page 'home' doesnt have any owners
+		$this->assertFalse($page->canBeReviewedBy($author));
+	}
+	
+	public function testCanBeReviewedByUser() {
+		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
+		$author = $this->objFromFixture('Member', 'author');
+		$page = $this->objFromFixture('Page', 'staff');
+		// page 'staff' is owned by author
+		$this->assertTrue($page->canBeReviewedBy($author));
+	}
+	
+	public function testCanNotBeReviewedByGroup() {
+		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
+		$author = $this->objFromFixture('Member', 'editor');
+		$page = $this->objFromFixture('Page', 'contact');
+		// page 'contact' is owned by the authorgroup
+		$this->assertFalse($page->canBeReviewedBy($author));
+	}
+	
+	public function testCanBeReviewedByGroup() {
+		SS_Datetime::set_mock_now('2010-03-01 12:00:00');
+		$author = $this->objFromFixture('Member', 'author');
+		$page = $this->objFromFixture('Page', 'contact');
+		// page 'contact' is owned by the authorgroup
+		$this->assertTrue($page->canBeReviewedBy($author));
+	}
+	
+	public function testReviewActionVisibleForAuthor() {
+		SS_Datetime::set_mock_now('2020-03-01 12:00:00');
+		$page = $this->objFromFixture('Page', 'contact');
+		$author = $this->objFromFixture('Member', 'author');
+		$this->logInAs($author);
+
+		$fields = $page->getCMSActions();
+		$this->assertNotNull($fields->fieldByName('action_reviewed'));
+	}
+	
+	public function testReviewActionNotVisibleForEditor() {
+		SS_Datetime::set_mock_now('2020-03-01 12:00:00');
+		$page = $this->objFromFixture('Page', 'contact');
+		$author = $this->objFromFixture('Member', 'editor');
+		$this->logInAs($author);
+		
+		$fields = $page->getCMSActions();
+		$this->assertNull($fields->fieldByName('action_reviewed'));
+	}
+}