From c140459ac624738630ac8bec6a665fa3040e2e54 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 --- model/Versioned.php | 27 ++++----- tests/model/VersionedTest.php | 107 +++++++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 18 deletions(-) diff --git a/model/Versioned.php b/model/Versioned.php index ea995f5d3..7ae0627e0 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -572,10 +572,8 @@ 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 = isset($manipulation[$table]['RecordID']) ? $manipulation[$table]['RecordID'] : $id; + $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); $newManipulation = array( "command" => "insert", @@ -588,9 +586,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::prepared_query("SELECT * FROM \"$table\" WHERE \"ID\" = ?", array($id))->record(); + $data = DB::prepared_query("SELECT * FROM \"$table\" WHERE \"ID\" = ?", array($rid))->record(); if($data) foreach($data as $k => $v) { if (!isset($newManipulation['fields'][$k])) { $newManipulation['fields'][$k] = $v; @@ -602,17 +600,17 @@ 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::prepared_query("SELECT MAX(\"Version\") + 1 FROM \"{$baseDataClass}_versions\" WHERE \"RecordID\" = ?", array($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; @@ -622,10 +620,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 @@ -650,7 +645,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { if($manipulation[$table]['command']=='insert') { DB::prepared_query( "DELETE FROM \"{$table}\" WHERE \"ID\" = ?", - array($id) + array($rid) ); } diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index 64e5e998d..74ed4337b 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', 'VersionedTest_WithIndexes', @@ -30,7 +31,7 @@ class VersionedTest extends SapphireTest { 'VersionedTest_WithIndexes_Live' => array('value' => false, 'message' => 'Unique indexes are no longer unique in _Live table'), ); - + // Test each table's performance foreach ($tableExpectations as $tableName => $expectation) { $indexes = DB::get_schema()->indexList($tableName); @@ -51,7 +52,7 @@ class VersionedTest extends SapphireTest { if (in_array($indexSpec['value'], $expectedColumns)) { $isUnique = $indexSpec['type'] === 'unique'; $this->assertEquals($isUnique, $expectation['value'], $expectation['message']); - } +} } } } @@ -627,6 +628,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::prepared_query( + $sql = "SELECT MAX(\"Version\") FROM \"{$table}_versions\" WHERE \"RecordID\" = ?", + array($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); + } + } @@ -701,6 +794,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