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

Implement Templates for Notifications #646

Open
deefdragon opened this issue Oct 11, 2021 · 19 comments
Open

Implement Templates for Notifications #646

deefdragon opened this issue Oct 11, 2021 · 19 comments
Labels
area:notifications Everything related to notifications feature-request Request for new features to be added type:enhance-existing feature wants to enhance existing monitor

Comments

@deefdragon
Copy link
Contributor

deefdragon commented Oct 11, 2021

Is it a duplicate question?

This is a potential solution to a number of other issues, consolidating them into one, and has been discussed briefly in those other issues.

Requests to have different information in the notification

Potential long term bonuses/considerations

Having templates built up would allow for custom reports to be written like requested in #366 (requires a reports task system to be built up)
Would allow for inserting custom data into notifications like requested in #202 (would require saving of custom information for monitors/tags/groups/global etc.)

Is your feature request related to a problem? Please describe.

The messages sent in notifications have several different components which many have requested the ability to modify parts of or all of, and each of which requires translation.

Describe the solution you'd like

The implementation of a template engine such as liquidjs to allow users to customize the message sent as they see fit if they wish to do so. This would also condense the messages into as few formats as possible for translation.

Some implementations such as Discord require an object to use the API. In this case, the template would be used to generate a JSON object that would then be parsed and used as the required object.

Describe alternatives you've considered

#627 currently proposes adding regex-bassed key replacement in the email subject. It would also possible to leave the components individual and just customize/translate each one, but that would likely be a messy, hard to update solution.

Additional context

I can see 4 major steps to this.

  1. Create the system to lookup, parse and return the templates
  2. Alter notifications to use common, simple templates in their message
  3. Alter front end & db to allow using custom templates
  4. Implement global template setting

2 and 3 would have to done for the simple notifications, as well as for the JSON object implementations. 1 would basically be re-usable, however a fallback will be required in the case that the data returned from the template is not valid JSON.

I am willing to work on this, however I would like some input & other eyes before I do so. I want to make sure I am not going in the completely wrong direction. This is also a large enough change that, as has been mentioned, I want the sign off of @louislam before I go any further.

@deefdragon deefdragon added the feature-request Request for new features to be added label Oct 11, 2021
@louislam
Copy link
Owner

louislam commented Oct 11, 2021

It would be really difficult in my opinion. I would suggest that it should be implemented once we have a whole picture of it.

I have a similar thoughts before, but I can't think of a good approach, because there are a lot of formats here, for example:

  • Line, Discord, Slack, Rocket.chat and more: All different JSON formats / payloads, also it is conditional (e.g. up = green, down = red).
  • Email: subject and body
  1. Implement global template setting

With this situation, how could global template be implemented?

  1. Alter front end

For notification like Discord, how is it look like in frontend? Textarea with json?

Current implementation

For your information, currently, there are two usages of Notification.send(...);

static async send(notification, msg, monitorJSON = null, heartbeatJSON = null) {

  1. General messages
Notification.send(notification, "Hello👋");
  1. Sending important up/down notifications
Notification.send(notification, "[Test][🔴 Down] 404 Not Found", monitor, heartbeat);

@bakeDong1
Copy link

how about global variable like statping

@deefdragon
Copy link
Contributor Author

I would suggest that it should be implemented once we have a whole picture of it.

By this, do you mean we need to the proof of concept to then iterate on? If so I will get started on it.

... there are a lot of formats here, for example:

  • Line, Discord, Slack, Rocket.chat and more: All different JSON formats / payloads, also it is conditional (e.g. up = green, down = red).
  • Email: subject and body

One of the things that came to mind would be to have different detail levels of templates. (Minimal, small, medium, large, full etc.) The defaults for emails could then have minimal in the subject and large or full in the body, Appraise::Twitter could use the small template, etc.. When Selecting what is displayed in the message, you can then choose "custom" which exposes a textarea where you can input a custom template.

As for the the conditional status, we could either make up/down templates for each, or, because its a template, we can include that in the template itself. Something to be examined in the Proof of Concept.

{% if monitor.status == "up" %}
  {{ monitor.name }} is now up.
{% else %}
  {{ monitor.name }} is now down. 
{% endif %}
  1. Implement global template setting

With this situation, how could global template be implemented?

This was a combination of scope creep & crossed wires on my part. I was thinking about how many monitors some people had, not notifications, and reconsidering it with notifications, it would be a VERY rare situation where someone has that many notifications requiring the same template. A large number of notifications, IE on a language by language basis is possible. But not requiring the same template for many notifications.

  1. Alter front end

For notification like Discord, how is it look like in frontend? Textarea with json?

Expanding what I have here. I see adding a "simple/advanced" toggle to the JSON endpoints.

  • "Simple" outputs something based on the templates explained earlier, which is then sent as a raw message.
  • "Advanced" mode would be able to take advantage of the features allowable in the different APIs.

Selecting "custom" for the advanced mode would then have the user generate the JSON parsed into an object which is then sent to the service.

The default advanced template would have to be added for each of the different services for translation, but I don't think that would be much more complicated than the current system, and it would centralize them.

Current implementation

[...]

I am going to have to dig into how the different .sends are called. I might start with Templates only applying for the case where monitor & heartbeat are not null.

@louislam
Copy link
Owner

So it is really a large structural change.

One very important thing that I forgot to mention is, there is no auto tests of notifications. Also I believe that it is almost impossible to implement those tests.

All of them had already been tested well by hand. I am afraid that touching them would have some unpredictable situations that make notifications unstable as a result.

Recently, I actually rejected a big pull request, because there is no way for me to verify the change completely. (#568)

I prefer not to implement this, just because of the testing problem.

Instead, at this moment, I would prefer the approach like #627. Each pull request just modify one notification. It is easy for me to check one by one.

But I agree that we should use the parser like liquidjs instead of regex.

@deefdragon
Copy link
Contributor Author

I can think of a potential solution to the testing problem, but I understand the not wanting to take on such a large change until that is implemented.

Would you be willing to consider it if I limited it in scope to not include the complex JSON cases for now? The changes would be limited to around here

let msg = `[${this.name}] [${text}] ${bean.msg}`;

(I was not aware how literal you were being when you said that there were 2 calls to Notification.send. I thought you meant two types of calls)

@PopcornPanda
Copy link
Contributor

PopcornPanda commented Oct 12, 2021

I think, there is no need for changing notification system itself. It could be basically a template engine that generate body from arguments.
Something like this:

msg = template(templateName,monitorJSON,heartbeatJSON,someExtraArgs...)

Where templateName is name of predefined template which we could create in dashboard. Maybe there should also be a way to pass template as a string like this:

msg = template("[{{NAME}}] {{$t(has changed status to)}} {{STATUS}}",monitorJSON,heartbeatJSON,someExtraArgs...)

For multilanguage support, text in $t() could be translated like in vue files.

If it only generates a text, then it should be safe for existing notifications (because it doesn't touch them) and simple to test. Just change ready msg to result of template engine in one notification provider at a time and check provider itself.

I'm not a nodejs dev, and I don't know that this has any sense in this language. In ruby ERB just handles generating content from templates and You just use result where You need it.

@deefdragon
Copy link
Contributor Author

So, I have started on this, and gotten a quick and dirty example to work.
While parsing the template in place of msg on line 328 can work, I am once again wondering if it would be better to put the template parsing inside Notification.send, and not 328.

I wonder this because I think that when a test notification is sent, it could include some mock objects for the monitor and heartbeat. Combined with parsing the template in Notification.send, the user will be able to not only test connection to the notification provider etc, but also be able to see what the notification will look like when received.

@Hamitamaru
Copy link

Was just about to cut a separate issue for the Slack particular notification improvement I would like to see but I found this one.

If I run this:

curl -X POST -H 'Content-type: application/json' --data '{"text": "[✅ Up][plex]","username": "uptime-kuma","blocks": [{"type": "header","text": {"type": "plain_text","text": "Uptime Kuma Alert 2",},},{"type": "section","fields": [{"type": "mrkdwn","text": "*Message*\n la2"},{"type": "mrkdwn","text": "*Time (UTC)*\n la"}]}]}' https://hooks.slack.com/services/replaceThisLinkWithYourSlackAppIncomingWebHookUrl

I get a more useful notification on my android phone that shows [✅ Up][plex] rather than less useful Uptime Kuma Alert so I don't need to open slack to understand what has changed (what went up or down).

Not sure what template variables we are adding support for, so far I've seen
[{{NAME}}] I guess this is the name of the monitor stored in monitorJSON["name"]
[{{STATUS}}] I guess this is the value in heartbeatJSON["status"]

Can we create template variables scheme that allow for arbitrary key access in both monitorJSON and heartbeatJSON e.g.
[{{monitor-name}}] results in monitorJSON["name"]
[{{heartbeat-status}}] results in heartbeatJSON["status"]

Aside from that arbitrary template variable scheme for access to those JSON variables if we can also find a way to accomplish:
[✅ Up] and the down equivalent via some template variable that would help my case.

@deefdragon
Copy link
Contributor Author

@Hamitamaru unfortunately, the more complex notification types like slack/discord are going to take a bit to implement. The notification message can be changed using a template without much worry as it's just text. Discord/Slack etc.'s richer features on the other hand will each require wiring up and testing individually to account for their different formats.

The templates should have access to the entire monitor and heartbeat objects (minus some potential sanitization). I also suspect there will be a number of helpers included such as ✅ Up, But time will tell on the full details.

@deefdragon
Copy link
Contributor Author

Question on translation: Do we have the a way to translate things server side? I'm not seeing any previous uses, and part of why I started this was so that the default templates could be translated.

@tiakun
Copy link

tiakun commented Mar 25, 2022

Would it be easier if this is implemented not globally but per notification type? It would also allow the customization per notification type, e.g. some users might want short message for SMS but longer messages for chat/message apps.

@deefdragon
Copy link
Contributor Author

It would be easier, but it would make the notifications aspect of uptime kuma even more of a dis-congruous mess.

The ideal would be to have a consistent notification form/message for everything, and while it's ok now, it could be much better.

The thing is, there are other aspects of UK that should get worked on first (groups, notification controls/cascading etc). Louis only has so much time to test, and addressing the notifications system is rather low on the list.

I'm going to close this issue for now, as it is very likely not going to get addressed until a V2/notifications core code re-write.

@Qumans

This comment was marked as off-topic.

@Qumans

This comment was marked as off-topic.

@CommanderStorm

This comment was marked as off-topic.

@borrelan
Copy link

borrelan commented Aug 8, 2024

Hello,

I would like to thank the team behind Uptime Kuma for such an awesome service that I have come to depend on. As this issue is ongoing, would you be willing to consider change the static entry of "[Uptime Kuma]" in the ntfy notification to include the "Friendly Name"? When you have more than one instance pushing to the same topic it makes it difficult to determine which instance is pushing the notification. One could also argue what's the point of the setting "Friendly Name" if it's not used?

Thanks again!!!

@realies
Copy link

realies commented Aug 30, 2024

Screenshot 2024-08-30 at 02 52 16

i see a lot of text duplication when i add a slack webhook notification

the notification friendly name is Uptime Kuma and the webhook name is Uptime Kuma

is there a way to remove the friendly name from the notification text? or even the whole "Uptime Kuma Alert" row altogether?

@CommanderStorm
Copy link
Collaborator

Removing that line is likely fine, feel free to provide a PR.
Given the number of notifications providers and maintainers I can't afford timewise to fix and test this myself
=> we are quite heavily reliant on contributors in this department, see our contribution guide ^^

PS: for the future, consider opening on new issues. For specific issues, which are not closely related to their parents, this otherwise spams too many people.

If you are instead looking for message templating, that code is in master but needs to be lifted from the SMTP/webhook implementations into one that can be shared between all monitors.
This would allow other monitors to make fields properly templated.
PRs welcome..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:notifications Everything related to notifications feature-request Request for new features to be added type:enhance-existing feature wants to enhance existing monitor
Projects
None yet
Development

No branches or pull requests