SyncProperty: simplify code and form

fixes #12700

application/forms/SyncPropertyForm.php

@@ -71,60 +71,8 @@ class SyncPropertyForm extends DirectorObjectForm
             }
         }
 
-        if ($destination === 'import') {
+        $this->addSourceColumnElement($destination);
-            $funcTemplates = 'enum' . ucfirst($this->rule->object_type) . 'Templates';
-            $templates = $this->db->$funcTemplates();
-            if (! empty($templates)) {
-                $templates = array_combine($templates, $templates);
-            }
 
-            $this->addElement('select', 'source_expression', array(
-                'label'        => $this->translate('Template'), // Objecttype?
-                'multiOptions' => $this->optionalEnum($templates),
-                'required'     => true,
-                'class'        => 'autosubmit',
-            ));
-        } else {
-            $this->addSourceColumnElement();
-        }
-
-        if ($this->hasObject()) {
-            if (($col = $this->getObject()->getSourceColumn()) === null) {
-                $this->setElementValue('source_column', self::EXPRESSION);
-                $this->addElement('text', 'source_expression', array(
-                    'label'    => $this->translate('Source Expression'),
-                    'required' => true,
-                ));
-                if ($this->getSentValue('source_column') === '${' . self::EXPRESSION . '}') {
-                    unset($this->source_column);
-                }
-            } else {
-                $this->setElementValue('source_column', $col);
-            }
-        }
-
-        if ($this->getSentValue('source_column') === self::EXPRESSION) {
-            $this->addElement('text', 'source_expression', array(
-                'label'    => $this->translate('Source Expression'),
-                'required' => true,
-            ));
-            if ($this->getSentValue('source_column') === '${' . self::EXPRESSION . '}') {
-                unset($this->source_column);
-            }
-        }
-
-        /*
-        if ($this->hasObject()) {
-            // TODO: Add up/down links to table
-            $this->addElement('text', 'priority', array(
-                'label'       => $this->translate('Priority'),
-                'description' => $this->translate('Priority for the specified source expression'),
-                'required'    => true,
-            ));
-        }
-        */
-
-        // TODO: we need modifier
         $this->addElement('YesNo', 'use_filter', array(
             'label'        => $this->translate('Set based on filter'),
             'ignore'       => true,
@@ -160,7 +108,8 @@ class SyncPropertyForm extends DirectorObjectForm
             $this->addElement('select', 'merge_policy', array(
                 'label'        => $this->translate('Merge Policy'),
                 'description'  => $this->translate(
-                    'Whether you want to merge or replace the destination field. Makes no difference for strings'
+                    'Whether you want to merge or replace the destination field.'
+                    . ' Makes no difference for strings'
                 ),
                 'required'     => true,
                 'multiOptions' => $this->optionalEnum(array(
@@ -176,25 +125,59 @@ class SyncPropertyForm extends DirectorObjectForm
 
     }
 
-    protected function addSourceColumnElement()
+    protected function hasSubOption($options, $key)
+    {
+        foreach ($options as $mainKey => $sub) {
+            if (! is_array($sub)) {
+                // null -> please choose - or similar
+                continue;
+            }
+
+            if (array_key_exists($key, $sub)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    protected function addSourceColumnElement($destination)
     {
         $error = false;
+
+        $srcTitle = $this->translate('Source columns');
         try {
-            $data = $this->listSourceColumns();
+            $columns[$srcTitle] = $this->listSourceColumns();
-            natsort($data);
+            natsort($columns[$srcTitle]);
         } catch (Exception $e) {
-            $data = array();
+            $srcTitle .= sprintf(' (%s)', $this->translate('failed to fetch'));
+            $columns[$srcTitle] = array();
             $error = sprintf(
                 $this->translate('Unable to fetch data: %s'),
                 $e->getMessage()
             );
         }
 
+        if ($destination === 'import') {
+            $funcTemplates = 'enum' . ucfirst($this->rule->object_type) . 'Templates';
+            $templates = $this->db->$funcTemplates();
+            if (! empty($templates)) {
+                $templates = array_combine($templates, $templates);
+            }
+
+            $importTitle = $this->translate('Existing templates');
+            $columns[$importTitle] = $templates;
+            natsort($columns[$importTitle]);
+        }
+
+        $xpTitle = $this->translate('Expert mode');
+        $columns[$xpTitle][self::EXPRESSION] = $this->translate('Custom expression');
+
         $this->addElement('select', 'source_column', array(
             'label'        => $this->translate('Source Column'),
-            // TODO: List them as ${} ?
+            'multiOptions' => $this->optionalEnum($columns),
-            'multiOptions' => $this->optionalEnum($data),
             'required'     => true,
+            'ignore'       => true,
             'class'        => 'autosubmit',
         ));
 
@@ -202,6 +185,35 @@ class SyncPropertyForm extends DirectorObjectForm
             $this->getElement('source_column')->addError($error);
         }
 
+        $showExpression = false;
+
+        if ($this->hasBeenSent()) {
+            $sentValue = $this->getSentValue('source_column');
+            if ($sentValue === self::EXPRESSION) {
+                $showExpression = true;
+            }
+        } elseif ($this->hasObject()) {
+            $objectValue = $this->getObject()->source_expression;
+            if ($this->hasSubOption($columns, $objectValue)) {
+                $this->setElementValue('source_column', $objectValue);
+            } else {
+                $this->setElementValue('source_column', self::EXPRESSION);
+                $showExpression = true;
+            }
+        }
+
+        if ($showExpression) {
+            $this->addElement('text', 'source_expression', array(
+                'label'       => $this->translate('Source Expression'),
+                'description' => $this->translate(
+                    'A custom string. Might contain source columns, please use placeholders'
+                    . ' of the form ${columnName} in such case'
+                ),
+                'required'    => true,
+            ));
+        }
+
+
         return $this;
     }
 
@@ -232,9 +244,11 @@ class SyncPropertyForm extends DirectorObjectForm
 
     protected function listSourceColumns()
     {
-        $columns = $this->getImportSource()->listColumns();
+        $columns = array();
-        $columns = array_combine($columns, $columns);
+        foreach ($this->getImportSource()->listColumns() as $col) {
-        $columns[self::EXPRESSION] = $this->translate('Custom expression');
+            $columns['${' . $col . '}'] = $col;
+        }
+
         return $columns;
     }
 
@@ -308,16 +322,6 @@ class SyncPropertyForm extends DirectorObjectForm
 
     public function onSuccess()
     {
-        $sourceColumn = $this->getValue('source_column');
-        if ($sourceColumn === self::EXPRESSION) {
-            unset($this->source_column);
-            $this->removeElement('source_column');
-        } else {
-            if (! $this->getElement('source_expression')) {
-                $this->addHidden('source_expression', '${' . $sourceColumn . '}');
-            }
-        }
-
         $object = $this->getObject();
         $object->rule_id = $this->rule->id; // ?!
 
@@ -325,6 +329,14 @@ class SyncPropertyForm extends DirectorObjectForm
             $object->filter_expression = null;
         }
 
+        $sourceColumn = $this->getValue('source_column');
+        unset($this->source_column);
+        $this->removeElement('source_column');
+
+        if ($sourceColumn !== self::EXPRESSION) {
+           $object->source_expression = $sourceColumn;
+        }
+
         $destination = $this->getValue('destination_field');
         if ($destination === 'vars.*') {
             $destination = $this->getValue('customvar');

library/Director/Objects/SyncProperty.php

@@ -22,35 +22,4 @@ class SyncProperty extends DbObject
         'filter_expression' => null,
         'merge_policy'      => null
     );
-
-    /**
-     * Virtual property for source_column
-     *
-     * Internally we always use an expression. Form indirectly uses this
-     *
-     * Avoid complaints for method names with underscore:
-     * @codingStandardsIgnoreStart
-     *
-     * @return self
-     */
-    public function setSource_column($value)
-    {
-        // @codingStandardsIgnoreEnd
-        $this->source_expression = '${' . $value . '}';
-        return $this;
-    }
-
-    public function sourceIsSingleColumn()
-    {
-        return $this->getSourceColumn() !== null;
-    }
-
-    public function getSourceColumn()
-    {
-        if (preg_match('/^\${([A-Za-z0-9_-]+)}$/', $this->source_expression, $m)) {
-            return $m[1];
-        }
-
-        return null;
-    }
 }