Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Add notification property to SmsMessage class#37818
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Notifier] Add notification property to SmsMessage class#37818
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| return$message; | ||
| } | ||
| publicfunctiongetNotification(): ?Notification |
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.
As I said in the other PR, I don't like that you can get a Notification from the SMS message, it does not feel right. Instead, if many providers have a "quality" setting, we might add a quality property on the SMS message. Can you see if "big" providers like Nexmo or Twilio also have such a setting?
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 know, I put thechangehere to open the discussion.
It appears that nothing equivalent is present in thoses provider.
They offer an sms pricing without paid options. Or I couldn't find it.
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.
That's what I feared unfortunately. Using options on the message is still possible though. So, I think it could be enough for people really wanting to use this feature.
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.
ok thanks for yout feedback.
I might need to clean the mobyt bridge to delete code that won't be used (e.g.: fromNotification in MobytOptions)
This PR is a follow up from the discussion there:#36648 (comment)
The idea is to allow SmsTransports to get the notification level in order to use or set some advanced parameters in the transport.
Work case: MobytTransport, added in PR#36648, has a parameter to choose between 3 levels of delivery quality(and costs).
Depending on the notification importance we should be able to set the parameter.
I'm open to any suggestion to make it as generic as possible.