Fix review comments

Icinga · May 15, 2024 · 8cb8eae · 8cb8eae
1 parent c81fd09
commit 8cb8eae
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 75 deletions.
diff --git a/application/controllers/EventRuleController.php b/application/controllers/EventRuleController.php
@@ -34,17 +34,14 @@ class EventRuleController extends CompatController
     /** @var Session\SessionNamespace */
     private $sessionNamespace;
 
-    /** @var ?string Event rule config filter */
-    protected $filter;
-
     public function init()
     {
         $this->sessionNamespace = Session::getSession()->getNamespace('notifications');
+        $this->assertPermission('notifications/config/event-rule');
     }
 
     public function indexAction(): void
     {
-        $this->assertPermission('notifications/config/event-rule');
         $this->sessionNamespace->delete('-1');
 
         $this->addTitleTab(t('Event Rule'));
@@ -77,7 +74,7 @@ public function indexAction(): void
                 $form->removeRule((int) $ruleId);
                 $this->sessionNamespace->delete($ruleId);
                 Notification::success(sprintf(t('Successfully deleted event rule %s'), $configValues['name']));
-                $this->redirectNow('__CLOSE__');
+                $this->redirectNow(Links::eventRules());
             })
             ->on(EventRuleConfigForm::ON_DISCARD, function () use ($ruleId, $configValues) {
                 $this->sessionNamespace->delete($ruleId);
@@ -90,8 +87,9 @@ public function indexAction(): void
                 $this->redirectNow(Links::eventRule((int) $ruleId));
             })
             ->on(EventRuleConfigForm::ON_CHANGE, function (EventRuleConfigForm $form) use ($ruleId, $configValues) {
-                $configValues = array_merge($configValues, $form->getValues());
-                $configValues['rule_escalation'] = $form->getValues()['rule_escalation'];
+                $formValues = $form->getValues();
+                $configValues = array_merge($configValues, $formValues);
+                $configValues['rule_escalation'] = $formValues['rule_escalation'];
                 $this->sessionNamespace->set($ruleId, $configValues);
             })
             ->handleRequest($this->getServerRequest());
@@ -109,6 +107,7 @@ public function indexAction(): void
                     'formnovalidate' => true,
                 ]
             );
+
             $disableSave = false;
         }
 
@@ -121,6 +120,7 @@ public function indexAction(): void
                 'disabled' => $disableSave
             ]
         );
+
         $deleteButton = new SubmitButtonElement(
             'delete',
             [
@@ -139,12 +139,15 @@ public function indexAction(): void
                 ->filter(Filter::equal('rule.id', $ruleId));
 
             if ($incidents->count() > 0) {
-                $deleteButton->addAttributes(['disabled' => true]);
+                $deleteButton->addAttributes([
+                    'disabled' => true,
+                    'title'    => t('There exist active incidents for this escalation and hence cannot be removed')
+                ]);
             }
         }
 
         $eventRuleForm = Html::tag('div', ['class' => 'event-rule-form'], [
-            Html::tag('h2', $configValues['name'] ?? ''),
+            Html::tag('h2', $configValues['name']),
             (new Link(
                 new Icon('edit'),
                 Url::fromPath('notifications/event-rule/edit', [
@@ -153,8 +156,8 @@ public function indexAction(): void
                 ['class' => 'control-button']
             ))->openInModal()
         ]);
-        $this->addControl($eventRuleForm);
 
+        $this->addControl($eventRuleForm);
         $this->addControl($buttonsWrapper);
         $this->addContent($eventRuleConfig);
     }
@@ -302,21 +305,10 @@ public function editAction(): void
             ->on(Form::ON_SUCCESS, function ($form) use ($ruleId, $config) {
                 $config['name'] = $form->getValue('name');
                 $config['is_active'] = $form->getValue('is_active');
-                $params = [];
-                if ($ruleId === '-1') {
-                    $params = [
-                        'id'        => -1,
-                        'name'      => $config['name'],
-                        'is_active' => $config['is_active']
-                    ];
-                } else {
-                    $params['id'] = $ruleId;
-                }
-
                 if ($ruleId === '-1') {
-                    $redirectUrl = Url::fromPath('notifications/event-rules/add', $params);
+                    $redirectUrl = Url::fromPath('notifications/event-rules/add', ['id' => '-1']);
                 } else {
-                    $redirectUrl = Url::fromPath('notifications/event-rule', $params);
+                    $redirectUrl = Url::fromPath('notifications/event-rule', ['id' => $ruleId]);
                     $this->sendExtraUpdates(['#col1']);
                 }
 

diff --git a/application/controllers/EventRulesController.php b/application/controllers/EventRulesController.php
@@ -104,13 +104,13 @@ public function indexAction(): void
     public function addAction(): void
     {
         $this->addTitleTab(t('Add Event Rule'));
-        $this->getTabs()->setRefreshUrl(Url::fromPath('notifications/event-rules/add'));
+        $this->getTabs()->setRefreshUrl(Url::fromPath('notifications/event-rules/add', ['id' => '-1']));
 
         $this->controls->addAttributes(['class' => 'event-rule-detail']);
-        $ruleId = $this->params->get('id') ?? '-1';
+        $ruleId = $this->params->get('id');
 
-        $params = $this->params->toArray(false);
-        $config = $this->sessionNamespace->get($ruleId) ?? $params;
+        $config = $this->sessionNamespace->get($ruleId);
+        $config['object_filter'] = $config['object_filter'] ?? null;
 
         $eventRuleConfigSubmitButton = (new SubmitButtonElement(
             'save',
@@ -139,8 +139,9 @@ public function addAction(): void
                 $this->redirectNow(Links::eventRule($insertId));
             })
             ->on(EventRuleConfigForm::ON_CHANGE, function (EventRuleConfigForm $form) use ($config) {
-                $config = array_merge($config, $form->getValues());
-                $config['rule_escalation'] = $form->getValues()['rule_escalation'];
+                $formValues = $form->getValues();
+                $config = array_merge($config, $formValues);
+                $config['rule_escalation'] = $formValues['rule_escalation'];
                 $this->sessionNamespace->set('-1', $config);
             })
             ->handleRequest($this->getServerRequest());

diff --git a/application/forms/EventRuleConfigElements/EscalationCondition.php b/application/forms/EventRuleConfigElements/EscalationCondition.php
@@ -31,12 +31,15 @@ public function __construct(string $prefix, EventRuleConfigForm $configForm)
     {
         $this->prefix = $prefix;
         $this->configForm = $configForm;
-        parent::__construct('escalation-condition_' . $this->prefix, []);
+
+        parent::__construct('escalation-condition_' . $this->prefix);
     }
 
     protected function assemble(): void
     {
         $this->addElement('hidden', 'condition-count');
+        // Escalation Id to which the condition belongs
+        $this->addElement('hidden', 'id');
 
         $addCondition = $this->createElement(
             'submitButton',
@@ -62,8 +65,6 @@ protected function assemble(): void
             $conditionCount = $conditionCount === null ? $defaultCount : (int) $conditionCount;
         }
 
-        $this->addElement('hidden', 'id');
-
         if ($addCondition->hasBeenPressed()) {
             ++$conditionCount;
             if ($defaultCount === 0 && $conditionCount === 1) {
@@ -79,11 +80,9 @@ protected function assemble(): void
             return;
         }
 
-        if ($this->getAttributes()) {
-            $this->getAttributes()->remove('class', 'zero-escalation-condition');
-        }
-
+        $this->getAttributes()->remove('class', 'zero-escalation-condition');
         $removePosition = null;
+
         for ($i = 1; $i <= $conditionCount; $i++) {
             $col = $this->createElement(
                 'select',
@@ -155,6 +154,7 @@ protected function assemble(): void
                                     }
 
                                     $validator->clearMessages();
+
                                     return true;
                                 })
                             ]
@@ -173,6 +173,7 @@ protected function assemble(): void
             $this->registerElement($col);
             $this->registerElement($op);
             $this->registerElement($val);
+
             $removeButton = null;
 
             if (($conditionCount > 1) || ($conditionCount === 1 && ! $configHasZeroConditionEscalation)) {

diff --git a/application/forms/EventRuleConfigElements/EscalationRecipient.php b/application/forms/EventRuleConfigElements/EscalationRecipient.php
@@ -87,12 +87,13 @@ protected function assemble(): void
                     $options[''] = $this->translate('Default User Channel');
 
                     $val->setOptions($options);
-
                     $val->setDisabledOptions([]);
 
                     if ($this->getPopulatedValue('val_' . $i, '') === '') {
                         $val->addAttributes(['class' => 'default-channel']);
                     }
+                } else {
+                    $val->addAttributes(['required' => true]);
                 }
             } else {
                 $val = $this->createElement('text', 'val_' . $i, [

diff --git a/application/forms/EventRuleConfigElements/EventRuleConfigFilter.php b/application/forms/EventRuleConfigElements/EventRuleConfigFilter.php
@@ -25,12 +25,13 @@ public function __construct(Url $searchEditorUrl, ?string $filter)
     {
         $this->searchEditorUrl = $searchEditorUrl;
         $this->objectFilter = $filter;
+
         parent::__construct('config-filter');
     }
 
     protected function assemble(): void
     {
-        if (! $this->objectFilter) {
+        if (! $this->getObjectFilter()) {
             $addFilterButton = new SubmitButtonElement(
                 'add-filter',
                 [
@@ -68,7 +69,7 @@ protected function assemble(): void
             [
                 'class'    => ['filter-input', 'control-button'],
                 'readonly' => true,
-                'value'    => $this->objectFilter
+                'value'    => $this->getObjectFilter()
             ]
         );
 

diff --git a/application/forms/EventRuleConfigForm.php b/application/forms/EventRuleConfigForm.php
@@ -22,7 +22,6 @@
 use ipl\Stdlib\Filter;
 use ipl\Stdlib\Filter\Condition;
 use ipl\Web\Common\CsrfCounterMeasure;
-use ipl\Web\Common\FormUid;
 use Icinga\Module\Notifications\Forms\EventRuleConfigElements\EventRuleConfigFilter;
 use ipl\Web\Filter\QueryString;
 use ipl\Web\Url;
@@ -31,7 +30,6 @@
 class EventRuleConfigForm extends Form
 {
     use CsrfCounterMeasure;
-    use FormUid;
     use Translation;
 
     public const ON_DELETE = 'delete';
@@ -64,8 +62,8 @@ class EventRuleConfigForm extends Form
     public function __construct(array $config, Url $searchEditorUrl)
     {
         $this->config = $config;
-        $this->config['object_filter'] = $this->config['object_filter'] ?? null;
         $this->searchEditorUrl = $searchEditorUrl;
+
         $this->on(self::ON_SENT, function () {
             $config = array_merge($this->config, $this->getValues());
 
@@ -107,7 +105,6 @@ public function hasZeroConditionEscalation(): bool
     protected function assemble(): void
     {
         $this->addElement($this->createCsrfCounterMeasure(Session::getSession()->getId()));
-        $this->add($this->createUidElement());
 
         // Replicate save button outside the form
         $this->addElement(
@@ -140,13 +137,12 @@ protected function assemble(): void
         );
 
         $defaultEscalationPrefix = bin2hex('1');
-        $ruleId = $this->config['id'];
 
-        $this->addElement(
-            'hidden',
-            'zero-condition-escalation',
-            ['value' => $ruleId === '-1' ? $defaultEscalationPrefix : null]
-        );
+        $this->addElement('hidden', 'zero-condition-escalation');
+
+        if (! isset($this->config['rule_escalation'])) {
+            $this->getElement('zero-condition-escalation')->setValue($defaultEscalationPrefix);
+        }
 
         $configFilter = new EventRuleConfigFilter($this->searchEditorUrl, $this->config['object_filter']);
         $this->registerElement($configFilter);
@@ -273,8 +269,10 @@ public function populate($values): self
         foreach ($values['rule_escalation'] as $position => $escalation) {
             $conditions = explode('|', $escalation['condition'] ?? '');
             $conditionFormValues = [];
-            $conditionFormValues['condition-count'] = count($conditions);
+            $conditionCount = count($conditions);
+            $conditionFormValues['condition-count'] = $conditionCount;
             $conditionFormValues['id'] = $escalation['id'] ?? bin2hex(random_bytes(4));
+
             foreach ($conditions as $key => $condition) {
                 if ($condition === '' && ! isset($formValues['zero-condition-escalation'])) {
                     $formValues['zero-condition-escalation'] = bin2hex($position);
@@ -377,19 +375,15 @@ protected function createRemoveButton(string $prefix): SubmitButtonElement
     {
         /** @var array<int, array<string, mixed>> $escalations */
         $escalations = $this->config['rule_escalation'] ?? [];
-
         $pos = hex2bin($prefix);
         $disableRemoveButton = false;
-        if ($pos) {
-            $escalationId = $escalations[$pos]['id'] ?? null;
-            $incident = Incident::on(Database::get())
-                ->with('rule_escalation');
-
-            if ($escalationId && ctype_digit($escalationId)) {
-                $incident->filter(Filter::equal('rule_escalation.id', $escalationId));
-                if ($incident->count() > 0) {
-                    $disableRemoveButton = true;
-                }
+        $escalationId = $escalations[$pos]['id'] ?? null;
+
+        if ($escalationId && ctype_digit($escalationId)) {
+            $incident = Incident::on(Database::get())->with('rule_escalation');
+            $incident->filter(Filter::equal('rule_escalation.id', $escalationId));
+            if ($incident->count() > 0) {
+                $disableRemoveButton = true;
             }
         }
 
@@ -399,21 +393,14 @@ protected function createRemoveButton(string $prefix): SubmitButtonElement
                 'class'          => ['remove-escalation', 'remove-button', 'control-button', 'spinner'],
                 'label'          => new Icon('minus'),
                 'formnovalidate' => true,
-                'value'          => $prefix
+                'value'          => $prefix,
+                'disabled'       => $disableRemoveButton,
+                'title'          => $disableRemoveButton
+                    ? $this->translate('There exist active incidents for this escalation and hence cannot be removed')
+                    : $this->translate('Remove escalation')
             ]
         );
 
-        if ($disableRemoveButton) {
-            $button->addAttributes([
-                'disabled' => true,
-                'title'    => $this->translate(
-                    'There exist active incidents for this escalation and hence cannot be removed'
-                )
-            ]);
-        } else {
-            $button->addAttributes(['title' => $this->translate('Remove escalation')]);
-        }
-
         $this->registerElement($button);
 
         return $button;
@@ -465,13 +452,15 @@ public function addOrUpdateRule(int $id, array $config): int
             $escalationInCache = array_filter($escalationsInCache, function (array $element) use ($escalationId) {
                 /** @var string $idInCache */
                 $idInCache = $element['id'] ?? null;
+
                 return (int) $idInCache === $escalationId;
             });
 
             if ($escalationInCache) {
                 $position = array_key_first($escalationInCache);
                 // Escalations in DB to update
                 $escalationsToUpdate[$position] = $escalationInCache[$position];
+
                 unset($escalationsInCache[$position]);
             } else {
                 // Escalation in DB to remove
@@ -573,16 +562,19 @@ function (array $element) use ($recipientId) {
                         $data['contact_id'] = $recipientConfig['contact_id'];
                         $data['contactgroup_id'] = null;
                         $data['schedule_id'] = null;
+
                         break;
                     case isset($recipientConfig['contactgroup_id']):
                         $data['contact_id'] = null;
                         $data['contactgroup_id'] = $recipientConfig['contactgroup_id'];
                         $data['schedule_id'] = null;
+
                         break;
                     case isset($recipientConfig['schedule_id']):
                         $data['contact_id'] = null;
                         $data['contactgroup_id'] = null;
                         $data['schedule_id'] = $recipientConfig['schedule_id'];
+
                         break;
                 }
 

diff --git a/library/Notifications/Widget/ItemList/EscalationConditionList.php b/library/Notifications/Widget/ItemList/EscalationConditionList.php
@@ -28,11 +28,9 @@ public function __construct(array $conditions)
     protected function assemble(): void
     {
         $removedPosition = null;
-        $conditionCount = count($this->conditions);
         foreach ($this->conditions as $position => $condition) {
             if ($condition->hasBeenRemoved()) {
                 $removedPosition = $position;
-                --$conditionCount;
 
                 continue;
             }