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

feature: ability to edit default alert template #7121

Merged
merged 6 commits into from Aug 12, 2017

Conversation

Projects
None yet
3 participants
@aldemira
Contributor

aldemira commented Aug 4, 2017

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

@aldemira aldemira changed the title from newfeature: ability to edit default alert template to feature: ability to edit default alert template Aug 4, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 4, 2017

Member

Wow you went for the full thing :)

I probably can't test till Sunday.

Member

laf commented Aug 4, 2017

Wow you went for the full thing :)

I probably can't test till Sunday.

@laf laf added the WebUI label Aug 4, 2017

@aldemira

This comment has been minimized.

Show comment
Hide comment
@aldemira

aldemira Aug 8, 2017

Contributor

Good thing that you didn't test it, I forgot to push the sql file.

Contributor

aldemira commented Aug 8, 2017

Good thing that you didn't test it, I forgot to push the sql file.

Show outdated Hide outdated includes/alerts.inc.php
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 8, 2017

Member

I also think it's worth adding an FAQ along the lines of, My 'Default Alert Template' no longer exists and tell people to run mysql -u librenms -p < sql-schema/202.sql maybe?

Member

laf commented Aug 8, 2017

I also think it's worth adding an FAQ along the lines of, My 'Default Alert Template' no longer exists and tell people to run mysql -u librenms -p < sql-schema/202.sql maybe?

@laf laf added the Schema label Aug 8, 2017

@aldemira

This comment has been minimized.

Show comment
Hide comment
@aldemira

aldemira Aug 9, 2017

Contributor

Do you think any other documentation is needed?

Contributor

aldemira commented Aug 9, 2017

Do you think any other documentation is needed?

@@ -28,6 +28,7 @@ source: Support/FAQ.md
- [What is the Difference between Disable Device and Ignore a Device?](#faq28)
- [Why can't Normal and Global View users see Oxidized?](#faq29)
- [What is the Demo User for?](#faq30)
- [Why does modifying 'Default Alert Template' fail?](#faq31)

This comment has been minimized.

@laf

laf Aug 9, 2017

Member

I'd say it needs to be why does the default alert template not appear in the templates list.

If it's not in the DB you won't see it to even modify it.

@laf

laf Aug 9, 2017

Member

I'd say it needs to be why does the default alert template not appear in the templates list.

If it's not in the DB you won't see it to even modify it.

This comment has been minimized.

@aldemira

aldemira Aug 10, 2017

Contributor

Actually, the listing on the on alert templates page is static. So it'll appear whether the record is in the db or not.

@aldemira

aldemira Aug 10, 2017

Contributor

Actually, the listing on the on alert templates page is static. So it'll appear whether the record is in the db or not.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Aug 9, 2017

Member

I think the doc needs clarification more.

Also, you seem to have removed the update to alerts.inc.php so the new DB template isn't used if no others exist, it will just fall back to the inbuilt template.

Member

laf commented Aug 9, 2017

I think the doc needs clarification more.

Also, you seem to have removed the update to alerts.inc.php so the new DB template isn't used if no others exist, it will just fall back to the inbuilt template.

@aldemira

This comment has been minimized.

Show comment
Hide comment
@aldemira

aldemira Aug 10, 2017

Contributor

Apparently I didn't quite understand your first change req. Anyway what happens with the latest commit is:
alert.inc.php checks for any associated templates.
If not found retrieve the default one from the db.
If we do not have that, just use the built-in one.

Contributor

aldemira commented Aug 10, 2017

Apparently I didn't quite understand your first change req. Anyway what happens with the latest commit is:
alert.inc.php checks for any associated templates.
If not found retrieve the default one from the db.
If we do not have that, just use the built-in one.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Aug 10, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Aug 10, 2017

The inspection completed: No new issues

@laf laf merged commit 34d0fc3 into librenms:master Aug 12, 2017

2 checks passed

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

@aldemira aldemira deleted the aldemira:defaultalerttmpl branch Sep 25, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

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