Skip to content
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

Twilio customized to phone number #119

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Twilio customized to phone number #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a938e53
override to number in message
skybitbbsr Mar 3, 2021
15bada3
use to number from message if defined
skybitbbsr Mar 3, 2021
8c625b3
added removed logic
mfsi-suryar Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/TwilioChannel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ public function __construct(Twilio $twilio, Dispatcher $events)
public function send($notifiable, Notification $notification)
{
try {
$to = $this->getTo($notifiable, $notification);
$message = $notification->toTwilio($notifiable);
$useSender = $this->canReceiveAlphanumericSender($notifiable);

if (is_string($message)) {
$message = new TwilioSmsMessage($message);
Expand All @@ -56,6 +54,9 @@ public function send($notifiable, Notification $notification)
throw CouldNotSendNotification::invalidMessageObject($message);
}

$to = $this->getTo($notifiable, $notification, $message);
Copy link
Member

Choose a reason for hiding this comment

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

I commented on the last PR, but shouldn't we merge the additional tos instead of overridding?

Also, could you fix the tests please?

Copy link
Author

@skybitbbsr skybitbbsr Mar 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. We shouldn't. As the notification is overriding the default phone number, we should not send also to the primary number defined in the model. But we can have one ->addTo([]) method to add list of phone numbers on primary phone or the phone number provided in the ->to() method.

  2. I am working on the tests.

Copy link

Choose a reason for hiding this comment

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

What happened with this PR? If this is not going anywhere, can I make a PR with proper tests for this patch?

$useSender = $this->canReceiveAlphanumericSender($notifiable);

return $this->twilio->sendMessage($message, $to, $useSender);
} catch (Exception $exception) {
$event = new NotificationFailed(
Expand All @@ -79,13 +80,17 @@ public function send($notifiable, Notification $notification)
* Get the address to send a notification to.
*
* @param mixed $notifiable
* @param Notification|null $notification
* @param Notification $notification
* @param TwilioMessage $message
*
* @return mixed
* @throws CouldNotSendNotification
*/
protected function getTo($notifiable, $notification = null)
protected function getTo($notifiable, $notification, $message)
{
if ($message->getTo()) {
return $message->getTo();
}
if ($notifiable->routeNotificationFor(self::class, $notification)) {
return $notifiable->routeNotificationFor(self::class, $notification);
}
Expand Down
31 changes: 31 additions & 0 deletions src/TwilioMessage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ abstract class TwilioMessage
* @var string
*/
public $from;

/**
* The phone number the message should be sent to.
* This is optional. If available, will override $notifiable phone number
*
* @var string
*/
public $to;

/**
* @var null|string
Expand Down Expand Up @@ -83,6 +91,29 @@ public function getFrom(): ?string
{
return $this->from;
}

/**
* Set the phone number the message should be sent to.
*
* @param string $to
* @return $this
*/
public function to(string $to): self
{
$this->to = $to;

return $this;
}

/**
* Get the to address.
*
* @return string|null
*/
public function getTo(): ?string
{
return $this->to;
}

/**
* Set the status callback.
Expand Down