New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alert: Added ability to make notes for acking alerts + record who did so #8433

Merged
merged 6 commits into from Apr 26, 2018

Conversation

Projects
None yet
3 participants
@laf
Member

laf commented Mar 21, 2018

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

We don't have any user level checks on acks right now so I've not introduced that.

This allows you to set an ack msg for an alert but also update it at any stage.

It also records who ackd the alert to eventlog

@laf laf added the Alerting 🔔 label Mar 21, 2018

@laf laf added the Schema label Mar 21, 2018

@laf

This comment has been minimized.

Member

laf commented Apr 2, 2018

I've moved the schema file to 248

@murrant

This comment has been minimized.

Member

murrant commented Apr 6, 2018

During testing.

  1. The UI didn't change to show the alert was acknowledged (probably just refreshing bootgrid would work)
  2. It said successfully ack'd even though it failed because I had not updated my schema yet.
  3. In the notes, it would be better if it had a timestamp and user like Sat Jul 23 02:16:57 2005 - Ack (murrant) ISP is down
  4. Maybe change the notes icon to indicate if there is no note yet.
  5. A modal might look nicer, but I'm not fussed.
  6. Also, I tend to prefer button groups I think instead of individual columns.
  7. Not related, but Ack is yellow? Odd since you can set alerts to warning, which is yellow...
@laf

This comment has been minimized.

Member

laf commented Apr 9, 2018

The UI didn't change to show the alert was acknowledged (probably just refreshing bootgrid would work)

It does for me. The entire page refreshes which was how it was originally.

It said successfully ack'd even though it failed because I had not updated my schema yet.

I'd class that as a bug in general with how we use dbFacile. Updates that don't change anything return 0 rows updated as they do if an error occurs.

In the notes, it would be better if it had a timestamp and user like Sat Jul 23 02:16:57 2005 - Ack (murrant) ISP is down

I can add that.

Maybe change the notes icon to indicate if there is no note yet.

I can do that.

A modal might look nicer, but I'm not fussed.

Modals require a good amount of code for minimal gain on something like this.

Also, I tend to prefer button groups I think instead of individual columns.

The issue with that - as we have elsewhere - is people have to hover over the icon to work out what it does

Not related, but Ack is yellow? Odd since you can set alerts to warning, which is yellow...

What would you suggest.

@murrant

This comment has been minimized.

Member

murrant commented Apr 9, 2018

Light blue is acknowledged where I come from :-)

@laf

This comment has been minimized.

Member

laf commented Apr 21, 2018

image

image

How's all that now?

@murrant

This comment has been minimized.

Member

murrant commented Apr 25, 2018

I think two tweaks and this is good. If you agree with the behavior, I've attached a patch.

  1. append to notes instead of overwritting when acking
  2. delete notes when the alert clears
diff --git a/html/includes/forms/ack-alert.inc.php b/html/includes/forms/ack-alert.inc.php
index 4eb94c43c..2e7c60f98 100644
--- a/html/includes/forms/ack-alert.inc.php
+++ b/html/includes/forms/ack-alert.inc.php
@@ -48,7 +48,11 @@ if (!is_numeric($alert_id)) {
 
     $data = ['state' => $state, 'open' => $open];
     if (!empty($ack_msg)) {
-        $data['note'] = date(Config::get('dateformat.long')) . " - Ack ({$_SESSION['username']}) $ack_msg";
+        $note = dbFetchCell('SELECT note FROM alerts WHERE id=?', [$alert_id]);
+        if (!empty($note)) {
+            $note .= PHP_EOL;
+        }
+        $data['note'] = $note . date(Config::get('dateformat.long')) . " - Ack ({$_SESSION['username']}) $ack_msg";
     }
 
     if (dbUpdate($data, 'alerts', 'id=?', array($alert_id)) >= 0) {
diff --git a/includes/alerts.inc.php b/includes/alerts.inc.php
index 3427f32e4..2d34c7d5f 100644
--- a/includes/alerts.inc.php
+++ b/includes/alerts.inc.php
@@ -230,7 +230,7 @@ function RunRules($device_id)
                 c_echo('Status: %bNOCHG');
             } else {
                 if (dbInsert(array('state' => 0, 'device_id' => $device_id, 'rule_id' => $rule['id']), 'alert_log')) {
-                    if (!dbUpdate(array('state' => 0, 'open' => 1), 'alerts', 'device_id = ? && rule_id = ?', array($device_id,$rule['id']))) {
+                    if (!dbUpdate(array('state' => 0, 'open' => 1, 'note' => ''), 'alerts', 'device_id = ? && rule_id = ?', array($device_id,$rule['id']))) {
                         dbInsert(array('state' => 0, 'device_id' => $device_id, 'rule_id' => $rule['id'], 'open' => 1, 'alerted' => 0), 'alerts');
                     }
                     c_echo(PHP_EOL . 'Status: %gOK');
@laf

This comment has been minimized.

Member

laf commented Apr 25, 2018

Doesn't bother me too much either way so I've added it in.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Apr 25, 2018

The inspection completed: 751 Issues, 36 Patches

@murrant murrant merged commit 1eee6e8 into librenms:master Apr 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the laf:alert/issue-8353-ack branch Apr 26, 2018

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

alert: Added ability to make notes for acking alerts + record who did…
… so (librenms#8433)

* alert: Added ability to make notes for acking alerts + record who did so

* Updated schema

* moved sql file

* Updated from comments in PR

* warning changed to blue

* reset notes + keep notes on ack

@lock lock bot locked as resolved and limited conversation to collaborators Jun 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.