API ChangeSet::publish() / canPublish() no longer treats hasChanges() = false as a permission error

BUG fix issues with doArchive() in live mode

admin/code/CampaignAdmin.php

@@ -194,7 +194,7 @@ class CampaignAdmin extends LeftAndMain implements PermissionProvider
         try {
             $changeSet->sync();
             $hal['Description'] = $changeSet->getDescription();
-            $hal['canPublish'] = $changeSet->canPublish();
+            $hal['canPublish'] = $changeSet->canPublish() && $changeSet->hasChanges();
 
             foreach ($changeSet->Changes() as $changeSetItem) {
                 if (!$changeSetItem) {

src/ORM/Versioning/ChangeSet.php

@@ -152,6 +152,10 @@ class ChangeSet extends DataObject
             throw new BadMethodCallException("ChangeSet must be saved before adding items");
         }
 
+        if (!$object->isInDB()) {
+            throw new BadMethodCallException("Items must be saved before adding to a changeset");
+        }
+
         $references = [
             'ObjectID'    => $object->ID,
             'ObjectClass' => $object->baseClass(),
@@ -353,17 +357,30 @@ class ChangeSet extends DataObject
      */
     public function canPublish($member = null)
     {
-        // All changes must be publishable
-        $atLeastOneChange = false;
         foreach ($this->Changes() as $change) {
-            $atLeastOneChange = true;
             /** @var ChangeSetItem $change */
             if (!$change->canPublish($member)) {
                 return false;
             }
         }
+        return true;
+    }
 
-        return $atLeastOneChange;
+    /**
+     * Determine if there are changes to publish
+     *
+     * @return bool
+     */
+    public function hasChanges()
+    {
+        // All changes must be publishable
+        /** @var ChangeSetItem $change */
+        foreach ($this->Changes() as $change) {
+            if ($change->hasChange()) {
+                return true;
+            }
+        }
+        return false;
     }
 
     /**

src/ORM/Versioning/ChangeSetItem.php

@@ -360,7 +360,17 @@ class ChangeSetItem extends DataObject implements Thumbnail
             }
         }
 
-        return false;
+        return true;
+    }
+
+    /**
+     * Determine if this item has changes
+     *
+     * @return bool
+     */
+    public function hasChange()
+    {
+        return $this->getChangeType() !== ChangeSetItem::CHANGE_NONE;
     }
 
     /**

src/ORM/Versioning/Versioned.php

@@ -1612,7 +1612,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider
 
         $owner->invokeWithExtensions('onBeforeArchive', $this);
         $owner->doUnpublish();
-        $owner->delete();
+        $owner->deleteFromStage(static::DRAFT);
         $owner->invokeWithExtensions('onAfterArchive', $this);
 
         return true;

tests/php/ORM/ChangeSetTest.php

@@ -257,6 +257,31 @@ class ChangeSetTest extends SapphireTest
         $this->assertTrue($changeSet->canPublish());
     }
 
+    public function testHasChanges()
+    {
+        // Create changeset containing all items (unpublished)
+        Versioned::set_stage(Versioned::DRAFT);
+        $this->logInWithPermission('ADMIN');
+        $changeSet = new ChangeSet();
+        $changeSet->write();
+        $base = new ChangeSetTest\BaseObject();
+        $base->Foo = 1;
+        $base->write();
+        $changeSet->addObject($base);
+
+        // New changeset with changes can be published
+        $this->assertTrue($changeSet->canPublish());
+        $this->assertTrue($changeSet->hasChanges());
+
+        // Writing the record to live dissolves the changes in this changeset
+        $base->publishSingle();
+        $this->assertTrue($changeSet->canPublish());
+        $this->assertFalse($changeSet->hasChanges());
+
+        // Changeset can be safely published without error
+        $changeSet->publish();
+    }
+
     public function testCanRevert()
     {
         $this->markTestSkipped("Requires ChangeSet::revert to be implemented first");

tests/php/ORM/VersionedOwnershipTest.php

@@ -40,9 +40,7 @@ class VersionedOwnershipTest extends SapphireTest
         foreach ($this->getFixtureFactory()->getFixtures() as $class => $fixtures) {
             foreach ($fixtures as $name => $id) {
                 if (stripos($name, '_published') !== false) {
-                    /**
+                    /** @var Versioned|DataObject $object */
- * @var Versioned|DataObject $object
-*/
                     $object = DataObject::get($class)->byID($id);
                     $object->copyVersionToStage(Versioned::DRAFT, Versioned::LIVE);
                 }
@@ -68,9 +66,7 @@ class VersionedOwnershipTest extends SapphireTest
      */
     public function testFindOwned()
     {
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass1 */
- * @var VersionedOwnershipTest\Subclass $subclass1
-*/
         $subclass1 = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass1_published');
         $this->assertDOSEquals(
             [
@@ -96,9 +92,7 @@ class VersionedOwnershipTest extends SapphireTest
             $subclass1->findOwned(false)
         );
 
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2 */
- * @var VersionedOwnershipTest\Subclass $subclass2
-*/
         $subclass2 = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass2_published');
         $this->assertDOSEquals(
             [
@@ -120,9 +114,7 @@ class VersionedOwnershipTest extends SapphireTest
             $subclass2->findOwned(false)
         );
 
-        /**
+        /** @var VersionedOwnershipTest\Related $related1 */
- * @var VersionedOwnershipTest\Related $related1
-*/
         $related1 = $this->objFromFixture(VersionedOwnershipTest\Related::class, 'related1');
         $this->assertDOSEquals(
             [
@@ -133,9 +125,7 @@ class VersionedOwnershipTest extends SapphireTest
             $related1->findOwned()
         );
 
-        /**
+        /** @var VersionedOwnershipTest\Related $related2 */
- * @var VersionedOwnershipTest\Related $related2
-*/
         $related2 = $this->objFromFixture(VersionedOwnershipTest\Related::class, 'related2_published');
         $this->assertDOSEquals(
             [
@@ -152,9 +142,7 @@ class VersionedOwnershipTest extends SapphireTest
      */
     public function testFindOwners()
     {
-        /**
+        /** @var VersionedOwnershipTest\Attachment $attachment1 */
- * @var VersionedOwnershipTest\Attachment $attachment1
-*/
         $attachment1 = $this->objFromFixture(VersionedOwnershipTest\Attachment::class, 'attachment1');
         $this->assertDOSEquals(
             [
@@ -172,9 +160,7 @@ class VersionedOwnershipTest extends SapphireTest
             $attachment1->findOwners(false)
         );
 
-        /**
+        /** @var VersionedOwnershipTest\Attachment $attachment5 */
- * @var VersionedOwnershipTest\Attachment $attachment5
-*/
         $attachment5 = $this->objFromFixture(VersionedOwnershipTest\Attachment::class, 'attachment5_published');
         $this->assertDOSEquals(
             [
@@ -195,9 +181,7 @@ class VersionedOwnershipTest extends SapphireTest
             $attachment5->findOwners(false)
         );
 
-        /**
+        /** @var VersionedOwnershipTest\Related $related1 */
- * @var VersionedOwnershipTest\Related $related1
-*/
         $related1 = $this->objFromFixture(VersionedOwnershipTest\Related::class, 'related1');
         $this->assertDOSEquals(
             [
@@ -224,9 +208,7 @@ class VersionedOwnershipTest extends SapphireTest
         $subclass2ID = $this->idFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass2_published');
 
         // Check that stage record is ok
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2Stage */
- * @var VersionedOwnershipTest\Subclass $subclass2Stage
-*/
         $subclass2Stage = Versioned::get_by_stage(VersionedOwnershipTest\Subclass::class, 'Stage')->byID($subclass2ID);
         $this->assertDOSEquals(
             [
@@ -248,9 +230,7 @@ class VersionedOwnershipTest extends SapphireTest
         );
 
         // Live records are unchanged
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2Live */
- * @var VersionedOwnershipTest\Subclass $subclass2Live
-*/
         $subclass2Live = Versioned::get_by_stage(VersionedOwnershipTest\Subclass::class, 'Live')->byID($subclass2ID);
         $this->assertDOSEquals(
             [
@@ -278,9 +258,7 @@ class VersionedOwnershipTest extends SapphireTest
      */
     public function testRecursivePublish()
     {
-        /**
+        /** @var VersionedOwnershipTest\Subclass $parent */
- * @var VersionedOwnershipTest\Subclass $parent
-*/
         $parent = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass1_published');
         $parentID = $parent->ID;
         $banner1 = $this->objFromFixture(VersionedOwnershipTest\RelatedMany::class, 'relatedmany1_published');
@@ -335,9 +313,7 @@ class VersionedOwnershipTest extends SapphireTest
         $this->assertEmpty($banner2Live->PageID);
 
         // Test that a changeset was created
-        /**
+        /** @var ChangeSet $changeset */
- * @var ChangeSet $changeset
-*/
         $changeset = ChangeSet::get()->sort('"ChangeSet"."ID" DESC')->first();
         $this->assertNotEmpty($changeset);
 
@@ -387,32 +363,22 @@ class VersionedOwnershipTest extends SapphireTest
         $this->assertFalse($unsaved->doUnpublish());
 
         // Draft-only objects can't be unpublished
-        /**
+        /** @var VersionedOwnershipTest\RelatedMany $banner3Unpublished */
- * @var VersionedOwnershipTest\RelatedMany $banner3Unpublished
-*/
         $banner3Unpublished = $this->objFromFixture(VersionedOwnershipTest\RelatedMany::class, 'relatedmany3');
         $this->assertFalse($banner3Unpublished->doUnpublish());
 
         // First test: mid-level unpublish; We expect that owners should be unpublished, but not
         // owned objects, nor other siblings shared by the same owner.
         $related2 = $this->objFromFixture(VersionedOwnershipTest\Related::class, 'related2_published');
-        /**
+        /** @var VersionedOwnershipTest\Attachment $attachment3 */
- * @var VersionedOwnershipTest\Attachment $attachment3
-*/
         $attachment3 = $this->objFromFixture(VersionedOwnershipTest\Attachment::class, 'attachment3_published');
-        /**
+        /** @var VersionedOwnershipTest\RelatedMany $relatedMany4 */
- * @var VersionedOwnershipTest\RelatedMany $relatedMany4
-*/
         $relatedMany4 = $this->objFromFixture(VersionedOwnershipTest\RelatedMany::class, 'relatedmany4_published');
-        /**
+        /** @var VersionedOwnershipTest\Related $related2 */
- * @var VersionedOwnershipTest\Related $related2
-*/
         $this->assertTrue($related2->doUnpublish());
         $subclass2 = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass2_published');
 
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2 */
- * @var VersionedOwnershipTest\Subclass $subclass2
-*/
         $this->assertFalse($subclass2->isPublished()); // Owner IS unpublished
         $this->assertTrue($attachment3->isPublished()); // Owned object is NOT unpublished
         $this->assertTrue($relatedMany4->isPublished()); // Owned object by owner is NOT unpublished
@@ -439,9 +405,7 @@ class VersionedOwnershipTest extends SapphireTest
         // When archiving an object, any published owners should be unpublished at the same time
         // but NOT achived
 
-        /**
+        /** @var VersionedOwnershipTest\Attachment $attachment3 */
- * @var VersionedOwnershipTest\Attachment $attachment3
-*/
         $attachment3 = $this->objFromFixture(VersionedOwnershipTest\Attachment::class, 'attachment3_published');
         $attachment3ID = $attachment3->ID;
         $this->assertTrue($attachment3->doArchive());
@@ -455,17 +419,13 @@ class VersionedOwnershipTest extends SapphireTest
         $this->assertEmpty($liveAttachment);
 
         // Owning object is unpublished only
-        /**
+        /** @var VersionedOwnershipTest\Related $stageOwner */
- * @var VersionedOwnershipTest\Related $stageOwner
-*/
         $stageOwner = $this->objFromFixture(VersionedOwnershipTest\Related::class, 'related2_published');
         $this->assertTrue($stageOwner->isOnDraft());
         $this->assertFalse($stageOwner->isPublished());
 
         // Bottom level owning object is also unpublished
-        /**
+        /** @var VersionedOwnershipTest\Subclass $stageTopOwner */
- * @var VersionedOwnershipTest\Subclass $stageTopOwner
-*/
         $stageTopOwner = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass2_published');
         $this->assertTrue($stageTopOwner->isOnDraft());
         $this->assertFalse($stageTopOwner->isPublished());
@@ -473,9 +433,7 @@ class VersionedOwnershipTest extends SapphireTest
 
     public function testRecursiveRevertToLive()
     {
-        /**
+        /** @var VersionedOwnershipTest\Subclass $parent */
- * @var VersionedOwnershipTest\Subclass $parent
-*/
         $parent = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass1_published');
         $parentID = $parent->ID;
         $banner1 = $this->objFromFixture(VersionedOwnershipTest\RelatedMany::class, 'relatedmany1_published');
@@ -523,9 +481,7 @@ class VersionedOwnershipTest extends SapphireTest
 
         // Check that the newly created banner, even though it still exist, has been
         // unlinked from the reverted draft record
-        /**
+        /** @var VersionedOwnershipTest\RelatedMany $banner4Draft */
- * @var VersionedOwnershipTest\RelatedMany $banner4Draft
-*/
         $banner4Draft = Versioned::get_by_stage(VersionedOwnershipTest\RelatedMany::class, Versioned::DRAFT)
             ->byID($banner4->ID);
         $this->assertTrue($banner4Draft->isOnDraft());
@@ -538,9 +494,7 @@ class VersionedOwnershipTest extends SapphireTest
      */
     public function testRecursiveRollback()
     {
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2 */
- * @var VersionedOwnershipTest\Subclass $subclass2
-*/
         $this->sleep(1);
         $subclass2 = $this->objFromFixture(VersionedOwnershipTest\Subclass::class, 'subclass2_published');
 
@@ -564,9 +518,7 @@ class VersionedOwnershipTest extends SapphireTest
         // Check reverting to first version
         $this->sleep(1);
         $subclass2->doRollbackTo($versions[1]);
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2Draft */
- * @var VersionedOwnershipTest\Subclass $subclass2Draft
-*/
         $subclass2Draft = Versioned::get_by_stage(VersionedOwnershipTest\Subclass::class, Versioned::DRAFT)
             ->byID($subclass2->ID);
         $this->assertEquals('Subclass 2 - v1', $subclass2Draft->Title);
@@ -584,9 +536,7 @@ class VersionedOwnershipTest extends SapphireTest
         // Check rolling forward to a later version
         $this->sleep(1);
         $subclass2->doRollbackTo($versions[3]);
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2Draft */
- * @var VersionedOwnershipTest\Subclass $subclass2Draft
-*/
         $subclass2Draft = Versioned::get_by_stage(VersionedOwnershipTest\Subclass::class, Versioned::DRAFT)
             ->byID($subclass2->ID);
         $this->assertEquals('Subclass 2 - v1 - v2 - v3', $subclass2Draft->Title);
@@ -604,9 +554,7 @@ class VersionedOwnershipTest extends SapphireTest
         // And rolling back one version
         $this->sleep(1);
         $subclass2->doRollbackTo($versions[2]);
-        /**
+        /** @var VersionedOwnershipTest\Subclass $subclass2Draft */
- * @var VersionedOwnershipTest\Subclass $subclass2Draft
-*/
         $subclass2Draft = Versioned::get_by_stage(VersionedOwnershipTest\Subclass::class, Versioned::DRAFT)
             ->byID($subclass2->ID);
         $this->assertEquals('Subclass 2 - v1 - v2', $subclass2Draft->Title);
@@ -628,13 +576,9 @@ class VersionedOwnershipTest extends SapphireTest
     public function testInferedOwners()
     {
         // Make sure findOwned() works
-        /**
+        /** @var VersionedOwnershipTest\TestPage $page1 */
- * @var VersionedOwnershipTest\TestPage $page1
-*/
         $page1 = $this->objFromFixture(VersionedOwnershipTest\TestPage::class, 'page1_published');
-        /**
+        /** @var VersionedOwnershipTest\TestPage $page2 */
- * @var VersionedOwnershipTest\TestPage $page2
-*/
         $page2 = $this->objFromFixture(VersionedOwnershipTest\TestPage::class, 'page2_published');
         $this->assertDOSEquals(
             [
@@ -656,13 +600,9 @@ class VersionedOwnershipTest extends SapphireTest
         );
 
         // Check that findOwners works
-        /**
+        /** @var VersionedOwnershipTest\Image $image1 */
- * @var VersionedOwnershipTest\Image $image1
-*/
         $image1 = $this->objFromFixture(VersionedOwnershipTest\Image::class, 'image1_published');
-        /**
+        /** @var VersionedOwnershipTest\Image $image2 */
- * @var VersionedOwnershipTest\Image $image2
-*/
         $image2 = $this->objFromFixture(VersionedOwnershipTest\Image::class, 'image2_published');
 
         $this->assertDOSEquals(
@@ -696,9 +636,7 @@ class VersionedOwnershipTest extends SapphireTest
         );
 
         // Test custom relation can findOwners()
-        /**
+        /** @var VersionedOwnershipTest\CustomRelation $custom1 */
- * @var VersionedOwnershipTest\CustomRelation $custom1
-*/
         $custom1 = $this->objFromFixture(VersionedOwnershipTest\CustomRelation::class, 'custom1_published');
         $this->assertDOSEquals(
             [['Title' => 'Page 1']],

tests/php/ORM/VersionedTest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\ORM\Tests;
 
+use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\ORM\DataObjectSchema;
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\Versioning\Versioned;
@@ -274,9 +275,7 @@ class VersionedTest extends SapphireTest
 
     public function testPublishCreateNewVersion()
     {
-        /**
+        /** @var VersionedTest\TestObject $page1 */
- * @var \SilverStripe\ORM\Tests\VersionedTest\VersionedTest_DataObject $page1
-*/
         $page1 = $this->objFromFixture(VersionedTest\TestObject::class, 'page1');
         $page1->Content = 'orig';
         $page1->write();
@@ -538,9 +537,7 @@ class VersionedTest extends SapphireTest
         // Create a few initial versions to ensure this version
         // doesn't clash with child versions
         $this->sleep(1);
-        /**
+        /** @var VersionedTest\TestObject $page2 */
- * @var \SilverStripe\ORM\Tests\VersionedTest\VersionedTest_DataObject $page2
-*/
         $page2 = $this->objFromFixture(VersionedTest\TestObject::class, 'page2');
         $page2->Title = 'dummy1';
         $page2->write();
@@ -556,9 +553,7 @@ class VersionedTest extends SapphireTest
         // Create another version where this object and some
         // child records have been modified
         $this->sleep(1);
-        /**
+        /** @var VersionedTest\TestObject $page2a */
- * @var \SilverStripe\ORM\Tests\VersionedTest\VersionedTest_DataObject $page2a
-*/
         $page2a = $this->objFromFixture(VersionedTest\TestObject::class, 'page2a');
         $page2a->Title = 'Page 2a - v2';
         $page2a->write();
@@ -577,9 +572,7 @@ class VersionedTest extends SapphireTest
         );
 
         // test selecting v1
-        /**
+        /** @var VersionedTest\TestObject $page2v1 */
- * @var \SilverStripe\ORM\Tests\VersionedTest\VersionedTest_DataObject $page2v1
-*/
         $page2v1 = Versioned::get_version(VersionedTest\TestObject::class, $page2->ID, $version1);
         $this->assertEquals('Page 2 - v1', $page2v1->Title);
 
@@ -600,9 +593,7 @@ class VersionedTest extends SapphireTest
         );
 
         // When selecting v2, we get the same as on stage
-        /**
+        /** @var VersionedTest\TestObject $page2v2 */
- * @var \SilverStripe\ORM\Tests\VersionedTest\VersionedTest_DataObject $page2v2
-*/
         $page2v2 = Versioned::get_version(VersionedTest\TestObject::class, $page2->ID, $version2);
         $this->assertEquals('Page 2 - v2', $page2v2->Title);
 
@@ -649,7 +640,6 @@ class VersionedTest extends SapphireTest
 
     public function testArchiveVersion()
     {
-
         // In 2005 this file was created
         DBDatetime::set_mock_now('2005-01-01 00:00:00');
         $testPage = new VersionedTest\Subclass();
@@ -699,6 +689,54 @@ class VersionedTest extends SapphireTest
         $this->assertEquals("I'm enjoying 2009", $testPageCurrent->Content);
     }
 
+    /**
+     * Test that archive works on live stage
+     */
+    public function testArchiveLive()
+    {
+        Versioned::set_stage(Versioned::LIVE);
+        $this->logInWithPermission('ADMIN');
+        $record = new VersionedTest\TestObject();
+        $record->Name = 'test object';
+        // Writing in live mode should write to draft as well
+        $record->write();
+        $recordID = $record->ID;
+        $this->assertTrue($record->isPublished());
+        $this->assertTrue($record->isOnDraft());
+
+        // Delete in live
+        /** @var VersionedTest\TestObject $recordLive */
+        $recordLive = VersionedTest\TestObject::get()->byID($recordID);
+        $recordLive->doArchive();
+        $this->assertFalse($recordLive->isPublished());
+        $this->assertFalse($recordLive->isOnDraft());
+    }
+
+    /**
+     * Test archive works on draft
+     */
+    public function testArchiveDraft()
+    {
+        Versioned::set_stage(Versioned::DRAFT);
+        $this->logInWithPermission('ADMIN');
+        $record = new VersionedTest\TestObject();
+        $record->Name = 'test object';
+
+        // Writing in draft mode requires publishing to effect on live
+        $record->write();
+        $record->publishRecursive();
+        $recordID = $record->ID;
+        $this->assertTrue($record->isPublished());
+        $this->assertTrue($record->isOnDraft());
+
+        // Delete in draft
+        /** @var VersionedTest\TestObject $recordDraft */
+        $recordDraft = VersionedTest\TestObject::get()->byID($recordID);
+        $recordDraft->doArchive();
+        $this->assertFalse($recordDraft->isPublished());
+        $this->assertFalse($recordDraft->isOnDraft());
+    }
+
     public function testAllVersions()
     {
         // In 2005 this file was created
@@ -958,9 +996,9 @@ class VersionedTest extends SapphireTest
      */
     public function testReadingModeSecurity()
     {
-        $this->setExpectedException('SilverStripe\\Control\\HTTPResponse_Exception');
+        $this->setExpectedException(HTTPResponse_Exception::class);
-        $session = Injector::inst()->create('SilverStripe\\Control\\Session', array());
+        $session = Injector::inst()->create(Session::class, array());
-        $result = Director::test('/?stage=Stage', null, $session);
+        Director::test('/?stage=Stage', null, $session);
     }
 
     /**