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

@@ -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'];;
+			$rid = $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);
+			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",
@@ -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];
+				$nextVersion = 0;
-				else unset($nextVersion);
+				if($rid) {
-
-				if($rid && !isset($nextVersion)) {
 					$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;
-
+				$manipulation[$table]['fields']['Version'] = $nextVersion;
-				// Add the version number to this data
-				$manipulation[$table]['fields']['Version'] = $newManipulation['fields']['Version'];
-				$version_table[$table] = $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)
 					);
 				}
 

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',
@@ -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