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.

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

[Notifier] Add SmsBiuras notifier bridge #40691

Merged
merged 1 commit into from
Apr 14, 2021
Merged

[Notifier] Add SmsBiuras notifier bridge #40691

merged 1 commit into from
Apr 14, 2021

Conversation

StaffNowa
Copy link
Contributor

@StaffNowa StaffNowa commented Apr 3, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#15219
Recipe PR symfony/recipes/pull/932

Sms Biuras notifier bridge

@StaffNowa StaffNowa changed the title [Notifier] Add MessageBird notifier bridge [Notifier] Add SmsBiuras notifier bridge Apr 3, 2021
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.

Test option should be optional in the DSN.
The argument in the Transport should not be optional:

bool $test

$from = $dsn->getRequiredOption('from');
$host = 'default' === $dsn->getHost() ? null : $dsn->getHost();
$port = $dsn->getPort();
$test = $dsn->getRequiredOption('test');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move below the from and make it optional by using:

$test = $dsn->getOption('test', false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'message' => $message->getSubject(),
'from' => $this->from,
'to' => $message->getPhone(),
'test' => $this->test,
Copy link
Contributor

Choose a reason for hiding this comment

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

$test ? 0 : 1;

If Test is a bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@StaffNowa
Copy link
Contributor Author

TODO: #40696

@StaffNowa
Copy link
Contributor Author

Screenshot_2021-04-04-18-55-33-022_com android chrome
Yes it is get 😏


where:
- `UID` is your client code
- `TOKEN` is your SmsBiuras token
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename TOKEN to API_KEY and $token to $apiKey because they name it the same:

image

throw new UnsupportedMessageTypeException(__CLASS__, SmsMessage::class, $message);
}

$endpoint = 'https://'.$this->getEndpoint().'/api?'.http_build_query([
Copy link
Member

Choose a reason for hiding this comment

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

use $response = $this->client->request('GET', $endpoint, ['query' => [....]]) instead of building the URL


$matches = [];
if (preg_match('/^ERROR: (\d+)$/', $response->getContent(), $matches)) {
throw new TransportException('Unable to send the SMS: '.$this->getErrorMsg(2 == \count($matches) ? $matches[1] : 999), $response);
Copy link
Member

Choose a reason for hiding this comment

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

=== everywhere


$matches = [];
if (preg_match('/^ERROR: (\d+)$/', $response->getContent(), $matches)) {
throw new TransportException('Unable to send the SMS: '.$this->getErrorMsg(2 == \count($matches) ? $matches[1] : 999), $response);
Copy link
Member

Choose a reason for hiding this comment

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

-2 == \count($matches) ? $matches[1] : 999
+$matches[1] ?? 999

return $sentMessage;
}

throw new TransportException('Unable to send the SMS: Service Unavailable.', $response);
Copy link
Member

Choose a reason for hiding this comment

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

Service Unavailable

You don't know it. I'd use Unable to send the SMS.

@Nyholm
Copy link
Member

Nyholm commented Apr 8, 2021

There has been some recent changes in the CI flow that will be good for this PR. Could I ask you to rebase this PR please?

@StaffNowa
Copy link
Contributor Author

@Nyholm made rebase, but see now too much commits.

Comment on lines 81 to 85
<<<<<<< HEAD
MicrosoftTeamsTransport::class,
=======
SmsBiurasTransportFactory::class,
>>>>>>> faebf2ce75... * Transport.php - smsbiuras
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the conflict

@@ -132,6 +132,10 @@ class UnsupportedSchemeException extends LogicException
'class' => Bridge\MicrosoftTeams\MicrosoftTeamsTransportFactory::class,
'package' => 'symfony/microsoft-teams-notifier',
],
'smsbiuras' => [
'class' => Bridge\SmsBiuras\SmsBiurasTransportFactory::class,
'package' => 'symfony/smsbiuras-notifier',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'package' => 'symfony/smsbiuras-notifier',
'package' => 'symfony/sms-biuras-notifier',

please rename the package name like this, thanks

@wouterj wouterj removed their request for review April 8, 2021 15:13
@StaffNowa
Copy link
Contributor Author

Finally return "home" to the good branch 👍 Do I need to make something with license headers? :)

*/
public function createTransport(?HttpClientInterface $client = null): TransportInterface
{
return new SmsBiurasTransport('uid', 'api_key', 'from', 1, $client ?: $this->createMock(HttpClientInterface::class));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return new SmsBiurasTransport('uid', 'api_key', 'from', 1, $client ?: $this->createMock(HttpClientInterface::class));
return new SmsBiurasTransport('uid', 'api_key', 'from', 1, $client ?? $this->createMock(HttpClientInterface::class));

@OskarStark
Copy link
Contributor

CHANGELOG file is missing

@jderusse jderusse added this to the 5.x milestone Apr 10, 2021

$matches = [];
if (preg_match('/^ERROR: (\d+)$/', $response->getContent(), $matches)) {
throw new TransportException('Unable to send the SMS: '.$this->getErrorMsg(2 === $matches[1] ?? 999), $response);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new TransportException('Unable to send the SMS: '.$this->getErrorMsg(2 === $matches[1] ?? 999), $response);
throw new TransportException('Unable to send the SMS: '.$this->getErrorMsg($matches[1] ?? 999), $response);

Copy link
Member

Choose a reason for hiding this comment

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

UP

$uid = $this->getUser($dsn);
$apiKey = $this->getPassword($dsn);
$from = $dsn->getRequiredOption('from');
$test = $dsn->getOption('test', false);
Copy link
Member

Choose a reason for hiding this comment

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

When the option is provided by DSN, you'll get a string.
You should explicitly convert it to boolean with something like
filter_var($dsn->getOption('test', false), \FILTER_VALIDATE_BOOLEAN)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not quite happy how we activate the test mode via DSN. Any other idea @jderusse ?

Anyway I would propose to rename it to "test_mode"

Any maybe use "on" and "off" for toggeling instead of magic/Boolean numbers?

$matches = [];
if (preg_match('/^OK: (\d+)$/', $response->getContent(), $matches)) {
$sentMessage = new SentMessage($message, (string) $this);
$sentMessage->setMessageId(2 === \count($matches) ? $matches[1] : 0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$sentMessage->setMessageId(2 === \count($matches) ? $matches[1] : 0);
$sentMessage->setMessageId($matches[1] ?? 0);

@OskarStark
Copy link
Contributor

image

This commit looks weird 🧐

@StaffNowa
Copy link
Contributor Author

Tried to solve weird commit :)

private $uid;
private $apiKey;
private $from;
private $test;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename $test to testMode

and test to test_mode in the DSN

Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

LGTM except the 2 === $matches[1] ?? 999

@StaffNowa
Copy link
Contributor Author

@jderusse fixed. Thanks ;)

@OskarStark
Copy link
Contributor

It took time, but here we go, this is in now. Thank you very much Vasilij.

@OskarStark OskarStark merged commit cc29772 into symfony:5.x Apr 14, 2021
@StaffNowa StaffNowa deleted the sms-biuras branch April 14, 2021 08:37
@fabpot fabpot mentioned this pull request Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants