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 Heii On-Call Notification Provider #4485

Conversation

hevans66
Copy link
Contributor

@hevans66 hevans66 commented Feb 13, 2024

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 area:notifications Everything related to notifications label Feb 13, 2024
CommanderStorm

This comment was marked as resolved.

@hevans66
Copy link
Contributor Author

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

src/components/notifications/HeiiOnCall.vue Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
CommanderStorm

This comment was marked as 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.

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
Contributor Author

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

Fix misspelled "Trigger"
@hevans66
Copy link
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
Collaborator

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
Contributor Author

Testing screenshot has been added.

@CommanderStorm
Copy link
Collaborator

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
9 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.

3 participants