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

Merged

Conversation

cfichtmueller
Copy link
Contributor

@cfichtmueller cfichtmueller commented Feb 12, 2024

⚠️⚠️⚠️ 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

I'd like to add a new notification provider for gtxmessaging. They provide an SMS gateway, so this is about SMS notifications.

@louislam please let me know if this is a feature you would like to have in the project. Cheers.

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 generates no new warnings
  • [ ] My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Setup

gtx-setup

Up / Down / Certificate Expired / Test Notification

gtx-screenshot

Signed-off-by: Christoph Fichtmüller <cf@cfichtmueller.com>
@CommanderStorm
Copy link
Collaborator

Notification providers do not require a discussion.
I don't know if we should classify gtxmessaging as a regional notification provider, given that their website is DE/EN or global given that they offer sending to a long country list.

When adding a notification provider, these things are common in the review:

  • add all translation keys to en.json; otherwise they won't be able to be translated
  • Please attach screenshots of the 3 types of events DOWN/UP, Certificate Expiry1 and Testing
  • If a field is sensitive, it should use the HiddenInput instead
  • If a field is difficult to find/ an average user does not know what to put there, adding an help text is something that helps us and the users

Footnotes

  1. See https://badssl.com/ for broken certs

@CommanderStorm CommanderStorm added the area:notifications Everything related to notifications label Feb 12, 2024
@cfichtmueller
Copy link
Contributor Author

@CommanderStorm thank you for your reply. The PR manual explicitly states that a discussion is needed beforehand, hence my approach. May I suggest to add a small note here? Unless of course I'm the only one who misunderstood 😄

I'll go ahead and implement the provider. I'll create it as a global provider since they offer english API documentation and operate internationally.

@cfichtmueller cfichtmueller marked this pull request as ready for review February 15, 2024 07:21
server/notification-providers/gtx-messaging.js Outdated Show resolved Hide resolved
server/notification-providers/gtx-messaging.js Outdated Show resolved Hide resolved
server/notification-providers/gtx-messaging.js Outdated Show resolved Hide resolved
server/notification-providers/gtx-messaging.js Outdated Show resolved Hide resolved
server/notification-providers/gtx-messaging.js Outdated Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Outdated Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Outdated Show resolved Hide resolved
src/components/notifications/GtxMessaging.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.

I am happy with the current code.

I have changed some helptexts to be more explicit in what is being asked (f.ex. the part about the TPOA) and have added the required links.

src/components/notifications/GtxMessaging.vue Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Outdated Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Outdated Show resolved Hide resolved
src/components/notifications/GtxMessaging.vue Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Collaborator

Thanks for the new notification provider!

All changes in this PR are uncontroversial
⇒ merging with junior maintainer approval

@CommanderStorm CommanderStorm merged commit 49b6dac into louislam:master Mar 26, 2024
17 checks passed
@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Apr 7, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants