Merge pull request #49 from creative-commoners/pulls/2.0/update-tests

FIX Use cached SiteTree object for history instead of creating a temporary object

README.md

@@ -22,7 +22,7 @@ Install with composer by running `composer require silverstripe/versionfeed` in
 
 For usage instructions see [user manual](docs/en/userguide/index.md).
 
-### Translations
+## Translations
 
 Translations of the natural language strings are managed through a third party translation interface, transifex.com. Newly added strings will be periodically uploaded there for translation, and any new translations will be merged back to the project source code.
 

docs/en/developer.md

@@ -23,8 +23,10 @@ for the current page. You can override this behaviour by defining the `getDefaul
 and returning the URL of your desired RSS feed:
 
 ```php
-class MyPage extends Page {
+class MyPage extends Page
-	function getDefaultRSSLink() {
+{
+	public function getDefaultRSSLink()
+	{
 		return $this->Link('myrssfeed');
 	}
 }
@@ -34,7 +36,7 @@ This can be used in templates as `$DefaultRSSLink`.
 
 ### Rate limiting and caching
 
-By default all content is filtered based on the rules specified in `versionfeed/_config/versionfeed.yml`.
+By default all content is filtered based on the rules specified in `vendor/silverstripe/versionfeed/_config/versionfeed.yml`.
 Two filters are applied on top of one another:
 
  * `SilverStripe\VersionFeed\Filters\CachedContentFilter` provides caching of versions based on an identifier built up of the record ID and the 

lang/en.yml

@@ -9,4 +9,3 @@ en:
   SilverStripe\VersionFeed\VersionFeedSiteConfig:
     ALLCHANGES: 'All page changes'
     ALLCHANGESLABEL: 'Make global changes feed public'
-    Warning: 'Publicising the history will also disclose the changes that have at the time been protected from the public view.'

src/VersionFeed.php

@@ -2,16 +2,16 @@
 
 namespace SilverStripe\VersionFeed;
 
-use SilverStripe\ORM\FieldType\DBField;
+use SilverStripe\CMS\Model\SiteTreeExtension;
-use SilverStripe\View\Parsers\Diff;
-use SilverStripe\ORM\ArrayList;
-use SilverStripe\Forms\FieldList;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Forms\CheckboxField;
 use SilverStripe\Forms\FieldGroup;
-use SilverStripe\Forms\LiteralField;
+use SilverStripe\Forms\FieldList;
+use SilverStripe\ORM\ArrayList;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\FieldType\DBField;
 use SilverStripe\SiteConfig\SiteConfig;
-use SilverStripe\CMS\Model\SiteTreeExtension;
+use SilverStripe\View\Parsers\Diff;
 
 class VersionFeed extends SiteTreeExtension
 {
@@ -146,7 +146,7 @@ class VersionFeed extends SiteTreeExtension
      * Return a single diff representing this version.
      * Returns the initial version if there is nothing to compare to.
      *
-     * @returns DataObject Object with relevant fields diffed.
+     * @return DataObject|null Object with relevant fields diffed.
      */
     public function getDiff()
     {
@@ -179,32 +179,34 @@ class VersionFeed extends SiteTreeExtension
 
     public function updateSettingsFields(FieldList $fields)
     {
-        if (!Config::inst()->get(get_class(), 'changes_enabled')) {
+        if (!$this->owner->config()->get('changes_enabled')) {
             return;
         }
         
         // Add public history field.
-        $fields->addFieldToTab('Root.Settings', $publicHistory = new FieldGroup(
+        $fields->addFieldToTab(
-            new CheckboxField('PublicHistory', $this->owner->fieldLabel('PublicHistory'))
+            'Root.Settings',
-        ));
+            $publicHistory = FieldGroup::create(
-
+                CheckboxField::create('PublicHistory', $this->owner->fieldLabel('PublicHistory'))
-        $warning = _t(
+            )
+                ->setDescription(_t(
                     __CLASS__ . '.Warning',
-            "Publicising the history will also disclose the changes that have at the time been protected " .
+                    "Publicising the history will also disclose the changes that have at the "
-            "from the public view."
+                    . "time been protected from the public view."
+                ))
         );
 
-        $fields->addFieldToTab('Root.Settings', new LiteralField('PublicHistoryWarning', $warning), 'PublicHistory');
-
         if ($this->owner->CanViewType != 'Anyone') {
-            $warning = _t(
+            $canViewType = $fields->fieldByName('Root.Settings.CanViewType');
+            if ($canViewType) {
+                $canViewType->setDescription(_t(
                     __CLASS__ . '.Warning2',
-                "Changing access settings in such a way that this page or pages under it become publicly<br>" .
+                    "Changing access settings in such a way that this page or pages under it become publicly<br>"
-                "accessible may result in publicising all historical changes on these pages too. Please review<br>" .
+                    . "accessible may result in publicising all historical changes on these pages too. Please review"
-                "this section's \"Public history\" settings to ascertain only intended information is disclosed."
+                    . "<br> this section's \"Public history\" settings to ascertain only intended information is "
-            );
+                    . "disclosed."
-
+                ));
-            $fields->addFieldToTab('Root.Settings', new LiteralField('PublicHistoryWarning2', $warning), 'CanViewType');
+            }
         }
     }
 

src/VersionFeedController.php

@@ -4,6 +4,8 @@ namespace SilverStripe\VersionFeed;
 
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Control\RSS\RSSFeed;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Security\Security;
 use SilverStripe\SiteConfig\SiteConfig;
 use SilverStripe\ORM\DB;
 use SilverStripe\Security\Member;
@@ -118,7 +120,7 @@ class VersionFeedController extends Extension
             // Cache the diffs to remove DOS possibility.
             $key = 'allchanges'
                 . preg_replace('#[^a-zA-Z0-9_]#', '', $lastChange['LastEdited'])
-                . (Member::currentUserID() ?: 'public');
+                . (Security::getCurrentUser() ? Security::getCurrentUser()->ID : 'public');
             $changeList = $this->filterContent($key, function () use ($latestChanges) {
                 $changeList = new ArrayList();
                 $canView = array();
@@ -127,14 +129,15 @@ class VersionFeedController extends Extension
                     // WARNING: although we are providing historical details, we check the current configuration.
                     $id = $record['RecordID'];
                     if (!isset($canView[$id])) {
-                        $page = SiteTree::get()->byID($id);
+                        $page = DataObject::get_by_id(SiteTree::class, $id);
-                        $canView[$id] = $page && $page->canView(new Member());
+                        $canView[$id] = $page && $page->canView(Security::getCurrentUser());
                     }
                     if (!$canView[$id]) {
                         continue;
                     }
 
                     // Get the diff to the previous version.
+                    $record['ID'] = $record['RecordID'];
                     $version = SiteTree::create($record);
                     if ($diff = $version->getDiff()) {
                         $changeList->push($diff);

src/VersionFeedSiteConfig.php

@@ -2,11 +2,10 @@
 
 namespace SilverStripe\VersionFeed;
 
-use SilverStripe\Forms\FieldList;
 use SilverStripe\Core\Config\Config;
-use SilverStripe\VersionFeed\VersionFeed;
 use SilverStripe\Forms\CheckboxField;
 use SilverStripe\Forms\FieldGroup;
+use SilverStripe\Forms\FieldList;
 use SilverStripe\ORM\DataExtension;
 
 /**
@@ -39,7 +38,7 @@ class VersionFeedSiteConfig extends DataExtension
             FieldGroup::create(new CheckboxField('AllChangesEnabled', $this->owner->fieldLabel('AllChangesEnabled')))
                 ->setTitle(_t(__CLASS__ . '.ALLCHANGES', 'All page changes'))
                 ->setDescription(_t(
-                    __CLASS__ . '.Warning',
+                    'SilverStripe\\VersionFeed\\VersionFeed.Warning',
                     "Publicising the history will also disclose the changes that have at the time been protected " .
                     "from the public view."
                 ))

tests/VersionFeedFunctionalTest.php

@@ -3,19 +3,18 @@
 namespace SilverStripe\VersionFeed\Tests;
 
 use Page;
-
+use Psr\SimpleCache\CacheInterface;
-use SilverStripe\VersionFeed\VersionFeed;
+use SilverStripe\CMS\Model\SiteTree;
-use SilverStripe\VersionFeed\Filters\CachedContentFilter;
+use SilverStripe\Control\Director;
-use SilverStripe\VersionFeed\Filters\RateLimitFilter;
-use SilverStripe\VersionFeed\VersionFeedController;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Injector\Injector;
-use SilverStripe\CMS\Model\SiteTree;
-use SilverStripe\Versioned\Versioned;
-use SilverStripe\SiteConfig\SiteConfig;
 use SilverStripe\Dev\FunctionalTest;
-use SilverStripe\Control\Director;
+use SilverStripe\SiteConfig\SiteConfig;
-use Psr\SimpleCache\CacheInterface;
+use SilverStripe\Versioned\Versioned;
+use SilverStripe\VersionFeed\Filters\CachedContentFilter;
+use SilverStripe\VersionFeed\Filters\RateLimitFilter;
+use SilverStripe\VersionFeed\VersionFeed;
+use SilverStripe\VersionFeed\VersionFeedController;
 
 class VersionFeedFunctionalTest extends FunctionalTest
 {
@@ -55,7 +54,7 @@ class VersionFeedFunctionalTest extends FunctionalTest
         Config::modify()->set(RateLimitFilter::class, 'lock_cooldown', false);
     }
 
-    public function tearDown()
+    protected function tearDown()
     {
         Director::config()->set('alternate_base_url', null);
         
@@ -69,24 +68,37 @@ class VersionFeedFunctionalTest extends FunctionalTest
         $page = $this->createPageWithChanges(array('PublicHistory' => false));
 
         $response = $this->get($page->RelativeLink('changes'));
-        $this->assertEquals(404, $response->getStatusCode());
+        $this->assertEquals(
+            404,
+            $response->getStatusCode(),
+            'With Page\'s "PublicHistory" disabled, `changes` action response code should be 404'
+        );
 
         $response = $this->get($page->RelativeLink('allchanges'));
         $this->assertEquals(200, $response->getStatusCode());
         $xml = simplexml_load_string($response->getBody());
-        $this->assertFalse((bool)$xml->channel->item);
+        $this->assertFalse(
+            (bool)$xml->channel->item,
+            'With Page\'s "PublicHistory" disabled, `allchanges` action should not have an item in the channel'
+        );
 
         $page = $this->createPageWithChanges(array('PublicHistory' => true));
 
         $response = $this->get($page->RelativeLink('changes'));
         $this->assertEquals(200, $response->getStatusCode());
         $xml = simplexml_load_string($response->getBody());
-        $this->assertTrue((bool)$xml->channel->item);
+        $this->assertTrue(
+            (bool)$xml->channel->item,
+            'With Page\'s "PublicHistory" enabled, `changes` action should have an item in the channel'
+        );
 
         $response = $this->get($page->RelativeLink('allchanges'));
         $this->assertEquals(200, $response->getStatusCode());
         $xml = simplexml_load_string($response->getBody());
-        $this->assertTrue((bool)$xml->channel->item);
+        $this->assertTrue(
+            (bool)$xml->channel->item,
+            'With "PublicHistory" enabled, `allchanges` action should have an item in the channel'
+        );
     }
 
     public function testRateLimiting()
@@ -149,7 +161,7 @@ class VersionFeedFunctionalTest extends FunctionalTest
         Config::modify()->set(CachedContentFilter::class, 'cache_enabled', false);
     }
 
-    public function testContainsChangesForPageOnly()
+    public function testChangesActionContainsChangesForCurrentPageOnly()
     {
         $page1 = $this->createPageWithChanges(array('Title' => 'Page1'));
         $page2 = $this->createPageWithChanges(array('Title' => 'Page2'));
@@ -173,7 +185,7 @@ class VersionFeedFunctionalTest extends FunctionalTest
         $this->assertContains('Changed: Page2', $titles);
     }
 
-    public function testContainsAllChangesForAllPages()
+    public function testAllChangesActionContainsAllChangesForAllPages()
     {
         $page1 = $this->createPageWithChanges(array('Title' => 'Page1'));
         $page2 = $this->createPageWithChanges(array('Title' => 'Page2'));
@@ -181,7 +193,7 @@ class VersionFeedFunctionalTest extends FunctionalTest
         $response = $this->get($page1->RelativeLink('allchanges'));
         $xml = simplexml_load_string($response->getBody());
         $titles = array_map(function ($item) {
-            return (string)$item->title;
+            return str_replace('Changed: ', '', (string) $item->title);
         }, $xml->xpath('//item'));
         $this->assertContains('Page1', $titles);
         $this->assertContains('Page2', $titles);
@@ -197,21 +209,21 @@ class VersionFeedFunctionalTest extends FunctionalTest
         ), $seed);
         $page->update($seed);
         $page->write();
-        $page->publish('Stage', 'Live');
+        $page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE);
 
         $page->update(array(
             'Title' => 'Changed: ' . $seed['Title'],
             'Content' => 'Changed: ' . $seed['Content'],
         ));
         $page->write();
-        $page->publish('Stage', 'Live');
+        $page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE);
 
         $page->update(array(
             'Title' => 'Changed again: ' . $seed['Title'],
             'Content' => 'Changed again: ' . $seed['Content'],
         ));
         $page->write();
-        $page->publish('Stage', 'Live');
+        $page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE);
 
         $page->update(array(
             'Title' => 'Unpublished: ' . $seed['Title'],
@@ -223,7 +235,7 @@ class VersionFeedFunctionalTest extends FunctionalTest
     }
 
     /**
-     * Tests response code for globally disabled feedss
+     * Tests response code for globally disabled feeds
      */
     public function testFeedViewability()
     {

tests/VersionFeedTest.php

@@ -3,15 +3,15 @@
 namespace SilverStripe\VersionFeed\Tests;
 
 use Page;
+use SilverStripe\CMS\Controllers\ContentController;
+use SilverStripe\CMS\Model\SiteTree;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Versioned\Versioned;
 use SilverStripe\VersionFeed\VersionFeed;
 use SilverStripe\VersionFeed\VersionFeedController;
-use SilverStripe\Dev\SapphireTest;
-use SilverStripe\CMS\Model\SiteTree;
-use SilverStripe\CMS\Controllers\ContentController;
 
 class VersionFeedTest extends SapphireTest
 {
-
     protected $usesDatabase = true;
 
     protected static $required_extensions = [
@@ -19,10 +19,6 @@ class VersionFeedTest extends SapphireTest
         ContentController::class => [VersionFeedController::class],
     ];
 
-    protected $illegalExtensions = [
-        'SiteTree' => ['Translatable']
-    ];
-
     public function testDiffedChangesExcludesRestrictedItems()
     {
         $this->markTestIncomplete();
@@ -37,11 +33,11 @@ class VersionFeedTest extends SapphireTest
     {
         $page = new Page(['Title' => 'My Title']);
         $page->write();
-        $page->publish('Stage', 'Live');
+        $page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE);
     
         $page->Title = 'My Changed Title';
         $page->write();
-        $page->publish('Stage', 'Live');
+        $page->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE);
 
         $page->Title = 'My Unpublished Changed Title';
         $page->write();