Skip to content

feat:​​Allow templating in AliyunSMS #6002

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feat:​​Allow templating in AliyunSMS #6002

wants to merge 4 commits into from

Conversation

@rhwong
Copy link

@rhwong rhwong commented Jul 21, 2025

📋 Overview

  • What problem does this pull request address?
    • Resolves SMS delivery issues in Mainland China due to telecom operator policies (MIIT regulations). Original English alert messages (e.g., "Child inaccessible") were being blocked because they were misinterpreted as sensitive content (like transaction codes or IM account IDs).
  • What features or functionality does this pull request introduce or enhance?
    • Added Enable Custom Variables toggle to allow full customization of Aliyun SMS name and msg variables and custom UP/DOWN Text .

🛠️ 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.

📷 Screenshots or Visual Changes

  • UI Modifications: Highlight any changes made to the user interface.
  • Before & After: Include screenshots or comparisons (if applicable).
Event Before After
Aliyun-SMS setting Before After
SMS Text Before After

@rhwong rhwong mentioned this pull request Jul 21, 2025
Open
@CommanderStorm CommanderStorm changed the title feat:​​Allow customizing AliyunSMS variables for compliance feat:​​Allow templating in AliyunSMS Jul 21, 2025
CommanderStorm
CommanderStorm requested changes Jul 21, 2025
View reviewed changes
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please change this to allow templating instead of just custom static texts.
Just a staitc custom up/down text is not ideal as you can imagine.

Please see the following PR for the things that need to be changed:

@CommanderStorm CommanderStorm marked this pull request as draft July 21, 2025 12:38
@CommanderStorm CommanderStorm added A:notifications Issues or PRs related to notifications type:enhance-existing feature wants to enhance existing monitor pr:please address review comments this PR needs a bit more work to be mergable labels Jul 21, 2025
@rhwong
Copy link
Author

rhwong commented Jul 24, 2025 •
edited
Loading

Hi, I'd like to clarify a key constraint of the Aliyun SMS API: it only accepts four predefined parameters (${name}, ${time}, ${status}, ${msg}) with no flexibility to modify the message structure. The actual SMS template content is predefined on Aliyun's platform and requires operator approval - once approved, it cannot be modified. This fundamentally differs from channels like Telegram that support full message templating.

Given this limitation, I've implemented parameter-level templating in the PR. This allows users to customize the values passed to these parameters (e.g. mapping status to "在线" instead of "UP"), while respecting Aliyun's API constraints.

I'm not entirely sure if I've correctly interpreted your requirements. Could you please verify if this implementation maintains consistency with our notification system architecture? Really appreciate your guidance!

Event Before After
Aliyun-SMS setting Before After

CommanderStorm
CommanderStorm requested changes Jul 24, 2025
View reviewed changes
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.

closer, but I am still not quite happy with this

src/components/notifications/AliyunSms.vue
<label for="msgTemplate" class="form-label">{{ $t("${msg} Template") }}</label>
<textarea id="msgTemplate" v-model="$parent.notification.msgTemplate" class="form-control" rows="2" placeholder="{% if status == 'UP' %}请查看钉钉{% else %}请及时处理{% endif %}"></textarea>
</div>

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 can explain the limitation that templates are fixed and only the content can be changed more clearly.

src/components/notifications/AliyunSms.vue
<div class="form-check form-switch mb-3">
<input id="enableCustomTemplate" v-model="$parent.notification.enableCustomTemplate" class="form-check-input" type="checkbox">
<label for="enableCustomTemplate" class="form-check-label">{{ $t("Enable Custom Template") }}</label>
</div>
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 quick helptext to explain in which circumstances this setting is usefull?
I think currently a lot of people would not know when to use this

Copy link
Author

Choose a reason for hiding this comment

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

Of course, I will add it later.

src/components/notifications/AliyunSms.vue
Comment on lines +29 to +30
<label for="statusTemplate" class="form-label">{{ $t("${status} Template") }}</label>
<textarea id="statusTemplate" v-model="$parent.notification.statusTemplate" class="form-control" rows="2" placeholder="{% if status == 'UP' %}[在线]{% else %}[故障]{% endif %}"></textarea>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess with the added context that you cannot send a full template, I don't think this makes that much sense anymore to template the status.

Given that aliyun seems to be quite focussed on china, I think simplifying this to just respond with chinese (no translation) is fair.
Alternatively, just providing 3 inputs (up,down,message) are likely better.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, as only 4 variables can be sent to aliyun API, my initial solution was to forcefully overwrite the original English fields, allowing users to set them to any content. The content of the SMS is mainly to remind operation and maintenance personnel that there is a serious system failure, prompting them to pay attention in a timely manner.

src/components/notifications/AliyunSms.vue
Comment on lines +32 to +33
<label for="msgTemplate" class="form-label">{{ $t("${msg} Template") }}</label>
<textarea id="msgTemplate" v-model="$parent.notification.msgTemplate" class="form-control" rows="2" placeholder="{% if status == 'UP' %}请查看钉钉{% else %}请及时处理{% endif %}"></textarea>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't re-invent the weel here.

Use renderTemplate in the backend and TemplatedTextarea in the frontend.
See the PR I linked previously for an usage example

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

Reviewers

@CommanderStorm CommanderStorm CommanderStorm requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

A:notifications Issues or PRs related to notifications pr:please address review comments this PR needs a bit more work to be mergable type:enhance-existing feature wants to enhance existing monitor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rhwong @CommanderStorm