Skip to content

feat: filter which event (UP/DOWN/CERT-EXPIRY) triggers alerts #5770

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DVDAndroid
Copy link

@DVDAndroid DVDAndroid commented Apr 12, 2025

📋 Overview

This PR introduces the ability to customize which types of events trigger notifications.

🔄 Changes

Migration on notification table: column trigger of type text and default value up,down,certificate.

Changed edit monitor page and edit notification dialog.

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify):
    • Provide additional details here.

🔗 Related Issues

📄 Checklist *

  • 🔍 My code adheres to the style guidelines of this project.
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

📷 Screenshots or Visual Changes

immagine
immagine

immagine

ℹ️ Additional Context

💬 Requested Feedback

@DVDAndroid DVDAndroid marked this pull request as draft April 12, 2025 12:27
@DVDAndroid DVDAndroid force-pushed the feat/notification-trigger-type branch from c03d09e to e191d13 Compare April 12, 2025 12:33
@CommanderStorm
Copy link
Collaborator

I don't like how this looks in the UI (this does not reaaaly fit the rest of the UI).
Here are some pointers:

  • Expired certificates are avaliable here https://expired.badssl.com/
  • Please use a multiselector instead. I think that is much more user friendly than the current dropdown
  • please have a look at how we space/align the rest of the UI, that is currently quite messy
  • More foundamental: Not sure where in the notification window this should be placed and how.
    Currently looks quite out of place. I think by default, this should be more out of the way
    => Maybe a link text at the bottom "customise which alerts trigger this notification provider"
  • I think the ability to customise for each monitor (instead of just at the notification provider level) what alerts trigger the notification provider is likely overkill and looks quite messy => lets just display an information tag icon which on hover shows that this does only trigger for XYZ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter which event (UP/DOWN/CERT-EXPIRY/...) triggers alerts
2 participants