BUG Use non-destructive pubilshing for editable options

Fixes #544

code/model/editableformfields/EditableFormField.php

@@ -16,6 +16,7 @@ use SilverStripe\Forms\SegmentField;
  * @property string $CustomErrorMessage
  * @method UserDefinedForm Parent() Parent page
  * @method DataList DisplayRules() List of EditableCustomRule objects
+ * @mixin Versioned
  */
 class EditableFormField extends DataObject
 {
@@ -482,16 +483,31 @@ class EditableFormField extends DataObject
      * Publish this Form Field to the live site
      *
      * Wrapper for the {@link Versioned} publish function
+     *
+     * @param string $fromStage
+     * @param string $toStage
+     * @param bool $createNewVersion
      */
     public function doPublish($fromStage, $toStage, $createNewVersion = false)
     {
         $this->publish($fromStage, $toStage, $createNewVersion);
+        $this->publishRules($fromStage, $toStage, $createNewVersion);
+	}
 
-		$seenIDs = array();
+    /**
+     * Publish all field rules
+     *
+     * @param string $fromStage
+     * @param string $toStage
+     * @param bool $createNewVersion
+     */
+    protected function publishRules($fromStage, $toStage, $createNewVersion)
+    {
+        $seenRuleIDs = array();
 
         // Don't forget to publish the related custom rules...
         foreach ($this->DisplayRules() as $rule) {
-			$seenIDs[] = $rule->ID;
+            $seenRuleIDs[] = $rule->ID;
             $rule->doPublish($fromStage, $toStage, $createNewVersion);
             $rule->destroy();
         }
@@ -500,8 +516,8 @@ class EditableFormField extends DataObject
         $rules = Versioned::get_by_stage('EditableCustomRule', $toStage)
             ->filter('ParentID', $this->ID);
 
-		if (!empty($seenIDs)) {
+        if (!empty($seenRuleIDs)) {
-            $rules = $rules->exclude('ID', $seenIDs);
+            $rules = $rules->exclude('ID', $seenRuleIDs);
         }
 
         foreach ($rules as $rule) {
@@ -528,7 +544,7 @@ class EditableFormField extends DataObject
     }
 
     /**
-     * checks wether record is new, copied from Sitetree
+     * checks whether record is new, copied from SiteTree
      */
     public function isNew()
     {
@@ -593,7 +609,7 @@ class EditableFormField extends DataObject
     /**
      * Set the allowed css classes for the extraClass custom setting
      *
-     * @param array The permissible CSS classes to add
+     * @param array $allowed The permissible CSS classes to add
      */
     public function setAllowedCss(array $allowed)
     {

code/model/editableformfields/EditableMultipleOptionField.php

@@ -87,25 +87,45 @@ class EditableMultipleOptionField extends EditableFormField
      * When publishing it needs to handle copying across / publishing
      * each of the individual field options
      *
-     * @return void
+     * @param string $fromStage
+     * @param string $toStage
+     * @param bool $createNewVersion
      */
     public function doPublish($fromStage, $toStage, $createNewVersion = false)
     {
-        $live = Versioned::get_by_stage("EditableOption", "Live", "\"EditableOption\".\"ParentID\" = $this->ID");
+        parent::doPublish($fromStage, $toStage, $createNewVersion);
-
+        $this->publishOptions($fromStage, $toStage, $createNewVersion);
-        if ($live) {
-            foreach ($live as $option) {
-                $option->deleteFromStage('Live');
-            }
     }
 
-        if ($this->Options()) {
+
+    /**
+     * Publish list options
+     *
+     * @param string $fromStage
+     * @param string $toStage
+     * @param bool $createNewVersion
+     */
+    protected function publishOptions($fromStage, $toStage, $createNewVersion)
+    {
+        $seenIDs = array();
+
+        // Publish all options
         foreach ($this->Options() as $option) {
+            $seenIDs[] = $option->ID;
             $option->publish($fromStage, $toStage, $createNewVersion);
         }
+
+        // remove any orphans from the "fromStage"
+        $options = Versioned::get_by_stage('EditableOption', $toStage)
+            ->filter('ParentID', $this->ID);
+
+        if (!empty($seenIDs)) {
+            $options = $options->exclude('ID', $seenIDs);
         }
 
-        parent::doPublish($fromStage, $toStage, $createNewVersion);
+        foreach ($options as $rule) {
+            $rule->deleteFromStage($toStage);
+        }
     }
 
     /**

tests/UserFormsVersionedTest.php

@@ -0,0 +1,49 @@
+<?php
+
+
+class UserFormsVersionedTest extends SapphireTest
+{
+    protected static $fixture_file = 'UserDefinedFormTest.yml';
+
+    public function setUp()
+    {
+        parent::setUp();
+        Versioned::reading_stage('Stage');
+    }
+
+    public function testPublishing()
+    {
+        /** @var UserDefinedForm $form */
+        $form = $this->objFromFixture('UserDefinedForm', 'filtered-form-page');
+
+        // Get id of options
+        $optionID = $this->idFromFixture('EditableOption', 'option-3');
+        $this->assertEmpty(Versioned::get_one_by_stage('EditableOption', 'Live', array('"ID" = ?' => $optionID)));
+
+        // Publishing writes this to live
+        $form->doPublish();
+        $liveVersion = Versioned::get_versionnumber_by_stage('EditableOption', 'Live', $optionID, false);
+        $this->assertNotEmpty($liveVersion);
+
+        // Add new option, and repeat publish process
+        /** @var EditableCheckboxGroupField $list */
+        $list = $this->objFromFixture('EditableCheckboxGroupField', 'checkbox-group');
+        $newOption = new EditableOption();
+        $newOption->Title = 'New option';
+        $newOption->Value = 'ok';
+        $newOption->write();
+        $newOptionID = $newOption->ID;
+        $list->Options()->add($newOption);
+
+        $form->doPublish();
+
+        // Un-modified option should not create a new version
+        $newLiveVersion = Versioned::get_versionnumber_by_stage('EditableOption', 'Live', $optionID, false);
+        $this->assertNotEmpty($newLiveVersion);
+        $this->assertEquals($liveVersion, $newLiveVersion);
+
+        // New option is successfully published
+        $newOptionLiveVersion = Versioned::get_versionnumber_by_stage('EditableOption', 'Live', $newOptionID, false);
+        $this->assertNotEmpty($newOptionLiveVersion);
+    }
+}