-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
feat: Better WeCom notification #3491
Conversation
// fallback address | ||
if (!clientUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use if-else insead
} | ||
break; | ||
default: | ||
address = monitorJSON["url"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"url"
is not a guaranteed parameter.
=> Please handle cases where no monitor exist like the docker monitor.
See #3327 for a bug report resulting from this handling
let clientUrl = ""; | ||
|
||
//#region computed address and clientUrl | ||
switch (monitorJSON["type"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please extract this switch to another method for readability
await axios.post( | ||
WeComUrl, | ||
{ | ||
msgtype: "text", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing it like this means that for testing, users will receive a message which is very differently formatted from what they will receive when they get an actual alert
=> Not sure if this is intentional
card_type: "text_notice", | ||
main_title: { | ||
title: monitorJSON["name"], | ||
desc: monitorJSON["type"] === "push" ? "Heartbeat" : address, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What?
In case of a push-monitor, this is set as "Heartbeat"
?
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Sorry, I will organize the code and open a new PR |
No need to open a new PR, you can just mark this PR as a draft and comment once done ^^ |
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
Fixes #3145
Type of change
Please delete any options that are not relevant.
Checklist
(including JSDoc for methods)
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.
see #3145