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

SevenIO Notification Provider #4219

Merged
merged 10 commits into from
Apr 27, 2024
Merged

SevenIO Notification Provider #4219

merged 10 commits into from
Apr 27, 2024

Conversation

scolastico
Copy link
Contributor

@scolastico scolastico commented Dec 12, 2023

  • I have read and understand the pull request rules.

Description

Added SevenIO notification provider
Resolves #3976

Type of change

  • User interface (UI)
  • 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)

@scolastico
Copy link
Contributor Author

Reminder to myself: after this is closed, remove codespace: https://github.com/codespaces/

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.

In general, this is a really well written notification provider 👍🏻

I have left a few things I noticed.
Could you add screenshots of the provider working using the following skip this part of testing (and cost as sms likely costs) for us:

<template>
<div class="mb-3">
<label for="sevenio-api-key" class="form-label">{{ $t("apiKeySevenIO") }}</label>
<input id="sevenio-api-key" v-model="$parent.notification.sevenioApiKey" type="text" class="form-control" required autocomplete="false">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use HiddenInput for sensitive fields instead ^^

This comment was marked as outdated.

src/components/notifications/SevenIO.vue Outdated Show resolved Hide resolved
src/components/notifications/SevenIO.vue Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
@louislam louislam added the question Further information is requested label Dec 12, 2023
@scolastico
Copy link
Contributor Author

Could you add screenshots of the provider working using the following skip this part of testing (and cost as sms likely costs) for us:

Allready tested it before opening pr:
image
(this is an screenshot of the debuger from SevenIO)

server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
src/lang/en.json Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm added the area:notifications Everything related to notifications label Dec 15, 2023
@CommanderStorm CommanderStorm removed the question Further information is requested label Dec 26, 2023
@louislam louislam added this to the 2.1.0 milestone Dec 28, 2023
@CommanderStorm CommanderStorm modified the milestones: 2.1.0, 2.0.0 Apr 19, 2024
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.

Thanks for the new notification provider!

All changes in this PR are as discussed in the contribution guide
=> moving this from v2.1 to v2.0

Merging despite #4219 (comment) being open by resolving the remaining issues myself

server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
server/notification-providers/sevenio.js Outdated Show resolved Hide resolved
<template>
<div class="mb-3">
<label for="sevenio-api-key" class="form-label">{{ $t("apiKeySevenIO") }}</label>
<input id="sevenio-api-key" v-model="$parent.notification.sevenioApiKey" type="text" class="form-control" required autocomplete="false">

This comment was marked as outdated.

src/components/notifications/SevenIO.vue Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm merged commit 19e8c75 into louislam:master Apr 27, 2024
17 checks passed
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.

Add notification provider seven.io
3 participants