Skip to content

Conversation

raviks789
Copy link
Contributor

@raviks789 raviks789 commented Feb 21, 2024

Use a single form for event rule configuration.

Blocked by Icinga/icingaweb2#5190

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Feb 21, 2024
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 4 times, most recently from c40476a to 8e16e7e Compare February 28, 2024 09:14
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 13 times, most recently from 1c9054f to 7ab548f Compare March 6, 2024 12:35
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 9 times, most recently from 25b4a12 to 163112c Compare March 13, 2024 09:32
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 97018d5 to 4b1380c Compare March 18, 2024 13:08
@nilmerg nilmerg force-pushed the event-rule-config-enhancement branch 2 times, most recently from 119c821 to 044a8a2 Compare August 13, 2025 14:33
@nilmerg
Copy link
Member

nilmerg commented Aug 13, 2025

Should be done now. Did change nearly everything again, because the most of what was previously there was not usable and the plethora of other open pull requests referring to this one were either obsolete due to it anyway or now integrated here.

Requires Icinga/ipl-html#151

@raviks789 Please have a look and test it.

The tests fail because they require: Icinga/ipl-web#307

Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only a quick review. I will test the UI tomorrow.
Some new classes are missing PHPDoc and method return types.

@sukhwinder33445
Copy link
Contributor

As discussed offline, I would suggest to change the order of the buttons to as follows:
Screenshot 2025-08-19 at 14 03 24
This would also provide in some way protection to prevent accidentally clicking on the Delete button instead of Save.

@nilmerg
Copy link
Member

nilmerg commented Aug 21, 2025

It now requires Icinga/ipl-html#153

@sukhwinder33445
Copy link
Contributor

Class RightArrow is not used anywhere and can be removed.

The class FlowLine can also be removed, as the CSS classes added by this class apply exactly the same rules. The getVerticalLine() method is not used. Instead of applying class right-arrow or horizontal-line, you can add class line connector or connector-line. Instead of the right-arrow and horizontal-line class, you can add the something like connector or connector-line class.

@sukhwinder33445
Copy link
Contributor

I'm not sure if this is the expected behavior. It is possible that there are character strings that contain operator symbols. these should then remain as they are and not be separated into column and value.

Before After
Screenshot 2025-08-28 at 10 07 11 Screenshot 2025-08-28 at 10 07 22
Screenshot 2025-08-28 at 10 08 55 Screenshot 2025-08-28 at 10 09 13
Screenshot 2025-08-28 at 10 07 53 Screenshot 2025-08-28 at 10 08 05

Customvars allows it too.
Screenshot 2025-08-28 at 10 33 56

@nilmerg
Copy link
Member

nilmerg commented Aug 28, 2025

Fixed. Though, now the shown filter shows escaped chars again. I'm not sure whether it's necessary to avoid this here. The object filters will change anyway and when they do, I doubt that such a simple text input will remain. So let's keep that for now.

@sukhwinder33445
Copy link
Contributor

Fixed. Though, now the shown filter shows escaped chars again. I'm not sure whether it's necessary to avoid this here. The object filters will change anyway and when they do, I doubt that such a simple text input will remain. So let's keep that for now.

Now the database entry has the operator as a suffix, is that OK?

@nilmerg
Copy link
Member

nilmerg commented Aug 28, 2025

In case the daemon accepts it, sure :P

@sukhwinder33445
Copy link
Contributor

@oxzi, that should be fine as discussed offline, right?

@oxzi
Copy link
Member

oxzi commented Aug 28, 2025

In case the daemon accepts it, sure :P

Well, based on the architectural change described in the "Custom variable support in Notifications" document, the event rules will not be evaluated in the Icinga Notifications daemon anymore, but will be moved to the source.

Implementation started in Icinga/icinga-notifications#324 and Icinga/icingadb#998.

Thus, I am a bit puzzled how this PR works with the upcoming changes, especially the intermediate representation mentioned in the docs.

@oxzi, that should be fine as discussed offline, right?

I was unable to review +2,762 −2,829 changes in 34 files, but at least having a suffix is something which works for us 🙃

@nilmerg nilmerg force-pushed the event-rule-config-enhancement branch from 8a3f063 to 9d5c1c6 Compare September 1, 2025 11:10
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

@nilmerg nilmerg merged commit 6d193d1 into main Sep 1, 2025
10 checks passed
@nilmerg nilmerg deleted the event-rule-config-enhancement branch September 1, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/view Affects an entire view cla/signed CLA is signed by all contributors of a PR enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite Event Rule Configuration
4 participants