From 1f4f5e68ba9ae237bab837d2717532dddb0119d4 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 2 Sep 2014 09:14:08 +1200 Subject: [PATCH] BUG Fix versioned Versioned is not writing Version to _version tables for subclasses of Version dataobjects which have their own DB fields - Fix disjoint of ID / RecordID (which should be the same) - Fix calculation of new record version - Fix use of empty vs !isset to check for existing version Conflicts: model/Versioned.php tests/model/VersionedTest.php Cherry picked from commit c140459ac624738630ac8bec6a665fa3040e2e54 --- model/Versioned.php | 27 ++++----- tests/model/VersionedTest.php | 105 +++++++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 17 deletions(-) diff --git a/model/Versioned.php b/model/Versioned.php index 8db06e8d0..6dbfe57e1 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -553,11 +553,9 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { unset($manipulation[$table]); continue; } - $id = $manipulation[$table]['id'] ? $manipulation[$table]['id'] : $manipulation[$table]['fields']['ID'];; - if(!$id) user_error("Couldn't find ID in " . var_export($manipulation[$table], true), E_USER_ERROR); + $rid = $manipulation[$table]['id'] ? $manipulation[$table]['id'] : $manipulation[$table]['fields']['ID'];; + if(!$rid) user_error("Couldn't find ID in " . var_export($manipulation[$table], true), E_USER_ERROR); - $rid = isset($manipulation[$table]['RecordID']) ? $manipulation[$table]['RecordID'] : $id; - $newManipulation = array( "command" => "insert", "fields" => isset($manipulation[$table]['fields']) ? $manipulation[$table]['fields'] : null @@ -569,9 +567,9 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { // If we haven't got a version #, then we're creating a new version. // Otherwise, we're just copying a version to another table - if(!isset($manipulation[$table]['fields']['Version'])) { + if(empty($manipulation[$table]['fields']['Version'])) { // Add any extra, unchanged fields to the version record. - $data = DB::query("SELECT * FROM \"$table\" WHERE \"ID\" = $id")->record(); + $data = DB::query("SELECT * FROM \"$table\" WHERE \"ID\" = $rid")->record(); if($data) foreach($data as $k => $v) { if (!isset($newManipulation['fields'][$k])) { $newManipulation['fields'][$k] = "'" . Convert::raw2sql($v) . "'"; @@ -583,15 +581,15 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { unset($newManipulation['fields']['ID']); // Create a new version # - if (isset($version_table[$table])) $nextVersion = $version_table[$table]; - else unset($nextVersion); - - if($rid && !isset($nextVersion)) { + $nextVersion = 0; + if($rid) { $nextVersion = DB::query("SELECT MAX(\"Version\") + 1 FROM \"{$baseDataClass}_versions\"" . " WHERE \"RecordID\" = $rid")->value(); } + $nextVersion = $nextVersion ?: 1; - $newManipulation['fields']['Version'] = $nextVersion ? $nextVersion : 1; + // Add the version number to this data + $newManipulation['fields']['Version'] = $nextVersion; if($isRootClass) { $userID = (Member::currentUser()) ? Member::currentUser()->ID : 0; @@ -601,10 +599,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { $manipulation["{$table}_versions"] = $newManipulation; - - // Add the version number to this data - $manipulation[$table]['fields']['Version'] = $newManipulation['fields']['Version']; - $version_table[$table] = $nextVersion; + $manipulation[$table]['fields']['Version'] = $nextVersion; } // Putting a Version of -1 is a signal to leave the version table alone, despite their being no version @@ -627,7 +622,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { ) { // If the record has already been inserted in the (table), get rid of it. if($manipulation[$table]['command']=='insert') { - DB::query("DELETE FROM \"{$table}\" WHERE \"ID\"='$id'"); + DB::query("DELETE FROM \"{$table}\" WHERE \"ID\"='$rid'"); } $newTable = $table . '_' . Versioned::current_stage(); diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index f2a1f9183..864e59463 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -11,6 +11,7 @@ class VersionedTest extends SapphireTest { protected $extraDataObjects = array( 'VersionedTest_DataObject', 'VersionedTest_Subclass', + 'VersionedTest_AnotherSubclass', 'VersionedTest_RelatedWithoutVersion', 'VersionedTest_SingleStage' ); @@ -18,7 +19,7 @@ class VersionedTest extends SapphireTest { protected $requiredExtensions = array( "VersionedTest_DataObject" => array('Versioned') ); - + public function testDeletingOrphanedVersions() { $obj = new VersionedTest_Subclass(); $obj->ExtraField = 'Foo'; // ensure that child version table gets written @@ -586,6 +587,98 @@ class VersionedTest extends SapphireTest { $this->assertEquals('Stage.Live', Versioned::get_reading_mode()); } + /** + * Ensures that the latest version of a record is the expected value + * + * @param type $record + * @param type $version + */ + protected function assertRecordHasLatestVersion($record, $version) { + foreach(ClassInfo::ancestry(get_class($record), true) as $table) { + $versionForClass = DB::query( + sprintf("SELECT MAX(\"Version\") FROM \"{$table}_versions\" WHERE \"RecordID\" = %d", + $record->ID) + )->value(); + $this->assertEquals($version, $versionForClass, "That the table $table has the latest version $version"); + } + } + + /** + * Tests that multi-table dataobjects are correctly versioned + */ + public function testWriteToStage() { + // Test subclass with versioned extension directly added + $record = VersionedTest_Subclass::create(); + $record->Title = "Test A"; + $record->ExtraField = "Test A"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 1); + $record->publish("Stage", "Live"); + $this->assertRecordHasLatestVersion($record, 1); + $record->Title = "Test A2"; + $record->ExtraField = "Test A2"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 2); + + // Test subclass without changes to base class + $record = VersionedTest_Subclass::create(); + $record->ExtraField = "Test B"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 1); + $record->publish("Stage", "Live"); + $this->assertRecordHasLatestVersion($record, 1); + $record->ExtraField = "Test B2"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 2); + + // Test subclass without changes to sub class + $record = VersionedTest_Subclass::create(); + $record->Title = "Test C"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 1); + $record->publish("Stage", "Live"); + $this->assertRecordHasLatestVersion($record, 1); + $record->Title = "Test C2"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 2); + + // Test subclass with versioned extension only added to the base clases + $record = VersionedTest_AnotherSubclass::create(); + $record->Title = "Test A"; + $record->AnotherField = "Test A"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 1); + $record->publish("Stage", "Live"); + $this->assertRecordHasLatestVersion($record, 1); + $record->Title = "Test A2"; + $record->AnotherField = "Test A2"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 2); + + + // Test subclass without changes to base class + $record = VersionedTest_AnotherSubclass::create(); + $record->AnotherField = "Test B"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 1); + $record->publish("Stage", "Live"); + $this->assertRecordHasLatestVersion($record, 1); + $record->AnotherField = "Test B2"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 2); + + // Test subclass without changes to sub class + $record = VersionedTest_AnotherSubclass::create(); + $record->Title = "Test C"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 1); + $record->publish("Stage", "Live"); + $this->assertRecordHasLatestVersion($record, 1); + $record->Title = "Test C2"; + $record->writeToStage("Stage"); + $this->assertRecordHasLatestVersion($record, 2); + } + } @@ -644,6 +737,16 @@ class VersionedTest_Subclass extends VersionedTest_DataObject implements TestOnl ); } +/** + * @package framework + * @subpackage tests + */ +class VersionedTest_AnotherSubclass extends VersionedTest_DataObject implements TestOnly { + private static $db = array( + "AnotherField" => "Varchar" + ); +} + /** * @package framework * @subpackage tests