Skip to content

Conversation

@jhuseinovic
Copy link

πŸ“‹ Overview

This pull request will add a Notification Type "Mobivate SMS" (www.mobivate.com)

Plain and Simple πŸ‘

πŸ› οΈ Type of change

  • πŸ› Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • πŸ“„ New Documentation (addition of new documentation)
  • πŸ“„ Documentation Update (modification of existing documentation)
  • πŸ“„ Documentation Update Required (the change requires updates to related documentation)
  • πŸ”§ Other (please specify):
    • Provide additional details here.

πŸ“„ Checklist

  • πŸ” My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any) I used an LLM for the contributions
  • βœ… I ran ESLint and other code linters for modified files.
  • πŸ› οΈ I have reviewed and tested my code.
  • πŸ“ I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • πŸ€– My code needed automated testing. I have added them (this is an optional task).
  • πŸ“„ Documentation updates are included (if applicable).
  • πŸ”’ I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • πŸ“š I have read and understood the Pull Request guidelines.

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.

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);
Copy link
Collaborator

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>
Copy link
Collaborator

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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<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)") }}

Comment on lines +14 to +15
<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>
Copy link
Collaborator

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 ^^

Comment on lines +3 to +4
<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>
Copy link
Collaborator

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})),
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants