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

Add HostedSMS.pl notification provider #3176

Closed
wants to merge 4 commits into from

Conversation

streamthing
Copy link

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
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]:

  • I have read and understand the pull request rules.

Description

This PR adds support HostedSMS.pl SMS gateway via HTTPS SimpleApi.

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

photo_2023-05-20_03-08-04

Copy link
Author

@streamthing streamthing left a comment

Choose a reason for hiding this comment

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

Fixed CRLF -> LF

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution, I have left some small inline comments, but nothing major

server/notification-providers/hostedsms.js Outdated Show resolved Hide resolved
server/notification-providers/hostedsms.js Outdated Show resolved Hide resolved
src/components/notifications/HostedSMS.vue Outdated Show resolved Hide resolved
server/notification-providers/hostedsms.js Outdated Show resolved Hide resolved
src/components/notifications/HostedSMS.vue Outdated Show resolved Hide resolved
src/components/notifications/HostedSMS.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution, I have left some small inline comments, but nothing major


name = "hostedsms";

async send(notification, msg, monitorJSON = null, heartbeatJSON = null) {
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that msg is never used.

When monitorJSON and heartbeantJSON are null, msg is used for other general messages such as testing or certificate alert.

@louislam louislam added the question Further information is requested label Jun 29, 2023
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I have added a few inline comments.
Some of them are for style by, but two of them also address the issue noted by Louis ^^

(sorry for making faulty suggestions before)

Comment on lines +23 to +28
let textMsg = "";
if (heartbeatJSON && heartbeatJSON.status === UP) {
textMsg = `✅ [${monitorJSON.name}] is back online`;
} else {
textMsg = `🔴 [${monitorJSON.name}] went down`;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let textMsg = "";
if (heartbeatJSON && heartbeatJSON.status === UP) {
textMsg = `✅ [${monitorJSON.name}] is back online`;
} else {
textMsg = `🔴 [${monitorJSON.name}] went down`;
}
let textMsg = msg;
if (heartbeatJSON && heartbeatJSON.status === UP) {
textMsg = `✅ [${monitorJSON.name}] is back online`;
} else if (heartbeatJSON && heartbeatJSON.status === DOWN) {
textMsg = `🔴 [${monitorJSON.name}] went down`;
}

Comment on lines +19 to +22
/*
Create shorter alert message to avoid extra SMS costs (1 SMS with unicode has limit to 70 characters)
https://github.com/louislam/uptime-kuma/pull/3176#discussion_r1199601497
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/*
Create shorter alert message to avoid extra SMS costs (1 SMS with unicode has limit to 70 characters)
https://github.com/louislam/uptime-kuma/pull/3176#discussion_r1199601497
*/
// Create shorter alert message to avoid extra SMS costs (1 SMS with unicode has limit to 70 characters)

@@ -0,0 +1,52 @@
const NotificationProvider = require("./notification-provider");
const axios = require("axios");
const { UP } = require("../../src/util");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const { UP } = require("../../src/util");
const { UP, DOWN } = require("../../src/util");

Comment on lines +10 to +11
let okMsg = "Sent Successfully.";

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let okMsg = "Sent Successfully.";
const okMsg = "Sent Successfully.";
const url = "https://api.hostedsms.pl/SimpleApi";

"Sender": notification.hostedsmsSenderName,
};

let resp = await axios.post("https://api.hostedsms.pl/SimpleApi", data, config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let resp = await axios.post("https://api.hostedsms.pl/SimpleApi", data, config);
let resp = await axios.post(url, data, config);

let okMsg = "Sent Successfully.";

try {
let config = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let config = {
const config = {

textMsg = `🔴 [${monitorJSON.name}] went down`;
}

let data = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let data = {
const data = {

@louislam louislam removed this from the 1.23.0 milestone Aug 2, 2023
@chakflying chakflying added area:notifications Everything related to notifications Stale labels Dec 2, 2023
@github-actions github-actions bot closed this Dec 3, 2023
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 question Further information is requested Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants