topaLE
/
uptime-kuma
1
0
Fork 0
Code Issues 185 Pull Requests 26 Projects Releases 28 Wiki Activity
Labels Milestones

Implement template engine for message [On Hold for #760] #751

Open
deefdragon wants to merge 6 commits from deefdragon/Template-Engine into master
Conversation 7 Commits 6 Files Changed 7
deefdragon commented 4 years ago (Migrated from github.com)
Owner

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

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](https://user-images.githubusercontent.com/3053654/138051189-51b47bd9-d1cb-4662-aa41-bc38b3946737.png)
👍 2 ❤️ 2
louislam (Migrated from github.com) reviewed 4 years ago
server/notification.js
louislam (Migrated from github.com) commented 4 years ago
Owner

The method signature should not be changed.

As I said in: https://github.com/louislam/uptime-kuma/issues/646#issuecomment-939725564

This is used for general message.

Notification.send(notification, "Hello👋");
The method signature should not be changed. As I said in: https://github.com/louislam/uptime-kuma/issues/646#issuecomment-939725564 This is used for general message. ``` Notification.send(notification, "Hello👋"); ```
louislam commented 4 years ago (Migrated from github.com)
Owner

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

Thanks, haven't checked it in detail, but remember to enable eslint autofix to fix the format issues.
deefdragon (Migrated from github.com) reviewed 4 years ago
server/notification.js
deefdragon (Migrated from github.com) commented 4 years ago
Poster
Owner

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.

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.
deefdragon commented 4 years ago (Migrated from github.com)
Poster
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 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 (Migrated from github.com) reviewed 4 years ago
server/notification.js
louislam (Migrated from github.com) commented 4 years ago
Owner

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

Planning for more notification such as domain renewal, cert renewal remind.
louislam commented 4 years ago (Migrated from github.com)
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.
> 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.
chakflying (Migrated from github.com) reviewed 4 years ago
server/notification.js
chakflying (Migrated from github.com) commented 4 years ago
Owner

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?

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 commented 4 years ago (Migrated from github.com)
Owner

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

So I strongly recommend that we should think and write a complete test first before implementing this function.
deefdragon commented 4 years ago (Migrated from github.com)
Poster
Owner

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.

> 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 commented 4 years ago (Migrated from github.com)
Poster
Owner

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?

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 commented 4 years ago (Migrated from github.com)
Poster
Owner

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

Putting on hold for implementation of the notification tests: #760
Saibamen (Migrated from github.com) reviewed 4 years ago
server/notification.js
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
            notificationIDList: {
                "1": true,
                "5": true,
            },
```suggestion notificationIDList: { "1": true, "5": true, }, ```
server/notification.js
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
            tags: [{
                "id": 21,
                "monitor_id": 16,
                "tag_id": 2,
                "value": "",
                "name": "Internal",
                "color": "#059669",
            }],
```suggestion tags: [{ "id": 21, "monitor_id": 16, "tag_id": 2, "value": "", "name": "Internal", "color": "#059669", }], ```
server/notification.js
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
    static getTemplateFromNotification(notification) {
```suggestion static getTemplateFromNotification(notification) { ```
src/components/NotificationDialog.vue
Saibamen (Migrated from github.com) commented 4 years ago
Owner
```suggestion ```
src/components/NotificationDialog.vue
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
            detailLevels: NotificationDetailList,
```suggestion detailLevels: NotificationDetailList, ```
src/components/NotificationDialog.vue
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
                this.notification.template = "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}";
```suggestion this.notification.template = "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}"; ```
src/languages/en.js
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
    "Message Template": "Message Template",
    "Default Template": "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}",
    HealthyStatus: "✅ Healthy",
    UnhealthyStatus: "❌ Unhealthy",
```suggestion "Message Template": "Message Template", "Default Template": "[{{monitor.name}}] [{{monitor.health}}] {{monitor.msg}}", HealthyStatus: "✅ Healthy", UnhealthyStatus: "❌ Unhealthy", ```
src/languages/en.js
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
    // template levels
```suggestion // template levels ```
src/languages/en.js
Saibamen (Migrated from github.com) commented 4 years ago
Owner 
    "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",
```suggestion "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", ```
This pull request has changes conflicting with the target branch.
package.json
server/notification.js
src/components/notifications/index.js
src/languages/en.js
package-lock.json
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Labels
Apply labels
Clear labels
bug
Something isn't working dependencies
Pull requests that update a dependency file discussion doc
Improvements or additions to documentation duplicate
This issue or pull request already exists feature-request
New feature or request good first issue
Good for newcomers hacktoberfest hacktoberfest-accepted help help wanted
Extra attention is needed High
High Priority impossible invalid
This doesn't seem right investigating k8s Low
Low Priority Medium
Medium Priority News prerelease bug question
Further information is requested resolved Unknown wontfix
This will not be worked on
No Label bug dependencies discussion doc duplicate feature-request good first issue hacktoberfest hacktoberfest-accepted help help wanted High impossible invalid investigating k8s Low Medium News prerelease bug question resolved Unknown wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'deefdragon/Template-Engine'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes