Mobivate SMS Notification plugin#6451
Conversation
There was a problem hiding this comment.
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.
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.
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.
| <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.
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.
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.
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?
CommanderStorm
added
the
pr:please address review comments
|
@jhuseinovic genlte ping: would you like to continue this PR or should I close this for now and we reopen when you have the time to adress the code review? |
2 participants
📋 Overview
This pull request will add a Notification Type "Mobivate SMS" (www.mobivate.com)
Plain and Simple 👍
🛠️ Type of change
📄 Checklist