FormDataFilter: simplify HTML, Code and style

application/forms/IcingaServiceForm.php

@@ -163,15 +163,21 @@ class IcingaServiceForm extends DirectorObjectForm
         $this->addElement('dataFilter', 'assign_filter', array(
             'columns' => IcingaHost::enumProperties($this->db),
             'required' => true,
+            'description' => $this->translate(
+                'This allows you to configure an assignment filter. Please feel'
+                . ' free to combine as many nested operators as you want'
+            )
         ));
         $el = $this->getElement('assign_filter');
-        $el->setDecorators(array(
+
-            'ViewHelper',
+        $el->clearDecorators()
-            array('HtmlTag', array(
+            ->addDecorator('ViewHelper')
-                'tag' => 'ul',
+            ->addDecorator('Errors')
-                'class' => 'assign-rule required'
+            ->addDecorator('Description', array('tag' => 'p', 'class' => 'description'))
-            )),
+            ->addDecorator('HtmlTag', array(
-        ));
+                'tag'   => 'dd',
+                'class' => 'full-width required',
+            ));
 
         $this->addDisplayGroup(array($el), 'assign', array(
             'decorators' => array(

application/views/helpers/FormDataFilter.php

@@ -59,41 +59,37 @@ class Zend_View_Helper_FormDataFilter extends Zend_View_Helper_FormElement
             $value = Filter::fromQueryString($value);
         }
 
-        return $this->renderFilter($value);
+        return $this->beginRoot()
+            . $this->renderFilter($value)
+            . $this->endRoot();
     }
 
-    protected function renderFilter(Filter $filter, $level = 0)
+    protected function renderFilter(Filter $filter)
     {
-        if ($level === 0 && (
-            ($filter->isChain() && $filter->isEmpty())
-            || $filter->isExpression())) {
-            $pre = '<ul class="filter-expression filter-root"><li class="active">';
-            $post = '</li></ul>';
-        } else {
-            $pre = $post = '';
-        }
-
         if ($filter instanceof FilterChain) {
-            return $pre . $this->renderFilterChain($filter, $level) . $post;
+            return $this->renderFilterChain($filter);
         } elseif ($filter instanceof FilterExpression) {
-            return $pre . $this->renderFilterExpression($filter, $level) . $post;
+            return $this->renderFilterExpression($filter);
         } else {
             throw new ProgrammingError('Got a Filter being neither expression nor chain');
         }
     }
 
-    protected function emptyExpression()
+    protected function beginRoot()
     {
-        return Filter::expression('', '=', '');
+        return '<ul class="filter-root">';
     }
 
-    protected function renderFilterChain(FilterChain $filter, $level)
+    protected function endRoot()
+    {
+        return '</ul>';
+    }
+
+    protected function renderFilterChain(FilterChain $filter)
     {
         $parts = array();
         foreach ($filter->filters() as $f) {
-            $parts[] = '<li>'
+            $parts[] = $this->renderFilter($f);
-                . $this->renderFilter($f, $level + 1)
-                . '</li>';
         }
 
         return $this->beginChain($filter)
@@ -103,12 +99,9 @@ class Zend_View_Helper_FormDataFilter extends Zend_View_Helper_FormElement
 
     protected function beginChain(FilterChain $filter)
     {
-        $root = $filter->isRootNode() === 0 ? ' class="filter-root"' : '';
+        $list = $filter->isEmpty() ? '' : '<ul>' . "\n";
 
-        $list = $filter->isEmpty() ? '' : '<ul' . $root . '>' . "\n";
+        return '<li class="filter-chain"><span class="handle"> </span>'
-
-        return '<li><div class="filter-chain'
-             . '"><span class="handle"> </span>'
              . $this->selectOperator($filter)
              . $this->removeLink($filter)
              . $this->addLink($filter)
@@ -119,13 +112,12 @@ class Zend_View_Helper_FormDataFilter extends Zend_View_Helper_FormElement
     protected function endChain(FilterChain $filter)
     {
         $list = $filter->isEmpty() ? '' : "</ul>\n";
-        return $list . "</div></li>\n";
+        return $list . "</li>\n";
     }
 
     protected function beginExpression(FilterExpression $filter)
     {
-        $root = $filter->isRootNode() === 0 ? ' filter-root' : '';
+        return '<div class="filter-expression">' . "\n";
-        return '<div class="filter-expression' . $root . '">' . "\n";
     }
 
     protected function endExpression(FilterExpression $filter)
@@ -133,23 +125,35 @@ class Zend_View_Helper_FormDataFilter extends Zend_View_Helper_FormElement
         return "</div>\n";
     }
 
-    protected function filterExpressionHtml(FilterExpression $filter, $level)
+    protected function beginElement(FilterExpression $filter)
+    {
+        return '<div class="expression-wrapper">' . "\n";
+    }
+
+    protected function endElement(FilterExpression $filter)
+    {
+        return "</div>\n";
+    }
+
+    protected function filterExpressionHtml(FilterExpression $filter)
     {
         return $this->selectColumn($filter)
              . $this->selectSign($filter)
+             . $this->beginElement($filter)
              . $this->element($filter)
+             . $this->endElement($filter)
              . $this->removeLink($filter)
              . $this->expandLink($filter);
     }
 
-    protected function renderFilterExpression(FilterExpression $filter, $level)
+    protected function renderFilterExpression(FilterExpression $filter)
     {
         return $this->beginExpression($filter)
-             . $this->filterExpressionHtml($filter, $level)
+             . $this->filterExpressionHtml($filter)
              . $this->endExpression($filter);
     }
 
-    protected function element(Filter $filter = null)
+    protected function element(FilterExpression $filter = null)
     {
         if ($filter) {
             // TODO: Make this configurable
@@ -217,9 +221,9 @@ class Zend_View_Helper_FormDataFilter extends Zend_View_Helper_FormElement
         );
     }
 
-    protected function renderNewFilter()
+    protected function emptyExpression()
     {
-        return $this->renderFilterExpression($this->emptyExpression(), 0);
+        return Filter::expression('', '=', '');
     }
 
     protected function arrayForSelect($array, $flip = false)
@@ -340,7 +344,7 @@ class Zend_View_Helper_FormDataFilter extends Zend_View_Helper_FormElement
             $this->elementId('column', $filter),
             $cols,
             $active,
-            array('class' => 'autosubmit')
+            array('class' => 'column autosubmit')
         );
     }
 

library/Director/Objects/IcingaObject.php

@@ -1942,7 +1942,7 @@ abstract class IcingaObject extends DbObject implements IcingaConfigRenderer
         // @codingStandardsIgnoreEnd
         return '    ' . AssignRenderer::forFilter(
             Filter::fromQueryString($this->assign_filter)
-        )->renderAssign();
+        )->renderAssign() . "\n";
     }
 
     public function toLegacyConfigString()

library/Director/Web/Form/Element/DataFilter.php

@@ -25,6 +25,28 @@ class DataFilter extends FormElement
 
     private $filter;
 
+    public function getValue()
+    {
+        $value = parent::getValue();
+        if ($value !== null && $this->isEmpty($value)) {
+            $value = null;
+        }
+
+        return $value;
+    }
+
+    protected function isEmpty(Filter $filter)
+    {
+        return $filter->isEmpty() || $this->isEmptyExpression($filter);
+    }
+
+    protected function isEmptyExpression($filter)
+    {
+        return $filter->isExpression() &&
+            $filter->getColumn() === '' &&
+            $filter->getExpression() === '""'; // -> json_encode('')
+    }
+
     /**
      * @codingStandardsIgnoreStart
      */
@@ -150,7 +172,10 @@ class DataFilter extends FormElement
                 }
             }
             foreach ($filter->filters() as $sub) {
-                $filter->replaceById($sub->getId(), $this->fixNotsWithMultipleChildrenForFilter($sub));
+                $filter->replaceById(
+                    $sub->getId(),
+                    $this->fixNotsWithMultipleChildrenForFilter($sub)
+                );
             }
         }
 
@@ -270,15 +295,36 @@ class DataFilter extends FormElement
         return null;
     }
 
+    protected function hasIncompleteExpressions(Filter $filter)
+    {
+        if ($filter->isChain()) {
+            foreach ($filter->filters() as $sub) {
+                if ($this->hasIncompleteExpressions($sub)) {
+                    return true;
+                }
+            }
+        } else {
+            if ($filter->isRootNode() && $this->isEmptyExpression($filter)) {
+                return false;
+            }
+
+            return $filter->getColumn() === '';
+        }
+    }
+
     public function isValid($value, $context = null)
     {
         if (! $value instanceof Filter) {
             // TODO: try, return false on E
             $filter = $this->arrayToFilter($value);
+            $this->setValue($filter);
         }
 
-        $this->setValue($filter);
+        if ($this->hasIncompleteExpressions($filter)) {
+            $this->addError('The configured filter is incomplete');
+            return false;
+        }
 
-        return true;
+        return parent::isValid($value);
     }
 }

public/css/module.less

@@ -186,6 +186,7 @@ input, select, select option, textarea {
 
 form ul.form-errors {
     margin-bottom: 0.5em;
+
     ul.errors li {
         background: @color-critical;
         font-weight: bold;
@@ -624,48 +625,6 @@ form dt label {
     }
 }
 
-ul.assign-rule {
-    margin: 0;
-    padding: 0;
-    list-style-type: none;
-    select, input[type=text] {
-        min-width: auto;
-    }
-
-    select.assign-type {
-        width: 8em;
-    }
-
-    select.assign-operator {
-        width: 3em;
-    }
-
-    select.assign-property {
-        width: 12em;
-    }
-
-    input.assign-expression {
-        width: 12em;
-    }
-
-    ul {
-        list-style-type: none;
-        padding-left: 2em;
-
-        li::before {
-            // icon: down-dir
-            font-family: 'ifont';
-            content: '\e81d';
-            // icon: right-small
-            content: '\e877';
-            margin-left: -1em;
-            padding-top: 0em;
-            float: left;
-            color: inherit;
-        }
-    }
-}
-
 form fieldset {
     min-width: 36em;
 }
@@ -710,6 +669,10 @@ form dt {
     }
 }
 
+form .errors label {
+    color: @color-critical;
+}
+
 form dd {
     display: inline-block;
     width: 63%;
@@ -721,6 +684,11 @@ form dd {
             border-color: @color-critical;
         }
     }
+
+    &.full-width {
+        padding: 0.5em;
+        width: 100%;
+    }
 }
 
 form dd:after {
@@ -1123,11 +1091,41 @@ table.config-diff {
 }
 
 
+ul.assign-rule {
+    margin: 0;
+    padding: 0;
+    list-style-type: none;
+    select, input[type=text] {
+        min-width: auto;
+    }
+
+    select.assign-type {
+        width: 8em;
+    }
+
+
+    ul {
+        list-style-type: none;
+        padding-left: 2em;
+
+        li::before {
+            // icon: down-dir
+            font-family: 'ifont';
+            content: '\e81d';
+            // icon: right-small
+            content: '\e877';
+            margin-left: -1em;
+            padding-top: 0em;
+            float: left;
+            color: inherit;
+        }
+    }
+}
 
 ul.filter-root {
     margin-top: 0;
     width: 100%;
-    padding-left: 1.5em;
+    padding-left: 0.5em;
     list-style-type: none;
 
     ul {
@@ -1136,11 +1134,13 @@ ul.filter-root {
         width: 100%;
     }
 
-    li {
+    li.filter-chain, div.filter-expression {
         width: 100%;
+        padding: 0.3em 0.5em;
+        min-width: 30em;
     }
 
-    li::before {
+    ul li.filter-chain::before, ul .filter-expression::before {
        font-family: 'ifont';
         // Formerly: icon-down-open: e821
         // icon-right-small:
@@ -1159,63 +1159,75 @@ ul.filter-root {
             content: none;
         }
     }
+
+    input[type=submit].icon-button {
+        display: none;
+        font-family: 'ifont';
+        font-weight: normal;
+        background: none;
+        border: none;
+        padding: 0.2em 0.4em 0.2em 0.4em;
+        margin: 0 0 0 0.2em;
+    }
+
+    .active input[type=submit].icon-button,
+    li:hover input[type=submit].icon-button,
+    div:hover input[type=submit].icon-button
+    {
+        display: inline;
+    }
 }
 
-div.filter-chain > select.operator {
+.errors > ul.filter-root {
+    input[type=text], select {
+        border-color: transparent;
+        border-bottom-color: @gray-lighter;
+    }
+
+    select.column, select.operator {
+        border-left-color: @color-critical;
+    }
+}
+
+
+li.filter-chain > select.operator {
     min-width: 5em;
     max-width: 5em;
     width: 5em;
 }
 
-div.filter-expression  select.sign, ul.filter-expression  select.sign {
+div.filter-expression {
-    min-width: 4em;
+    .column {
-    max-width: 4em;
+        min-width: 7em;
-    width: 4em;
-
-    &.wide {
-        min-width: 10em;
         max-width: 10em;
         width: 10em;
     }
-}
 
-div.filter-expression, ul.filter-expression {
+    .sign {
-    select {
+        min-width: 4em;
-        min-width: 7em;
+        max-width: 4em;
-        max-width: 7em;
+        width: 4em;
-        width: 7em;
+        margin: 0 0.3em;
+        &.wide {
+            min-width: 6em;
+            max-width: 6em;
+            width: 6em;
+        }
     }
 
-    input[type=text] {
+    div.expression-wrapper {
+        display: inline-block;
+    }
+
+    div.expression-wrapper > input[type=text], 
+    div.expression-wrapper > select {
         min-width: 7em;
-        max-width: 7em;
+        width: 10em;
-        width: 7em;
+        max-width: 10em;
     }
 }
 
-ul.assign-rule {
-  select.assign-type {
-    vertical-align: top;
-  }
 
-  div.filter-expression {
-    display: inline-block;
-  }
-}
-
-input[type=submit].icon-button {
-  display: none;
-  font-family: 'ifont';
-  font-weight: normal;
-  background: none;
-  border: none;
-  padding: 0.2em 0.4em 0.2em 0.4em;
-  margin: 0 0 0 0.2em;
-}
-
-.active input[type=submit].icon-button, li:hover > div > input[type=submit].icon-button  {
-  display: inline;
-}
 
 
 

public/js/module.js

@@ -188,7 +188,7 @@
             }
             var $li = $input.closest('li');
             var $dt = $dd.prev();
-            var $form = $dt.closest('form');
+            var $form = $dd.closest('form');
 
             $form.find('dt, dd, li').removeClass('active');
             $li.addClass('active');