-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Mobivate SMS Notification plugin #6451
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a few comments, but they should be simple enough to adress.
Also CI is not quite happy it seems.
Could you in addition to the comments please provide the screenshot of an up/down/testing/certificate expiry event?
I don't have access to this notification provider and this way only you have to test that everything works and that there are no typos somewhere ^^
|
|
||
| try { | ||
| // smspartner does not support non ascii characters and only a maximum 639 characters | ||
| let cleanMsg = msg.replace(/[^\x00-\x7F]/g, "").substring(0, 639); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add an indicator ("...") if we cut off a message.
639 is rather large, but it might still happen
| <label for="mobivate-recipients" class="form-label">{{ $t("Recipients") }}</label> | ||
| <input id="mobivate-recipients" v-model="$parent.notification.mobivateRecipients" type="text" minlength="3" maxlength="20" pattern="^[\d+,]+$" class="form-control" required> | ||
| <div class="form-text"> | ||
| <p>{{ $t("Comma separated list of numbers in international format. (eg. 447930000000,447930000001)") }}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that all translations are in en.json as otherwise they cannot be translated
| <label for="mobivate-recipients" class="form-label">{{ $t("Recipients") }}</label> | ||
| <input id="mobivate-recipients" v-model="$parent.notification.mobivateRecipients" type="text" minlength="3" maxlength="20" pattern="^[\d+,]+$" class="form-control" required> | ||
| <div class="form-text"> | ||
| <p>{{ $t("Comma separated list of numbers in international format. (eg. 447930000000,447930000001)") }}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <p>{{ $t("Comma separated list of numbers in international format. (eg. 447930000000,447930000001)") }}</p> | |
| {{ $t("Comma separated list of numbers in international format. (eg. 447930000000,447930000001)") }} |
| <label for="mobivate-originator" class="form-label">{{ $t("Originator") }}</label> | ||
| <input id="mobivate-originator" v-model="$parent.notification.mobivateOriginator" type="text" minlength="3" maxlength="15" pattern="^[a-zA-Z0-9]*$" class="form-control" required> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a helptext to what this field is?
I would not know what this does ^^
| <label for="mobivate-key" class="form-label">{{ $t("API Key") }}<span style="color: red;"><sup>*</sup></span></label> | ||
| <HiddenInput id="mobivate-key" v-model="$parent.notification.mobivateApikey" :required="true" autocomplete="new-password"></HiddenInput> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a helptext where in the UI this can be found helpful or is this not a problem?
|
|
||
| let data = { | ||
| "originator": notification.mobivateOriginator.substring(0, 15), | ||
| "recipients": notification.mobivateRecipients.split(",").map(n => n.replace(/[^0-9]/g, "")).filter(n => n.length >= 9).map(recipient => ({recipient})), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering a recipient away is not very user obvious.
Could we either throw an error in such a case (obvious because the test button does not work) or add validation to the frontend?
π Overview
This pull request will add a Notification Type "Mobivate SMS" (www.mobivate.com)
Plain and Simple π
π οΈ Type of change
π Checklist