From 8cb8eae2a96ec2373c2c26eba94dd220f6a16f55 Mon Sep 17 00:00:00 2001 From: raviks789 Date: Wed, 15 May 2024 12:27:45 +0200 Subject: [PATCH] Fix review comments --- .../controllers/EventRuleController.php | 38 +++++------- .../controllers/EventRulesController.php | 13 ++-- .../EscalationCondition.php | 15 ++--- .../EscalationRecipient.php | 3 +- .../EventRuleConfigFilter.php | 5 +- application/forms/EventRuleConfigForm.php | 60 ++++++++----------- .../ItemList/EscalationConditionList.php | 2 - 7 files changed, 61 insertions(+), 75 deletions(-) diff --git a/application/controllers/EventRuleController.php b/application/controllers/EventRuleController.php index af8643569..a72387782 100644 --- 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 index 576cd3752..2934e2034 100644 --- 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 index cf46b4b8b..a906ace48 100644 --- 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 index 8f5956eb8..46e9d2a3d 100644 --- 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 index 79369bb3b..b723be2ae 100644 --- 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 index e195e639c..579d8a367 100644 --- 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> $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,6 +452,7 @@ 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; }); @@ -472,6 +460,7 @@ public function addOrUpdateRule(int $id, array $config): int $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 index 8bf252075..ba503e572 100644 --- 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; }