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.

Sign up for GitHub

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 gtxmessaging Notification Support #4481

Merged
merged 9 commits into from
Mar 26, 2024
Merged

Add gtxmessaging Notification Support #4481

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0e7ef54
Add gtxmessaging Notification Support
cfichtmueller Feb 12, 2024
ac109d0
Add gtx messaging Notification Provider
cfichtmueller Feb 15, 2024
a028373
Merge branch 'master' into feat/gtx-notification-provider
cfichtmueller Feb 15, 2024
a5742d8
apply pull request suggestions
cfichtmueller Feb 15, 2024
b29cf1e
Apply suggestions from code review
cfichtmueller Feb 15, 2024
24e2491
code cleanup
cfichtmueller Feb 15, 2024
7696e7e
remove "more info" section from setup dialog
cfichtmueller Feb 15, 2024
7ce1cd7
improved the helptexts a bit
CommanderStorm Feb 15, 2024
3a2c001
Merge branch 'master' into feat/gtx-notification-provider
CommanderStorm Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions server/notification-providers/gtx-messaging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
const NotificationProvider = require("./notification-provider");
const axios = require("axios");

class GtxMessaging extends NotificationProvider {
name = "gtxmessaging";

/**
* @inheritDoc
*/
async send(notification, msg, monitorJSON = null, heartbeatJSON = null) {
const okMsg = "Sent Successfully.";

let authKey = notification.gtxMessagingApiKey;
let from = notification.gtxMessagingFrom.trim();
let to = notification.gtxMessagingTo.trim();
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
let text = msg.replaceAll("🔴 ", "").replaceAll("✅ ", "");
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved

try {

cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
let data = new URLSearchParams();
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
data.append("from", from);
data.append("to", to);
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
data.append("text", text);

let url = `https://rest.gtx-messaging.net/smsc/sendsms/${authKey}/json`;
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved

console.log(`will post url: ${url}, data:`, data);

cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
await axios.post(url, data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is returned in the message-status field?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

E.g.

{
    "message-count": "1",
    "message-status": "OK",
    "message-id": "d593f2d6-de4a-48b2-8abe-17487d7f3c30e"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a message should only be marked as successfully sent iff response["message-status"] === "OK"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I aligned my implementation with the other existing notification providers in uptime kuma. The ones I checked all discard the results from the API provider. E.g.

I appreciate your thoroughness but begin to wonder why things that seem to be totally ok in other providers are not ok in this one.

I can add a check for the message status. However the API documentation doesn't state which values can be returned for message-status. I could only return a message relaying the message-status. Without knowing the possible values we run the risk of reporting false-positives (e.g messages could be queued up resulting in status like DELAYED or QUEUED).

If you insist I will add the check. I personally see no need at this time since the API documentation doesn't describe error cases with an HTTP 200 response. I tested with invalid phone numbers. These also return HTTP 200 with message status OK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but begin to wonder why things that seem to be totally ok in other providers are not ok in this one.

This depends on the thoroughness of the code review.
The review comments you in an OSS project might not always reflect the current state of the code (while useful for the initial draft of a PR), but rather the level of software-quality the project wants to be at.
Here this came up because I looked at the API docs and saw that 200 might not indicate success as normally thought.


return okMsg;
} catch (error) {
this.throwGeneralAxiosError(error);
}
}
}

module.exports = GtxMessaging;
4 changes: 3 additions & 1 deletion server/notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const GoAlert = require("./notification-providers/goalert");
const SMSManager = require("./notification-providers/smsmanager");
const ServerChan = require("./notification-providers/serverchan");
const ZohoCliq = require("./notification-providers/zoho-cliq");
const GtxMessaging = require("./notification-providers/gtx-messaging");

class Notification {

Expand Down Expand Up @@ -124,7 +125,8 @@ class Notification {
new Webhook(),
new WeCom(),
new GoAlert(),
new ZohoCliq()
new ZohoCliq(),
new GtxMessaging(),
];
for (let item of list) {
if (! item.name) {
Expand Down
3 changes: 2 additions & 1 deletion src/components/NotificationDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ export default {
"Splunk": "Splunk",
"webhook": "Webhook",
"GoAlert": "GoAlert",
"ZohoCliq": "ZohoCliq"
"ZohoCliq": "ZohoCliq",
"gtxmessaging": "GtxMessaging"
};

// Put notifications here if it's not supported in most regions or its documentation is not in English
Expand Down
29 changes: 29 additions & 0 deletions src/components/notifications/GtxMessaging.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<template>
<div class="mb-3">
<label for="gtxmessaging-api-key" class="form-label">{{ $t("gtxMessagingApiKey") }}</label>
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
<HiddenInput id="gtxmessaging-api-key" v-model="$parent.notification.gtxMessagingApiKey" :required="true"></HiddenInput>
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
</div>
<div class="mb-3">
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
<label for="gtxmessaging-from" class="form-label">{{ $t("gtxMessagingFrom") }}</label>
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
CommanderStorm marked this conversation as resolved.
Show resolved Hide resolved
<input id="gtxmessaging-from" v-model="$parent.notification.gtxMessagingFrom" type="text" class="form-control" required>
</div>
<div class="mb-3">
<label for="gtxmessaging-to" class="form-label">{{ $t("gtxMessagingTo") }}</label>
CommanderStorm marked this conversation as resolved.
Show resolved Hide resolved
<input id="gtxmessaging-to" v-model="$parent.notification.gtxMessagingTo" type="text" pattern="^[\d-]{10,31}(@[\w\.]{1,})?$" class="form-control" required>
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
</div>
<div class="mb-3">
<i18n-t tag="p" keypath="More info on:" style="margin-top: 8px;">
<a href="https://www.gtx-messaging.com/en/solutions/sms-gateway-api/" target="_blank">https://www.gtx-messaging.com/en/solutions/sms-gateway-api/</a>
</i18n-t>
</div>
CommanderStorm marked this conversation as resolved.
Show resolved Hide resolved
</template>

<script>
import HiddenInput from "../HiddenInput.vue";

export default {
components: {
HiddenInput
}
};
</script>
4 changes: 3 additions & 1 deletion src/components/notifications/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import GoogleChat from "./GoogleChat.vue";
import Gorush from "./Gorush.vue";
import Gotify from "./Gotify.vue";
import GrafanaOncall from "./GrafanaOncall.vue";
import GtxMessaging from "./GtxMessaging.vue";
import HomeAssistant from "./HomeAssistant.vue";
import Kook from "./Kook.vue";
import Line from "./Line.vue";
Expand Down Expand Up @@ -111,7 +112,8 @@ const NotificationFormList = {
"WeCom": WeCom,
"GoAlert": GoAlert,
"ServerChan": ServerChan,
"ZohoCliq": ZohoCliq
"ZohoCliq": ZohoCliq,
"gtxmessaging": GtxMessaging,
};

export default NotificationFormList;
5 changes: 4 additions & 1 deletion src/lang/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,5 +883,8 @@
"deleteRemoteBrowserMessage": "Are you sure want to delete this Remote Browser for all monitors?",
"GrafanaOncallUrl": "Grafana Oncall URL",
"Browser Screenshot": "Browser Screenshot",
"What is a Remote Browser?": "What is a Remote Browser?"
"What is a Remote Browser?": "What is a Remote Browser?",
"gtxMessagingApiKey": "API Key",
cfichtmueller marked this conversation as resolved.
Show resolved Hide resolved
"gtxMessagingFrom": "From",
"gtxMessagingTo": "To"
}
Loading