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 template engine for message [On Hold for #760] #751

Closed
wants to merge 6 commits into from

Conversation

deefdragon
Copy link
Contributor

Template engine implementation for #646.

The current method of implementation only shows the template options when it has been specifically enabled for that notification type. This is to allow testing of templates for each notification method individually.

For all existing notifications, because the option is empty, it uses the "low detail" template, which is the current message received from most notifications. This is also the default value in the drop-down for newly created templates.

I realized while writing this that it might be best to pass the templates in via the front-end instead of just their name as that would allow them to easily be added to the translation files, and make it easier to modify the templates available when based on the notification (adding specific templates for discord detail levels for example). The current templates are also only starting points, and I feel not the best matches to their namesakes, so I welcome input there.

The testing of a notification now passes in a test heartbeat and monitor, but it is not perfect. They should ideally loop through different values so that the user can test complex templates easily.

The generation of the template argument really should be pulled out to it's own function. Specifically to have the most common keys such as name and health to not require monitor.name and monitor.health etc. as well as to add data for the names used in #627 for compatibility. This would also move the "healthy/unhealthy" to a set location.

What the UI looks like adding the template, Added before the notification specific options.
TemplateForm

@@ -77,14 +168,54 @@ class Notification {
* @returns {Promise<string>} Successful msg
* Throw Error with fail msg
*/
static async send(notification, msg, monitorJSON = null, heartbeatJSON = null) {
static async send(notification, msg, monitorJSON, heartbeatJSON) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method signature should not be changed.

As I said in: #646 (comment)

This is used for general message.

Notification.send(notification, "Hello👋");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been wondering about that. There were only two calls to send, and now the test call has example monitors and heartbeats. As such, there are no calls for a general message. Can you expand on what you plan on using that for beyond the test message?

I wonder especially given the number of special cases that pop up around the more complicated notifiers, if it would not be better to split out those calls explicitly instead of having one function with logic for both cases.

Notification.notify(notification,monitor,heartbeat) for notification data
Notification.message(notification,message) for simple messages

It would make it a larger change, but given I am already going to have to go through basically every notification type to verify/test the templates and notifications anyway, I don't think that it would be all that much more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Planning for more notification such as domain renewal, cert renewal remind.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO if the function name is send, then it should really be as accommodating as possible with respect to what can be sent, so yeah it would be a problem if in the future there are other things to send. Keep extending send() with more parameters doesn't seem like a good practice anyway. I haven't really thought about this, but is it possible that send is just send(notification, httpBody), then we can have a method for each type of notification which wraps the send() call, maybe something like sendDown() sendUp() sendCertExp() etc?

@louislam
Copy link
Owner

Thanks, haven't checked it in detail, but remember to enable eslint autofix to fix the format issues.

@deefdragon
Copy link
Contributor Author

You mentioned that it would be impossible to write tests of the different notifications. Could you expand on why?
Are you referring to just e2e tests? I believe it will be easy enough to write unit tests for the notifications.

@louislam
Copy link
Owner

You mentioned that it would be impossible to write tests of the different notifications. Could you expand on why? Are you referring to just e2e tests? I believe it will be easy enough to write unit tests for the notifications.

  • You cannot verify them in message platforms automatically.
  • Some of them are paid services such as SMS
  • Some of them always returns successful message even if it is using a invalid api key.
  • Some of them are having word limit.

@louislam
Copy link
Owner

So I strongly recommend that we should think and write a complete test first before implementing this function.

@deefdragon
Copy link
Contributor Author

So I strongly recommend that we should think and write a complete test first before implementing this function.

That is completely fair. I am working on writing some tests for the SMTP notification. I think that I am close to an example. If you think it works, Ill start on the other notifications. Ill throw another PR out with it shortly for examination.

@deefdragon
Copy link
Contributor Author

While I am looking at it, question: what do we want the response to be in the case of a template build/parse error?
Just return that template error?
Attempt to build the default message, and then send it to the notification, returning that "error" (or lack of)?
Attempt to build&send default, then combine the errors if one shows up?

@deefdragon deefdragon changed the title Implement template engine for message Implement template engine for message [On Hold for #760] Oct 21, 2021
@deefdragon
Copy link
Contributor Author

Putting on hold for implementation of the notification tests: #760

Comment on lines +113 to +114
notificationIDList: { "1": true,
"5": true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
notificationIDList: { "1": true,
"5": true },
notificationIDList: {
"1": true,
"5": true,
},

Comment on lines +115 to +120
tags: [{ "id": 21,
"monitor_id": 16,
"tag_id": 2,
"value": "",
"name": "Internal",
"color": "#059669" }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags: [{ "id": 21,
"monitor_id": 16,
"tag_id": 2,
"value": "",
"name": "Internal",
"color": "#059669" }],
tags: [{
"id": 21,
"monitor_id": 16,
"tag_id": 2,
"value": "",
"name": "Internal",
"color": "#059669",
}],

Comment on lines +204 to +205
static getTemplateFromNotification(notification) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static getTemplateFromNotification(notification) {
static getTemplateFromNotification(notification) {

Comment on lines +114 to +115
detailLevels: NotificationDetailList,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
detailLevels: NotificationDetailList,
detailLevels: NotificationDetailList,

@@ -72,7 +89,8 @@ import { Modal } from "bootstrap";
import { ucfirst } from "../util.ts";

import Confirm from "./Confirm.vue";
import NotificationFormList from "./notifications";
import { NotificationFormList, NotificationDetailList, TemplateEnabledList } from "./notifications";
// import from "./notifications";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// import from "./notifications";

Comment on lines +179 to +180
this.notification.template = "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.notification.template = "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}";
this.notification.template = "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}";

"Default Template":"[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}",
HealthyStatus:"✅ Healthy",
UnhealthyStatus:"❌ Unhealthy",
//template levels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//template levels
// template levels

Comment on lines +309 to +312
"Message Template":"Message Template",
"Default Template":"[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}",
HealthyStatus:"✅ Healthy",
UnhealthyStatus:"❌ Unhealthy",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Message Template":"Message Template",
"Default Template":"[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}",
HealthyStatus:"✅ Healthy",
UnhealthyStatus:"❌ Unhealthy",
"Message Template": "Message Template",
"Default Template": "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}",
HealthyStatus: "✅ Healthy",
UnhealthyStatus: "❌ Unhealthy",

Comment on lines +314 to +320
"Notification Message Detail":"Notification Message Detail",

"Minimal Detail":"Minimal Detail",
"Low Detail":"Low Detail",
"Medium Detail":"Medium Detail",
"Full Detail":"Full Detail",
"Custom Template":"Custom Template",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Notification Message Detail":"Notification Message Detail",
"Minimal Detail":"Minimal Detail",
"Low Detail":"Low Detail",
"Medium Detail":"Medium Detail",
"Full Detail":"Full Detail",
"Custom Template":"Custom Template",
"Notification Message Detail": "Notification Message Detail",
"Minimal Detail": "Minimal Detail",
"Low Detail": "Low Detail",
"Medium Detail": "Medium Detail",
"Full Detail": "Full Detail",
"Custom Template": "Custom Template",

@louislam louislam mentioned this pull request Oct 22, 2021
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

We are clearing up our old Pull Requests and yours has been open for 3 months with no activity. Remove stale label or comment or this will be closed in 2 days.

@github-actions github-actions bot added the Stale label Dec 5, 2022
@louislam louislam removed the Stale label Dec 5, 2022
@deefdragon deefdragon closed this Dec 5, 2022
@GalacticAust
Copy link

Hi All

I know this is a old post but did we ever get to edit the body of the email? from the dashboard?

If not how do we edit the layout of the email sent

Thanks all

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

Successfully merging this pull request may close these issues.

5 participants