Skip to content

Conversation

@amirparsadd
Copy link
Contributor

📋 Overview

When I was using the SMSIR notification provider, it seemed like some notifications failed to send, it seems like the issue was from the message shortening logic so i did 2 things to absolutely make sure SMSes were being sent:

  1. set max message length to 20
  2. change how the shorten logic works (the old one didn't remove spaces)

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

@amirparsadd
Copy link
Contributor Author

Now that im thinking theres no need to decrease max length to 25

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.

If you do it this way a message will end up looking like similar to below

Yourcertificatehasex...

Are you sure that this is what you want?

It's really ugly to have this 20 char limit.

@amirparsadd
Copy link
Contributor Author

If you do it this way a message will end up looking like similar to below

Yourcertificatehasex...

Are you sure that this is what you want?

It's really ugly to have this 20 char limit.

Better than not receiving a message

Screenshot 1404-09-04 at 2 46 52 AM

this is what is happening rn (the error says parameter is longer than 25 chars)

and also this notification is mostly used for when the user doesn't have internet so they can get notified from sms, just knowing that something happened is enough, more info is available in the panel

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 am going to defer to your judgement on this one.
20 is below 25 by a few chars, but you likely chose this limit for a good reason.

In the end, you are likely the only user of this for a long time.

@CommanderStorm CommanderStorm enabled auto-merge (squash) November 25, 2025 00:13
@CommanderStorm CommanderStorm merged commit eb78389 into louislam:master Nov 25, 2025
20 checks passed
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