Skip to content

Add Heii On-Call Notification Provider#4485

Merged
CommanderStorm merged 19 commits intolouislam:masterfrom
hevans66:hevans/add-heii-on-call-notification-provider
Mar 11, 2024
Merged

Add Heii On-Call Notification Provider#4485
CommanderStorm merged 19 commits intolouislam:masterfrom
hevans66:hevans/add-heii-on-call-notification-provider

Conversation

@hevans66
Copy link
Copy Markdown
Contributor

@hevans66 hevans66 commented Feb 13, 2024 •
edited by CommanderStorm
Loading

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Add notification provider for Heii On-Call.

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 Notification

image

Events

Event Before After
UP -
DOWN -
Certificate-expiry -
Testing -

@hevans66 hevans66 changed the title Hevans/add heii on call notification provider Add Heii On-Call Notification Provider Feb 13, 2024
@hevans66 hevans66 marked this pull request as ready for review February 13, 2024 02:16
@chakflying chakflying added the A:notifications Issues or PRs related to notifications label Feb 13, 2024
CommanderStorm

This comment was marked as resolved.

Sign in to view

@hevans66
Copy link
Copy Markdown
Contributor Author

hevans66 commented Feb 14, 2024

@CommanderStorm Thanks for the review. I believe I have addressed all your concerns.

CommanderStorm
CommanderStorm reviewed Feb 14, 2024
View reviewed changes
Comment thread server/notification-providers/heii-oncall.js
CommanderStorm
CommanderStorm reviewed Feb 14, 2024
View reviewed changes
Comment thread src/components/notifications/HeiiOnCall.vue Outdated
Comment thread src/lang/en.json Outdated
CommanderStorm

This comment was marked as resolved.

Sign in to view

CommanderStorm
CommanderStorm approved these changes Feb 14, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Okay, I changed the formatting of the code a bit to be closer to the other notification providers.
Could you review the changes I added in cc00011?

Other than that, I am happy with the current code.

I am unsure if we should change the translations in this context as suggested in #4485 (review) => I am going to leave this up to you to either accept/resolve

@hevans66
Copy link
Copy Markdown
Contributor Author

hevans66 commented Feb 16, 2024

@CommanderStorm awesome! Accepted your changes. Thanks so much for the review.

@hevans66
Update en.json
daa59b8 
Fix misspelled "Trigger"
@hevans66
Copy link
Copy Markdown
Contributor Author

hevans66 commented Mar 4, 2024

@CommanderStorm let me know if there is anything else I can do to help this get merged. I'd like to work on a guide for setting up Uptime Kuma with Heii On-Call, and would like to have this in before its published.

@CommanderStorm
Copy link
Copy Markdown
Collaborator

CommanderStorm commented Mar 11, 2024

Actually there is:
I missed in the previous review that this is missing the screenshot from the Testing state (the "Test"-button in above screenshot)

Other than that I would be happy to merge this into the upcoming v2.0 release.

@hevans66
Copy link
Copy Markdown
Contributor Author

hevans66 commented Mar 11, 2024

Testing screenshot has been added.

@CommanderStorm
Copy link
Copy Markdown
Collaborator

CommanderStorm commented Mar 11, 2024

Thanks for the notification provider!
I think adding it is uncontroversial as it matches the code style and the appropriate tests have been performed.
⇒ merging with junior maintainer approval.

@CommanderStorm CommanderStorm merged commit bfd65ab into louislam:master Mar 11, 2024
@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

Reviewers

@CommanderStorm CommanderStorm CommanderStorm approved these changes

Assignees

No one assigned

Labels

A:notifications Issues or PRs related to notifications

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@hevans66 @CommanderStorm @chakflying