Merge pull request #132 from jthomerson/pulls/only_write_translations…

…_when_class_actually_changed Only write translations when class actually changed
silverstripe · Sep 5, 2013 · 69adec9 · 69adec9
2 parents 3476a04 + 9e37602
commit 69adec9
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 15 deletions.
diff --git a/code/model/Translatable.php b/code/model/Translatable.php
@@ -873,7 +873,33 @@ class_exists('SiteTree')
 		// dropdown is readonly on all translations.
 		if($this->owner->ID && $this->owner->Locale == Translatable::default_locale()) {
 			$changedFields = $this->owner->getChangedFields();
-			if(isset($changedFields['ClassName'])) {
+			$changed = isset($changedFields['ClassName']);
+
+			if ($changed && $this->owner->hasExtension('Versioned')) {
+				// this is required because when publishing a node the before/after
+				// values of $changedFields['ClassName'] will be the same because
+				// the record was already written to the stage/draft table and thus
+				// the record was updated, and then publish('Stage', 'Live') is
+				// called, which uses forceChange, which will make all the fields
+				// act as though they are changed, although the before/after values
+				// will be the same
+				// So, we load one from the current stage and test against it
+				// This is to prevent the overhead of writing all translations when
+				// the class didn't actually change.
+				$baseDataClass = ClassInfo::baseDataClass($this->owner->class);
+				$currentStage = Versioned::current_stage();
+				$fresh = Versioned::get_one_by_stage(
+					$baseDataClass,
+					Versioned::current_stage(),
+					'"ID" = ' . $this->owner->ID,
+					null
+				);
+				if ($fresh) {
+					$changed = $changedFields['ClassName']['after'] != $fresh->ClassName;
+				}
+			}
+
+			if($changed) {
 				$this->owner->ClassName = $changedFields['ClassName']['before'];
 				$translations = $this->owner->getTranslations();
 				$this->owner->ClassName = $changedFields['ClassName']['after'];

diff --git a/tests/unit/TranslatableTest.php b/tests/unit/TranslatableTest.php
@@ -15,7 +15,7 @@ class TranslatableTest extends FunctionalTest {
 	);
 
 	protected $requiredExtensions = array(
-		'SiteTree' => array('Translatable'),
+		'SiteTree' => array('Translatable', 'Versioned', 'EveryoneCanPublish'),
 		'SiteConfig' => array('Translatable'),
 		'TranslatableTest_DataObject' => array('Translatable'),
 		'TranslatableTest_OneByLocaleDataObject' => array('Translatable'),
@@ -71,7 +71,7 @@ function testGetOneByLocale() {
 		Translatable::enable_locale_filter();
 
 		$found = Translatable::get_one_by_locale('TranslatableTest_OneByLocaleDataObject', $obj->Locale);
-                $this->assertNotNull($found, 'should have found one for ' . $obj->Locale);
+		$this->assertNotNull($found, 'should have found one for ' . $obj->Locale);
 		$this->assertEquals($obj->ID, $found->ID);
 
 		$translated = $obj->createTranslation('de_DE');
@@ -226,6 +226,12 @@ function testTranslationGroups() {
 		);
 	}
 
+	function assertClass($class, $node) {
+		$this->assertNotNull($node);
+		$this->assertEquals($class, $node->ClassName);
+		$this->assertEquals($class, get_class($node));
+	}
+
 	function testChangingClassOfDefaultLocaleTranslationChangesOthers() {
 		// see https://github.com/silverstripe/silverstripe-translatable/issues/97
 		// create an English SiteTree
@@ -247,12 +253,9 @@ function testChangingClassOfDefaultLocaleTranslationChangesOthers() {
 		$esPg = DataObject::get_by_id('Page', $esST->ID, $cache = false);
 
 		// make sure they are all the right class
-		$this->assertEquals('Page', $enPg->ClassName);
-		$this->assertEquals('Page', get_class($enPg));
-		$this->assertEquals('Page', $frPg->ClassName);
-		$this->assertEquals('Page', get_class($frPg));
-		$this->assertEquals('Page', $esPg->ClassName);
-		$this->assertEquals('Page', get_class($esPg));
+		$this->assertClass('Page', $enPg);
+		$this->assertClass('Page', $frPg);
+		$this->assertClass('Page', $esPg);
 
 		// test that we get the right translations back from each instance
 		$this->assertArrayEqualsAfterSort(
@@ -268,6 +271,36 @@ function testChangingClassOfDefaultLocaleTranslationChangesOthers() {
 			$esPg->getTranslations()->column('Locale')
 		);
 	}
+
+	function testChangingClassOfDefaultLocaleTranslationChangesOthersWhenPublished() {
+		// create an English SiteTree
+		$enST = new SiteTree();
+		$enST->Locale = 'en_US';
+		$enST->write();
+		$enST->doPublish();
+
+		// create and publish French and Spanish translations
+		$frST = $enST->createTranslation('fr_FR');
+		$this->assertTrue($frST->doPublish(), 'should have been able to publish French translation');
+		$esST = $enST->createTranslation('es_ES');
+		$this->assertTrue($esST->doPublish(), 'should have been able to publish Spanish translation');
+
+		// change the class name of the default locale's translation (as CMS admin would)
+		// and publish the change - we should see both versions of English change class
+		$enST->setClassName('Page');
+		$enST->doPublish();
+		$this->assertClass('Page', Versioned::get_one_by_stage('SiteTree', 'Stage', '"ID" = ' . $enST->ID));
+		$this->assertClass('Page', Versioned::get_one_by_stage('SiteTree', 'Live', '"ID" = ' . $enST->ID));
+
+		// and all of the draft versions of translations:
+		$this->assertClass('Page', Versioned::get_one_by_stage('SiteTree', 'Stage', '"ID" = ' . $frST->ID));
+		$this->assertClass('Page', Versioned::get_one_by_stage('SiteTree', 'Stage', '"ID" = ' . $esST->ID));
+
+		// and all of the live versions of translations as well:
+		$this->assertClass('Page', Versioned::get_one_by_stage('SiteTree', 'Live', '"ID" = ' . $frST->ID));
+		$this->assertClass('Page', Versioned::get_one_by_stage('SiteTree', 'Live', '"ID" = ' . $esST->ID));
+
+	}
 
 	function testTranslationGroupsWhenTranslationIsSubclass() {
 		// create an English SiteTree
@@ -303,16 +336,13 @@ function testTranslationGroupsWhenTranslationIsSubclass() {
 		$esPg = DataObject::get_by_id('Page', $esST->ID, $cache = false);
 
 		// make sure we successfully changed the class
-		$this->assertEquals('Page', $esPg->ClassName);
-		$this->assertEquals('Page', get_class($esPg));
+		$this->assertClass('Page', $esPg);
 
 		// and make sure that the class of the others did not change
 		$frST = DataObject::get_by_id('SiteTree', $frST->ID, $cache = false);
-		$this->assertEquals('SiteTree', $frST->ClassName);
-		$this->assertEquals('SiteTree', get_class($frST));
+		$this->assertClass('SiteTree', $frST);
 		$enST = DataObject::get_by_id('SiteTree', $enST->ID, $cache = false);
-		$this->assertEquals('SiteTree', $enST->ClassName);
-		$this->assertEquals('SiteTree', get_class($enST));
+		$this->assertClass('SiteTree', $enST);
 
 		// now that we know our edge case is successfully configured, we need to
 		// make sure that we get the right translations back from everything
@@ -1213,4 +1243,12 @@ function getCMSFields() {
 	}
 }
 
+
+class EveryoneCanPublish extends DataExtension {
+
+	function canPublish($member = null) {
+		return true;
+	}
+}
+
 TranslatableTest_DataObject::add_extension('TranslatableTest_Extension');