Skip to content

[Notifier] Added telnyx notifier bridge #38909

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
Closed
wants to merge 1 commit into from
Closed

[Notifier] Added telnyx notifier bridge #38909

wants to merge 1 commit into from

Conversation

krasilnikovs
Copy link
Contributor

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Added telnyx notifier bridge for sending sms

https://developers.telnyx.com/docs/api/v2/messaging/Messages?lang=curl#createMessage

@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from 06a2f12 to fd68f79 Compare October 30, 2020 20:52
jderusse
jderusse reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@jderusse jderusse 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.

Thank you for this contribution.

Don't forget to add an entry in the ChangeLog. and in UnsupportedSchemeException files

@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from fd68f79 to d109aa2 Compare October 30, 2020 21:07
@derrabus derrabus added this to the 5.x milestone Oct 30, 2020
@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from d109aa2 to 4325eba Compare October 30, 2020 21:16
@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from 4325eba to 48269b5 Compare November 7, 2020 19:15
jschaedl
jschaedl reviewed Nov 8, 2020
View reviewed changes
Copy link
Contributor

@jschaedl jschaedl left a comment

Choose a reason for hiding this comment

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

@krasilnikovm Thank you for your adjustments. Code looks good to me now, except the one line where sprintf could be used as well.

I know some of the bridges got merged without tests, but I think having tests here would be really great. Do you think you could add some?

@krasilnikovs
Copy link
Contributor Author

krasilnikovs commented Nov 8, 2020 •
edited
Loading

@krasilnikovm Thank you for your adjustments. Code looks good to me now, except the one line where sprintf could be used as well.

I know some of the bridges got merged without tests, but I think having tests here would be really great. Do you think you could add some?

Hi @jschaedl, sorry for I'm not checked other places for using sprintf , and now I'm working on writing tests but I have noticed that SmsMessage::getOptions method always returns null,

symfony/src/Symfony/Component/Notifier/Message/SmsMessage.php

Lines 98 to 101 in 0137609

public function getOptions(): ?MessageOptionsInterface
{
return null;
}

symfony/src/Symfony/Component/Notifier/Message/SmsMessage.php

Lines 28 to 37 in 0137609

public function __construct(string $phone, string $subject)
{
if ('' === $phone) {
throw new InvalidArgumentException(sprintf('"%s" needs a phone number, it cannot be empty.', static::class));
}
$this->subject = $subject;
$this->phone = $phone;
}

Can I add initializing options in the PR or better to create separate PR?

Updated
I have added initializing options in the PR

@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch 3 times, most recently from 821a6d7 to 12f35e7 Compare November 16, 2020 16:21
@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from 12f35e7 to dde143c Compare November 16, 2020 19:36
fabpot
fabpot requested changes Dec 5, 2020
View reviewed changes
src/Symfony/Component/Notifier/Message/SmsMessage.php Outdated

public function __construct(string $phone, string $subject)
public function __construct(string $phone, string $subject, MessageOptionsInterface $options = null)
Copy link
Member

Choose a reason for hiding this comment

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

This change should be removed and discussed in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR created: #39365

fabpot
fabpot requested changes Dec 5, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Looks like you forgot to update the src/Symfony/Component/Notifier/Exception/UnsupportedSchemeException.php file.

OskarStark
OskarStark approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Ready to merge after my comments 👍

@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch 2 times, most recently from c1b9210 to a526e46 Compare January 13, 2021 09:05
@krasilnikovs krasilnikovs mentioned this pull request Jan 14, 2021
Closed
OskarStark
OskarStark requested changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

This PR is blocked until #39774 is merged!

@OskarStark
Copy link
Contributor

Can you please rebase to get rid of the conflicts? Thanks

@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from a526e46 to 287c6d2 Compare January 22, 2021 14:31
@krasilnikovs krasilnikovs force-pushed the add-telnyx-notifier-bridge branch from 287c6d2 to 261cfee Compare January 22, 2021 15:02
@OskarStark
Copy link
Contributor

@krasilnikovm I need to close this PR, because it uses your other PR [Notifier] Added ability to initialize sms options which was closed ❌ without merging. 😢

Feel free to reopen and update this PR or reopen a new one if you found another solution without the need of MessageOptions for SMS.

Thanks for your patience 🚀

@OskarStark OskarStark closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@jderusse jderusse jderusse left review comments

@OskarStark OskarStark OskarStark requested changes

@derrabus derrabus derrabus left review comments

@jschaedl jschaedl jschaedl left review comments

@fabpot fabpot Awaiting requested review from fabpot

Assignees
No one assigned
Labels
Feature Notifier Status: Needs Work
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
9 participants
@krasilnikovs @OskarStark @fabpot @nicolas-grekas @stof @jderusse @derrabus @jschaedl @carsonbot