Add notifications for changes to monitor messages #4865
Conversation
…monitor message, if monitor is configured for it
|
@louislam I'm not sure if this will be something your interested in or not. The description / naming may need to change (both variables and UI). No hurt feelings if you don't think it's useful, but I think it could be a useful feature. My main use case is watching DNS records, but I think it would work with a number of monitors. I didn't limit the UI option to any specific monitor, as I'm not sure which monitors change their "message" field without a status change. I'm not sure if the |
|
Oh, also worth noting - this change includes the I saw a comment in |
CommanderStorm
added
area:notifications
| <div class="my-3 form-check"> | ||
| <input id="monitor-change" v-model="monitor.watchChanges" class="form-check-input" type="checkbox"> | ||
| <label class="form-check-label" for="monitor-change"> | ||
| <!-- todo use the $t thing here --> |
There was a problem hiding this comment.
@ maintainers I'm not sure what the $t thing was or where it came from, so I left a TODO.
If the PR is something you'd consider accepting, what do I need to change to utilize that instead of hard-coding the descriptions?
There was a problem hiding this comment.
$t is a shorthand for a localised string.
It grabs the translation key provided in the $t("KEY") and looks it up here: https://github.com/louislam/uptime-kuma/blob/master/src/lang/en.json
Docs: https://vue-i18n.intlify.dev/
what do I need to change to utilize that instead of hard-coding the descriptions
| <!-- todo use the $t thing here --> | |
| {{ $t("Monitor Changes") }} |
And then add the translation key Monitor Changes here https://github.com/louislam/uptime-kuma/blob/master/src/lang/en.json
the second point is nessesary as otherwise our awsome translators can't translate this.
There was a problem hiding this comment.
I have left a few comments below.
What I am torn about is if this is the best place to put this setting (if we add such a setting).
I think we could simplify the app by either adding it to the settings.
What I am also unsure if this is nessesary.
=> What is your inital motivation? Could you go further into this? #432 seems like a pretty faar strech and is something different.
To get #432, proposals like #3919 seem more promising and less of a testing nightmare to me. (remember that maintainers should test every combination of relevant settings)
| * @returns {boolean} True if is an important beat else false | ||
| */ | ||
| static isImportantBeat(isFirstBeat, previousBeatStatus, currentBeatStatus) { | ||
| isImportantBeat(isFirstBeat, previousBeat, currentBeat) { |
There was a problem hiding this comment.
please refactor the function to remain static.
If it is static, this is better testable in the future than currently as static methods don't provide interiour mutability of the class.
Adding a parameter for this is fine.
| <div class="my-3 form-check"> | ||
| <input id="monitor-change" v-model="monitor.watchChanges" class="form-check-input" type="checkbox"> | ||
| <label class="form-check-label" for="monitor-change"> | ||
| <!-- todo use the $t thing here --> |
There was a problem hiding this comment.
$t is a shorthand for a localised string.
It grabs the translation key provided in the $t("KEY") and looks it up here: https://github.com/louislam/uptime-kuma/blob/master/src/lang/en.json
Docs: https://vue-i18n.intlify.dev/
what do I need to change to utilize that instead of hard-coding the descriptions
| <!-- todo use the $t thing here --> | |
| {{ $t("Monitor Changes") }} |
And then add the translation key Monitor Changes here https://github.com/louislam/uptime-kuma/blob/master/src/lang/en.json
the second point is nessesary as otherwise our awsome translators can't translate this.
My initial need was to monitor a TXT DNS record and verify when it's changed, related to ACME SSL certification validation. If it was fully implemented, I could see myself adding several more DNS monitors for A records so I'm notified when a DNS entry changes - though I don't have any explicit need for it now.
Yea, I totally get that - and a change that affects everything like this adds ton of testing. I was able to build a docker image locally for my needs, and I'm happy to continue using a dedicated custom-built instance for just the DNS monitors I need. I can't imagine this will be a widely used feature, and #3919 would also solve my TXT problem long-term with a far more robust solution. (It would require updating the content check with every change, but that's okay for such an edge case IMO). Your other comments make sense, but I'll hold off implementing unless you explicitly say you're interested in the feature @CommanderStorm. I wanted to put up a PR because it is a functioning thing, but I won't be upset if this it's denied. Thank you for both an amazing product and making it so easily extensible!! I hacked in my use case within just a couple hours 😄 |
|
Thanks for your work.
I am going to close this PR, as I think #3919 is a better (="fells less messy") solution. While I can imagine some other usecases for this exact change, those are mostly abusing our system and pusing it farther than what it was designed for (e.g. "You have 60% disk space avalible"). Those usecases can be better dealt with either alerts or metrics. |
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
Related to #432 (#432 is a full content check, but this atleast alerts on changes)
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots (if any)
Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.
UI option for watching monitor messages:
Discord notifications including message field, and consecutive up messages representing the changed message.