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

Allow admins to add/remove/create sticky notifications #9865

Merged
merged 3 commits into from Mar 5, 2019

Conversation

Projects
None yet
4 participants
@cppmonkey
Copy link
Contributor

commented Feb 23, 2019

Allow admins to add/remove/create sticky notifications
Added auth error to ajax form - notifications
$status is already 'error'

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.

webui:
Allow admins to add/remove/create sticky notifications
Added auth error to ajax form - notifications
	$status is already 'error'
@cppmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

I've left a present on the Demo site, do you intend for Demo Users to be able to add notifications?

The code still works that way, but now regular users won't be able to mark notifications as read.
This is now reserved for Admins and... ehh, Demo users

https://demo.librenms.org/notifications

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

Hi @cppmonkey
Could you have a look at Travis CI tests and correct the errors ? It seems to be only minor typo/spaces:

FILE: ...uild/librenms/librenms/html/includes/forms/notifications.inc.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 36 | ERROR | [x] Expected 1 space after IF keyword; 0 found
 36 | ERROR | [x] Expected 0 spaces after opening bracket; 1 found
@murrant

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

This seems to be a cleanup rather than a semantic change, or did I miss something? Yes, Demo users should be able to do most everything an admin can do.

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Nope, I see the ! now...

I think demo is included in hasGlobalAdmin... so you could drop the or

@cppmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

An Admin could not make changes and yet the demo user could.

It's a bit of a clean up with an extra error. As it just said Unknown Error.

@murrant

This comment has been minimized.

@cppmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I asked about that on Discord, as I realised it covered both.
The consensus was that things might change or that it wasn't clear global admin included demo, without inspecting the function body. There fore I left it in

The ! Before hasGlobalAdmin was stopping actual admins but isDemo allowed demo users to make changes

@cppmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Just amazed no one brought it up.

I had to go into the database to delete some stickied notifications a few months ago.

Only recently drilled down to the fault, as I wanted to sticky the php minimum version.

The ! is such an easy thing to over look.

The snmpsim validation was amazing! Just wish I'd had it working on my dev branch sooner. Could have saved a few commits with the mammoth PR

Test driven is the bees knees haha!

@murrant

murrant approved these changes Mar 4, 2019

@murrant murrant merged commit 7e71d2e into librenms:master Mar 5, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate 2 fixed issues
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant changed the title webui: Allow admins to add/remove/create sticky notifications Allow admins to add/remove/create sticky notifications Mar 5, 2019

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

webui: Allow admins to add/remove/create sticky notifications (libren…
…ms#9865)

* webui:
Allow admins to add/remove/create sticky notifications
Added auth error to ajax form - notifications
	$status is already 'error'

* fix: code style

* remove demo check

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2019

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