Merge pull request #5302 from open-sausages/pulls/4.0/changesets-improve-api

Improve changset api & resiliency

model/versioning/ChangeSet.php

@@ -117,7 +117,7 @@ class ChangeSet extends DataObject {
 
 		$references = [
 			'ObjectID'    => $object->ID,
-			'ObjectClass' => $object->ClassName
+			'ObjectClass' => ClassInfo::baseDataClass($object)
 		];
 
 		// Get existing item in case already added
@@ -146,7 +146,7 @@ class ChangeSet extends DataObject {
 	public function removeObject(DataObject $object) {
 		$item = ChangeSetItem::get()->filter([
 				'ObjectID' => $object->ID,
-				'ObjectClass' => $object->ClassName,
+				'ObjectClass' => ClassInfo::baseDataClass($object),
 				'ChangeSetID' => $this->ID
 			])->first();
 
@@ -161,7 +161,7 @@ class ChangeSet extends DataObject {
 
 	protected function implicitKey($item) {
 		if ($item instanceof ChangeSetItem) return $item->ObjectClass.'.'.$item->ObjectID;
-		return $item->ClassName.'.'.$item->ID;
+		return ClassInfo::baseDataClass($item).'.'.$item->ID;
 	}
 
 	protected function calculateImplicit() {
@@ -183,7 +183,7 @@ class ChangeSet extends DataObject {
 
 				$referenced[$key] = [
 					'ObjectID' => $referee->ID,
-					'ObjectClass' => $referee->ClassName
+					'ObjectClass' => ClassInfo::baseDataClass($referee)
 				];
 
 				$references[$key][] = $item->ID;

model/versioning/ChangeSetItem.php

@@ -9,8 +9,8 @@ use SilverStripe\Filesystem\Thumbnail;
  * A single line in a changeset
  *
  * @property string $Added
- * @property string $ObjectClass
- * @property int $ObjectID
+ * @property string $ObjectClass The _base_ data class for the referenced DataObject
+ * @property int $ObjectID The numeric ID for the referenced object
  * @method ManyManyList ReferencedBy() List of explicit items that require this change
  * @method ManyManyList References() List of implicit items required by this change
  * @method ChangeSet ChangeSet()
@@ -65,6 +65,12 @@ class ChangeSetItem extends DataObject implements Thumbnail {
 		)
 	);
 
+	public function onBeforeWrite() {
+		// Make sure ObjectClass refers to the base data class in the case of old or wrong code
+		$this->ObjectClass = ClassInfo::baseDataClass($this->ObjectClass);
+		parent::onBeforeWrite();
+	}
+
 	public function getTitle() {
 		// Get title of modified object
 		$object = $this->getObjectLatestVersion();
@@ -74,8 +80,6 @@ class ChangeSetItem extends DataObject implements Thumbnail {
 		return $this->i18n_singular_name() . ' #' . $this->ID;
 	}
 
-
-
 	/**
 	 * Get a thumbnail for this object
 	 *
@@ -91,7 +95,6 @@ class ChangeSetItem extends DataObject implements Thumbnail {
 		return null;
 	}
 
-
 	/**
 	 * Get the type of change: none, created, deleted, modified, manymany
 	 *
@@ -315,4 +318,31 @@ class ChangeSetItem extends DataObject implements Thumbnail {
 		// Default permissions
 		return (bool)Permission::checkMember($member, ChangeSet::config()->required_permission);
 	}
+
+	/**
+	 * Get the ChangeSetItems that reference a passed DataObject
+	 *
+	 * @param DataObject $object
+	 * @return DataList
+	 */
+	public static function get_for_object($object) {
+		return ChangeSetItem::get()->filter([
+			'ObjectID' => $object->ID,
+			'ObjectClass' => ClassInfo::baseDataClass($object)
+		]);
+	}
+
+	/**
+	 * Get the ChangeSetItems that reference a passed DataObject
+	 *
+	 * @param int $objectID The ID of the object
+	 * @param string $objectClass The class of the object (or any parent class)
+	 * @return DataList
+	 */
+	public static function get_for_object_by_id($objectID, $objectClass) {
+		return ChangeSetItem::get()->filter([
+			'ObjectID' => $objectID,
+			'ObjectClass' => ClassInfo::baseDataClass($objectClass)
+		]);
+	}
 }

tests/model/ChangeSetItemTest.php

@@ -28,7 +28,7 @@ class ChangeSetItemTest extends SapphireTest {
 
 		$item = new ChangeSetItem([
 			'ObjectID' => $object->ID,
-			'ObjectClass' => $object->ClassName
+			'ObjectClass' => ClassInfo::baseDataClass($object->ClassName)
 		]);
 
 		$this->assertEquals(
@@ -73,4 +73,25 @@ class ChangeSetItemTest extends SapphireTest {
 			'Objects that have been deleted and then unpublished should return no change'
 		);
 	}
+
+	function testGetForObject() {
+		$object = new ChangeSetItemTest_Versioned(['Foo' => 1]);
+		$object->write();
+
+		$item = new ChangeSetItem([
+			'ObjectID' => $object->ID,
+			'ObjectClass' => ClassInfo::baseDataClass($object)
+		]);
+		$item->write();
+
+		$this->assertEquals(
+			ChangeSetItemTest_Versioned::get()->byID($object->ID)->toMap(),
+			ChangeSetItem::get_for_object($object)->first()->Object()->toMap()
+		);
+
+		$this->assertEquals(
+			ChangeSetItemTest_Versioned::get()->byID($object->ID)->toMap(),
+			ChangeSetItem::get_for_object_by_id($object->ID, $object->ClassName)->first()->Object()->toMap()
+		);
+	}
 }

tests/model/ChangeSetTest.php

@@ -98,6 +98,16 @@ class ChangeSetTest_End extends DataObject implements TestOnly {
 	];
 }
 
+/**
+ * @mixin Versioned
+ */
+class ChangeSetTest_EndChild extends ChangeSetTest_End implements TestOnly {
+
+	private static $db = [
+		'Qux' => 'Int',
+	];
+}
+
 /**
  * Test {@see ChangeSet} and {@see ChangeSetItem} models
  */
@@ -109,6 +119,7 @@ class ChangeSetTest extends SapphireTest {
 		'ChangeSetTest_Base',
 		'ChangeSetTest_Mid',
 		'ChangeSetTest_End',
+		'ChangeSetTest_EndChild',
 	];
 
 	/**
@@ -139,7 +150,7 @@ class ChangeSetTest extends SapphireTest {
 			$object = $this->objFromFixture($class, $identifier);
 
 			foreach($items as $i => $item) {
-				if ($item->ObjectClass == $object->ClassName && $item->ObjectID == $object->ID && $item->Added == $mode) {
+				if ($item->ObjectClass == ClassInfo::baseDataClass($object) && $item->ObjectID == $object->ID && $item->Added == $mode) {
 					unset($items[$i]);
 					continue 2;
 				}
@@ -160,6 +171,19 @@ class ChangeSetTest extends SapphireTest {
 			);
 		}
 	}
+	
+	public function testAddObject() {
+		$cs = new ChangeSet();
+		$cs->write();
+
+		$cs->addObject($this->objFromFixture('ChangeSetTest_End', 'end1'));
+		$cs->addObject($this->objFromFixture('ChangeSetTest_EndChild', 'endchild1'));
+
+		$this->assertChangeSetLooksLike($cs, [
+			'ChangeSetTest_End.end1' => ChangeSetItem::EXPLICITLY,
+			'ChangeSetTest_EndChild.endchild1' => ChangeSetItem::EXPLICITLY
+		]);
+	}
 
 	public function testRepeatedSyncIsNOP() {
 		$this->publishAllFixtures();
@@ -207,10 +231,11 @@ class ChangeSetTest extends SapphireTest {
 			'ChangeSetTest_End.end1' => ChangeSetItem::IMPLICITLY
 		]);
 
-		$endItem = $cs->Changes()->filter('ObjectClass', 'ChangeSetTest_End')->first();
+		$baseItem = ChangeSetItem::get_for_object($base)->first();
+		$endItem = ChangeSetItem::get_for_object($end)->first();
 
 		$this->assertEquals(
-			[$base->ID],
+			[$baseItem->ID],
 			$endItem->ReferencedBy()->column("ID")
 		);
 
@@ -256,7 +281,6 @@ class ChangeSetTest extends SapphireTest {
 		$this->assertTrue($cs->isSynced());
 	}
 
-
 	public function testCanPublish() {
 		// Create changeset containing all items (unpublished)
 		$this->logInWithPermission('ADMIN');

tests/model/ChangeSetTest.yml

@@ -6,6 +6,10 @@ ChangeSetTest_End:
     Baz: 1
   end2:
     Baz: 2
+ChangeSetTest_EndChild:
+  endchild1:
+    Baz: 3
+    Qux: 3
 ChangeSetTest_Mid:
   mid1:
     Bar: 1