Skip to content
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

Corrected active_count for Alert icon color #9933

Merged
merged 6 commits into from Mar 14, 2019

Conversation

Projects
None yet
2 participants
@PipoCanaja
Copy link
Contributor

commented Mar 11, 2019

Hi,

I did receive questions from my colleagues on the Alert Icon color in the menubar. Red or green does not follow the implicit logic.

After a check, it seems that the SQL calculating it is wrong. As far as I see it, we want:

  • Green if all alerts in DB are OK (status 0) or ACK (status 2)
  • Red in any other cases.

That means : AND `alerts`.`state` NOT IN (0,2)' instead of AND `alerts`.`state` NOT IN (0,1)' as it is now.
Moreover, Devices with disabled or ignore state should not be taken into account.

Here is a patch for legacy and laravel icon coloring.

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
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

We should also probably ignore "devices.ignore = 1" in the SQL, as ignore means "ignore alerts" ...

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@PipoCanaja yeah, can you check the functionality of the Laravel menu (menu.blade.php and MenuComposer.php)

We might be able to remove the old menu and just use the Laravel menu. That is technically possible, just not sure about the details.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Not clear on how to test these. Should we review and merge this first (so we correct the behaviour of the legacy code) and I'll check on how to test the laravel version ?

@PipoCanaja PipoCanaja changed the title WIP Corrected active_count for Alert icon color Corrected active_count for Alert icon color Mar 12, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Go to the all locations page (/locations) if both are broken, I'd like to fix them both in the same PR.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

correct, issue is not corrected on locations. So I'll dig that.

@PipoCanaja PipoCanaja changed the title Corrected active_count for Alert icon color WIP - Corrected active_count for Alert icon color Mar 12, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@murrant Alert Transports are not in the Laravel version of the menu.
Legacy:
Capture d’écran 2019-03-12 à 19 28 48

Laravel:
Capture d’écran 2019-03-12 à 19 28 42

That's out of scope of this PR. New PR #9946 started.

@PipoCanaja PipoCanaja changed the title WIP - Corrected active_count for Alert icon color Corrected active_count for Alert icon color Mar 12, 2019

Show resolved Hide resolved app/Models/AlertRule.php Outdated

@PipoCanaja PipoCanaja force-pushed the PipoCanaja:alert_icon_color branch from 2b37b79 to 2023bf7 Mar 13, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Tests in my environnement are good and results are the same with Laravel and legacy code.

@murrant murrant merged commit 787a8fc into librenms:master Mar 14, 2019

5 of 6 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@PipoCanaja PipoCanaja deleted the PipoCanaja:alert_icon_color branch Mar 17, 2019

@murrant murrant removed the Bug 🐞 label Apr 1, 2019

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

Corrected active_count for Alert icon color (librenms#9933)
* Corrected active_count for Alert icon color

* Corrected active_count for Alert icon color

* Corrected active_count for Alert icon color

* Laravel version

* Laravel version check status worse and better for isActive

* cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.