Event rule config enhancement #159

raviks789 · 2024-02-21T13:19:57Z

Use a single form for event rule configuration.

Blocked by Icinga/icingaweb2#5190

application/forms/EventRuleConfigForm.php

application/controllers/EventRuleController.php

application/forms/EventRuleConfigForm.php

sukhwinder33445

Looks good to me, works fine.

@nilmerg please have a look.

sukhwinder33445 · 2024-05-15T14:40:28Z

Please squash the commits.

sukhwinder33445 · 2024-05-17T11:04:14Z

Please do not forget to keep an eye on the phpstan errors.

sukhwinder33445

LGTM

sukhwinder33445 · 2024-06-20T12:12:20Z

library/Notifications/Model/Contact.php

 use ipl\Orm\Relations;

 /**
 * @property int $id
+ * @property string $full_name
+ * @property ?string $username
+ * @property string $color


Remove please.

sukhwinder33445 · 2024-06-20T12:17:08Z

library/Notifications/Common/Links.php

+    {
+        return Url::fromPath(
+            "notifications/event-rule/complete",
+            ['_disableLayout' => true, 'showCompact' => true, 'id' => $id]


Please do not add any model-related parameters here. This class should only provide basic links. I already had a discussion with Johannes about this topic. You can add more parameters while calling this function, or remove them altogether and create them in a specific class if required.

sukhwinder33445 · 2024-06-20T12:24:08Z

library/Notifications/Model/Contact.php

+ * @property Channel | Query $channel
+ * @property ContactAddress | Query $contact_address
+ * @property Incident | Query $incident
+ * @property IncidentContact | Query $incident_contact
+ * @property IncidentHistory | Query $incident_history
+ * @property RuleEscalationRecipient | Query $rule_escalation_recipient
+ * @property RotationMember | Query $rotation_member
+ * @property ContactAddress | Query $contactgroup


This is optional, Most of the existing and upcoming annotations are defined as following:
@property Query|Channel $channel. (without a space between |)
This way the code looks consistent.

…escalations

nilmerg

Not finished yet. Will continue tomorrow.

nilmerg · 2024-06-25T11:13:38Z

application/forms/EventRuleConfigForm.php

+            $config = array_merge($this->config, $this->getValues());
+
+            if ($config !== $this->config) {
+                $this->emit(self::ON_CHANGE, [$this]);


This must not be emitted without validating the CSRF token first.

nilmerg · 2024-06-25T11:17:36Z

application/forms/EventRuleConfigForm.php

+                $this->emit(self::ON_DELETE, [$this]);
+            } elseif ($buttonName === 'discard_changes') {
+                $this->emit(self::ON_DISCARD, [$this]);


I don't like that this method emits events. Do it like it's done here.

nilmerg · 2024-06-25T11:29:20Z

application/forms/EventRuleConfigForm.php

+            ]
+        );
+
+        $defaultEscalationPrefix = bin2hex('1');


Please get rid of that bin2hex call

nilmerg · 2024-06-25T11:34:28Z

application/forms/EventRuleConfigForm.php

+        $configFilter = new EventRuleConfigFilter($this->searchEditorUrl, $this->config['object_filter']);
+        $this->registerElement($configFilter);
+
+        $addEscalationButton = new SubmitButtonElement(


Please use $this->createElement() instead

nilmerg · 2024-06-25T11:41:51Z

application/forms/EventRuleConfigForm.php

+        $removeEscalationButtons = [];
+
+        if ($escalationCount > 1) {
+            foreach ($prefixesMap as $prefixMap) {


$prefixMap is a member of $prefixesMap, so $prefix is more suited here

It's also more a list than a map, but never mind..

nilmerg · 2024-06-25T13:09:31Z

application/forms/EventRuleConfigForm.php

+        $formValues['prefixes-map'] = $this->getPrefixesMap(count($values['rule_escalation']));
+
+        foreach ($values['rule_escalation'] as $position => $escalation) {
+            $conditions = explode('|', $escalation['condition'] ?? '');


It's odd to me that these are parsed here. For one, in getValues() they aren't built, so that must be done somewhere else, so why are they parsed here? And second, they're parsed by hand, meaning, not by QueryString::parse. I hope this is just a mistake here, and where ever they're built, this is not also done by hand.

nilmerg · 2024-06-25T13:12:26Z

application/forms/EventRuleConfigForm.php

+                    continue;
+                }
+
+                $count = $key + 1;


$position, no?

nilmerg · 2024-06-25T13:19:41Z

application/forms/EventRuleConfigForm.php

+                /** @var Condition $filter */
+                $filter = QueryString::parse($condition);
+                $conditionFormValues['column_' . $count] = $filter->getColumn() === 'placeholder'
+                    ? null
+                    : $filter->getColumn();
+
+                if ($conditionFormValues['column_' . $count]) {
+                    $conditionFormValues['type_' . $count] = $conditionFormValues['column_' . $count];
+                }
+
+                $conditionFormValues['operator_' . $count] = QueryString::getRuleSymbol($filter);
+                $conditionFormValues['val_' . $count] = $filter->getValue();
+            }
+
+            $formValues['escalation-condition_' . bin2hex($position)] = $conditionFormValues;
+            $recipientFormValues = [];
+            if (isset($escalation['recipients'])) {
+                $recipientFormValues['recipient-count'] = count($escalation['recipients']);
+                foreach ($escalation['recipients'] as $key => $recipient) {
+                    if (is_array($recipient)) {
+                        $count = 0;
+                        foreach ($recipient as $elementName => $elementValue) {
+                            if ($elementValue === null) {
+                                continue;
+                            }
+
+                            $count = $key + 1;
+                            $selectedOption = str_replace('id', $elementValue, $elementName, $replaced);
+                            if ($replaced && $elementName !== 'channel_id') {
+                                $recipientFormValues['column_' . $count] = $selectedOption;
+                            } elseif ($elementName === 'channel_id') {
+                                $recipientFormValues['val_' . $count] = $elementValue;
+                            }
+                        }
+
+                        if (isset($recipient['id'])) {
+                            $recipientFormValues['id_' . $count] = (int) $recipient['id'];
+                        }
+                    }
+                }
+            }


Now I'm sure something is off here. Much of this is split here and in EscalationCondition as well as EscalationRecipient.

It's bad that processing of input and output is split that way. There should be only one location where this is done, and these are the iframes to me. They accept and provide form data as well as config. They know best what structure is needed where. No need to spread so much intimate knowledge around.

nilmerg · 2024-06-25T13:20:34Z

application/forms/EventRuleConfigForm.php

+        $disableRemoveButton = false;
+        $escalationId = $escalations[$pos]['id'] ?? null;
+
+        if ($escalationId && ctype_digit($escalationId)) {


What else could these be than digits?

This was used because I am assigning hexadecimal values as fake IDs for escalations that are not yet saved. In this case using is_numeric to avoid the call to database does not work as that would be true even if the IDs are hexadecimal.

nilmerg · 2024-06-25T13:21:12Z

application/forms/EventRuleConfigForm.php

+            $disableRemoveButton = $incidentCount > 0;
+        }
+
+        $button = new SubmitButtonElement(


Again, please use $this->createElement

nilmerg

next: controllers, fieldsets, css

nilmerg · 2024-06-26T11:08:23Z

library/Notifications/Widget/ItemList/EscalationConditionList.php

+
+class EscalationConditionList extends BaseHtmlElement
+{
+    protected $defaultAttributes = ['class' => 'options'];


I'm not a fan of such generic class names. The problem with these is sometimes that they get undesired styles applied due to some random rule defined somewhere else. So please use a less generic name. Such as escalation-condition-list 😉

nilmerg · 2024-06-26T11:09:33Z

library/Notifications/Widget/ItemList/EscalationConditionList.php

+    protected $conditions;
+
+    /**
+     * Create conditions list of the escalation


Create a list of escalation conditions

nilmerg · 2024-06-26T11:12:04Z

library/Notifications/Widget/ItemList/EscalationConditionList.php

+                continue;
+            }
+
+            if ($removedPosition) {


Just for a litte bit of safety: (I know positions start at 1)

Suggested change

if ($removedPosition) {

if ($removedPosition !== null) {

nilmerg · 2024-06-26T11:13:23Z

library/Notifications/Widget/ItemList/EscalationConditionList.php

+            }
+        }
+
+        foreach ($this->conditions as $position => $condition) {


why two loops? One should suffice.

nilmerg · 2024-06-26T11:14:21Z

library/Notifications/Widget/ItemList/EscalationRecipientList.php

+
+class EscalationRecipientList extends BaseHtmlElement
+{
+    protected $defaultAttributes = ['class' => 'options'];


escalation-recipient-list

nilmerg · 2024-06-26T11:24:42Z

library/Notifications/Widget/ItemList/EscalationConditionListItem.php

+        return $this;
+    }
+
+    public function setRemoveButton(?SubmitButtonElement $removeButton): self


missing doc

nilmerg · 2024-06-26T11:25:35Z

library/Notifications/Widget/ItemList/EscalationRecipientListItem.php

+
+class EscalationRecipientListItem extends BaseHtmlElement
+{
+    protected $defaultAttributes = ['class' => 'option'];


you know the drill

nilmerg · 2024-06-26T11:27:08Z

library/Notifications/Widget/ItemList/EscalationRecipientListItem.php

+        return $this;
+    }
+
+    public function removeRemoveButton(): self


missing doc

nilmerg · 2024-06-26T11:49:52Z

library/Notifications/Widget/ItemList/Escalation.php

+    protected $recipient;
+
+    /** @var bool Whether the widget has a remove button */
+    protected $hasNoRemoveButton = false;


This is always false, if I'm not mistaken, and as such unused.

nilmerg · 2024-06-26T11:51:43Z

library/Notifications/Widget/ItemList/Escalation.php

+    /**
+     * Create first component of the escalation widget
+     *
+     * @return ?FlowLine|SubmitButtonElement


this cannot return null

nilmerg

next: controllers, css

nilmerg · 2024-06-26T13:27:37Z

application/forms/EventRuleConfigElements/EventRuleConfigFilter.php

+            $this->registerElement($addFilterButton);
+
+            if ($addFilterButton->hasBeenPressed()) {
+                $this->removeAttribute('class', 'empty-filter');


If it's just added a few lines below, why is it removed here? Seems useless to me.

nilmerg · 2024-06-26T13:29:03Z

application/forms/EventRuleConfigElements/EventRuleConfigFilter.php

+     *
+     * @return ?string
+     */
+    public function getObjectFilter(): ?string


This is a getter, so please move it near the constructor

nilmerg · 2024-06-26T13:30:06Z

application/forms/EventRuleConfigElements/EscalationRecipient.php

+
+    public function __construct($name)
+    {
+        parent::__construct('escalation-recipient_' . $name, []);


If you don't pass any attributes, don't pass an argument

nilmerg · 2024-06-26T13:33:49Z

application/forms/EventRuleConfigElements/EscalationRecipient.php

+        $count = $this->getValue('recipient-count');
+        $removePosition = $this->getValue('remove');
+        if ($removePosition) {
+            $count += 1;


Why the addition? If something has been removed, I'd expect a subtraction.

Please explain. And add a comment afterwards.

nilmerg · 2024-06-26T13:38:05Z

application/forms/EventRuleConfigElements/EscalationRecipient.php

+            $value['id'] = $this->getValue('id_' . $i);
+
+            /** @var ?string $columnName */
+            $columnName = $this->getValue('column_' . $i);


You really need to invest more time in naming your variables. This isn't a column name, it's a [recipient_]type.

nilmerg · 2024-06-26T13:44:49Z

application/forms/EventRuleConfigElements/EscalationRecipient.php

+            $options['Contacts']['contact_' . $contact->id] = $contact->full_name;
+        }
+
+        /** @var Contactgroup $contactgroup */
+        foreach (Contactgroup::on(Database::get()) as $contactgroup) {
+            $options['Contact Groups']['contactgroup_' . $contactgroup->id] = $contactgroup->name;
+        }
+
+        /** @var Schedule $schedule */
+        foreach (Schedule::on(Database::get()) as $schedule) {
+            $options['Schedules']['schedule_' . $schedule->id] = $schedule->name;


translate the labels

nilmerg · 2024-06-26T13:48:31Z

application/forms/EventRuleConfigElements/EscalationRecipient.php

+                'column_' . $i,
+                [
+                    'class'           => ['autosubmit', 'left-operand'],
+                    'options'         => $defaultOption + $this->fetchOptions(),


please fetch those options just once..

nilmerg · 2024-06-26T13:49:22Z

application/forms/EventRuleConfigElements/EscalationRecipient.php

+
+            $this->registerElement($col);
+
+            $options = $defaultOption + Channel::fetchChannelNames(Database::get());


same here, just once please.

I really wonder why I have to comment on this!

nilmerg · 2024-06-26T14:04:38Z

application/forms/EventRuleConfigElements/EscalationCondition.php

+        $filter = Filter::any();
+        $removePosition = (int) $this->getValue('remove');
+        if ($removePosition) {
+            $count += 1;


same here, comment please

nilmerg

Finito. For now.

I didn't look at CSS, will do this at the very end.

Since the controllers will receive a rather large rework, I didn't look that detailed over them as in the previous reviews. Will do this once they're adjusted.

nilmerg · 2024-06-27T10:39:50Z

application/controllers/EventRulesController.php

@@ -45,6 +45,7 @@ public function init()
    public function indexAction(): void
    {
        $eventRules = Rule::on(Database::get());
+        $this->sessionNamespace->delete('-1');


If I reload the left column while in the right a new rule is about to be created, this clears all my changes made so far.

The session storage should only be reset if absolutely necessary, which is when a new one is being set up.

Note that, as I just saw that the New Event Rule button used to have a parameter with such functionality, you should use a separate action for new event rules. This then should clear the session before writing anything to it upon submit.

/And now I noticed that this very action is already the addAction here, but that's used to set up a new rule. i.e. It's not just the endpoint for the very first modal, it's a detail.

I think that there doesn't need to be a distinction between event-rule?id=-1 and event-rule?id!=-1. The event-rule/index route should be able to handle new rules (=-1) and existing ones (!=-1). There's not much difference between them, only a few buttons and event handlers. By transferring the responsibility to create buttons to the form, this is already not part of the action anymore. So that only leaves the events and those are just not emitted in case of a new rule, so it doesn't really hurt that they are defined.

So in the end:

event-rules/add is the endpoint for the the New Event Rule button, this resets the session upon submit
event-rule/index is the endpoint to configure new and existing rules
event-rule/edit is the endpoint to edit new and existing rules

sukhwinder33445 · 2024-07-01T13:39:46Z

Removing an escalation condition writes '' (empty string) (rule_escalation.condition='') instead of null to the database.

cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Feb 21, 2024

raviks789 force-pushed the event-rule-config-enhancement branch 4 times, most recently from c40476a to 8e16e7e Compare February 28, 2024 09:14

raviks789 force-pushed the event-rule-config-enhancement branch 13 times, most recently from 1c9054f to 7ab548f Compare March 6, 2024 12:35

raviks789 force-pushed the event-rule-config-enhancement branch 9 times, most recently from 25b4a12 to 163112c Compare March 13, 2024 09:32

raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 97018d5 to 4b1380c Compare March 18, 2024 13:08

sukhwinder33445 reviewed May 15, 2024

View reviewed changes

application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved

sukhwinder33445 reviewed May 15, 2024

View reviewed changes

application/controllers/EventRuleController.php Outdated Show resolved Hide resolved

application/forms/EventRuleConfigForm.php Outdated Show resolved Hide resolved

raviks789 requested a review from sukhwinder33445 May 15, 2024 13:47

raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 8b5bffe to be66793 Compare May 15, 2024 14:28

sukhwinder33445 previously approved these changes May 15, 2024

View reviewed changes

raviks789 dismissed sukhwinder33445’s stale review via d31e8a6 May 16, 2024 12:29

raviks789 force-pushed the event-rule-config-enhancement branch from be66793 to d31e8a6 Compare May 16, 2024 12:29

raviks789 force-pushed the event-rule-config-enhancement branch from d31e8a6 to fd35468 Compare May 21, 2024 15:52

nilmerg removed this from the Beta milestone May 24, 2024

sukhwinder33445 previously approved these changes May 31, 2024

View reviewed changes

raviks789 dismissed sukhwinder33445’s stale review via c77091c June 4, 2024 08:25

raviks789 force-pushed the event-rule-config-enhancement branch from fd35468 to c77091c Compare June 4, 2024 08:25

raviks789 force-pushed the event-rule-config-enhancement branch from c77091c to 437291d Compare June 20, 2024 10:17

sukhwinder33445 reviewed Jun 20, 2024

View reviewed changes

raviks789 and others added 6 commits June 20, 2024 15:10

Build a single form for the event rule configuration

2b0614e

Remove unnecessary type hints

8bf6fdd

Remove unused files

c0658ca

PHPStan: Update baseline

166cd53

EventRuleConfigForm: Modify the HTML structure of filter wrapper and …

1aa3eef

…escalations

Fix alignment issue for select elements in safari

c00a2f1

raviks789 force-pushed the event-rule-config-enhancement branch from 437291d to c00a2f1 Compare June 20, 2024 13:10

nilmerg requested changes Jun 25, 2024

View reviewed changes

nilmerg requested changes Jun 26, 2024

View reviewed changes

nilmerg requested changes Jun 27, 2024

View reviewed changes


		$this->registerElement($col);

		$options = $defaultOption + Channel::fetchChannelNames(Database::get());

Event rule config enhancement #159

Are you sure you want to change the base?

Event rule config enhancement #159

Conversation

raviks789 commented Feb 21, 2024 • edited Loading

sukhwinder33445 left a comment

Choose a reason for hiding this comment

sukhwinder33445 commented May 15, 2024

sukhwinder33445 commented May 17, 2024

sukhwinder33445 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilmerg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilmerg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilmerg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilmerg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukhwinder33445 commented Jul 1, 2024

raviks789 commented Feb 21, 2024 •

edited

Loading