[Notifier] Add SmsBiuras notifier bridge #40691

StaffNowa · 2021-04-03T08:43:46Z

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

OskarStark

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

bool $test

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

OskarStark · 2021-04-03T09:03:11Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransportFactory.php

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


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

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

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

OskarStark · 2021-04-03T09:04:18Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

+                'message' => $message->getSubject(),
+                'from' => $this->from,
+                'to' => $message->getPhone(),
+                'test' => $this->test,


$test ? 0 : 1;

If Test is a bool

src/Symfony/Component/Notifier/Bridge/SmsBiuras/Tests/SmsBiurasTransportFactoryTest.php

StaffNowa · 2021-04-03T13:52:30Z

TODO: #40696

src/Symfony/Component/Notifier/Bridge/SmsBiuras/README.md

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

StaffNowa · 2021-04-04T15:56:15Z

Yes it is get 😏

OskarStark · 2021-04-05T06:33:43Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/README.md

+
+where:
+- `UID` is your client code
+- `TOKEN` is your SmsBiuras token


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

jderusse · 2021-04-06T12:10:02Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

+            throw new UnsupportedMessageTypeException(__CLASS__, SmsMessage::class, $message);
+        }
+
+        $endpoint = 'https://'.$this->getEndpoint().'/api?'.http_build_query([


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

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

jderusse · 2021-04-06T12:11:01Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

+
+        $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);


=== everywhere

jderusse · 2021-04-06T12:11:29Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

+
+        $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);


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

jderusse · 2021-04-06T12:16:03Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

+            return $sentMessage;
+        }
+
+        throw new TransportException('Unable to send the SMS: Service Unavailable.', $response);


Service Unavailable

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

Nyholm · 2021-04-08T06:43:09Z

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 · 2021-04-08T10:16:57Z

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

OskarStark · 2021-04-08T10:53:07Z

src/Symfony/Component/Notifier/Transport.php

+<<<<<<< HEAD
        MicrosoftTeamsTransport::class,
+=======
+        SmsBiurasTransportFactory::class,
+>>>>>>> faebf2ce75... * Transport.php - smsbiuras


Please fix the conflict

OskarStark · 2021-04-08T10:57:23Z

src/Symfony/Component/Notifier/Exception/UnsupportedSchemeException.php

@@ -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',


Suggested change

'package' => 'symfony/smsbiuras-notifier',

'package' => 'symfony/sms-biuras-notifier',

please rename the package name like this, thanks

StaffNowa · 2021-04-08T18:55:08Z

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

OskarStark · 2021-04-09T12:11:10Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/Tests/SmsBiurasTransportTest.php

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


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 · 2021-04-09T12:11:51Z

CHANGELOG file is missing

jderusse · 2021-04-10T21:03:04Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

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


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);

jderusse · 2021-04-10T21:07:26Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransportFactory.php

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


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)

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?

jderusse · 2021-04-10T21:10:03Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

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


Suggested change

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

$sentMessage->setMessageId($matches[1] ?? 0);

OskarStark · 2021-04-11T05:38:26Z

This commit looks weird 🧐

StaffNowa · 2021-04-11T06:17:42Z

Tried to solve weird commit :)

OskarStark · 2021-04-13T11:14:05Z

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

+    private $uid;
+    private $apiKey;
+    private $from;
+    private $test;


Please rename $test to testMode

and test to test_mode in the DSN

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php

src/Symfony/Component/Notifier/Bridge/SmsBiuras/README.md

src/Symfony/Component/Notifier/Bridge/SmsBiuras/Tests/SmsBiurasTransportTest.php

src/Symfony/Component/Notifier/Bridge/SmsBiuras/Tests/SmsBiurasTransportFactoryTest.php

jderusse

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

StaffNowa · 2021-04-14T07:01:08Z

@jderusse fixed. Thanks ;)

OskarStark · 2021-04-14T07:30:33Z

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

StaffNowa requested a review from OskarStark as a code owner April 3, 2021 08:43

carsonbot added Status: Needs Review Feature Notifier labels Apr 3, 2021

StaffNowa changed the title ~~[Notifier] Add MessageBird notifier bridge~~ [Notifier] Add SmsBiuras notifier bridge Apr 3, 2021

OskarStark reviewed Apr 3, 2021

View reviewed changes

OskarStark reviewed Apr 4, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/SmsBiuras/README.md Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php Outdated Show resolved Hide resolved

OskarStark reviewed Apr 4, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php Outdated Show resolved Hide resolved

OskarStark reviewed Apr 5, 2021

View reviewed changes

jderusse reviewed Apr 6, 2021

View reviewed changes

OskarStark reviewed Apr 8, 2021

View reviewed changes

StaffNowa requested review from chalasr, dunglas, sroze, wouterj, xabbuh and yceruto as code owners April 8, 2021 15:12

wouterj removed their request for review April 8, 2021 15:13

OskarStark reviewed Apr 9, 2021

View reviewed changes

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

jderusse reviewed Apr 10, 2021

View reviewed changes

OskarStark reviewed Apr 13, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/SmsBiuras/SmsBiurasTransport.php Outdated Show resolved Hide resolved

OskarStark added the Ready label Apr 13, 2021

OskarStark reviewed Apr 13, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/SmsBiuras/README.md Outdated Show resolved Hide resolved

OskarStark reviewed Apr 13, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/SmsBiuras/Tests/SmsBiurasTransportTest.php Outdated Show resolved Hide resolved

OskarStark reviewed Apr 13, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/SmsBiuras/Tests/SmsBiurasTransportFactoryTest.php Show resolved Hide resolved

OskarStark approved these changes Apr 13, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 13, 2021

OskarStark requested a review from jderusse April 13, 2021 14:39

carsonbot added Status: Needs Review and removed Status: Reviewed labels Apr 13, 2021

jderusse approved these changes Apr 14, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 14, 2021

[Notifier] Add SmsBiuras notifier bridge

79572b2

OskarStark merged commit cc29772 into symfony:5.x Apr 14, 2021

StaffNowa deleted the sms-biuras branch April 14, 2021 08:37

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifier] Add SmsBiuras notifier bridge #40691

[Notifier] Add SmsBiuras notifier bridge #40691

StaffNowa commented Apr 3, 2021 •

edited

Loading

OskarStark left a comment

OskarStark Apr 3, 2021

StaffNowa Apr 3, 2021

OskarStark Apr 3, 2021

StaffNowa Apr 3, 2021

StaffNowa commented Apr 3, 2021

StaffNowa commented Apr 4, 2021

OskarStark Apr 5, 2021

jderusse Apr 6, 2021

jderusse Apr 6, 2021

jderusse Apr 6, 2021

jderusse Apr 6, 2021

Nyholm commented Apr 8, 2021

StaffNowa commented Apr 8, 2021

OskarStark Apr 8, 2021

OskarStark Apr 8, 2021

StaffNowa commented Apr 8, 2021

OskarStark Apr 9, 2021

OskarStark commented Apr 9, 2021

jderusse Apr 10, 2021

jderusse Apr 14, 2021

jderusse Apr 10, 2021

OskarStark Apr 11, 2021

jderusse Apr 10, 2021

OskarStark commented Apr 11, 2021

StaffNowa commented Apr 11, 2021

OskarStark Apr 13, 2021

jderusse left a comment

StaffNowa commented Apr 14, 2021

OskarStark commented Apr 14, 2021

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

	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));

	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);

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

[Notifier] Add SmsBiuras notifier bridge #40691

[Notifier] Add SmsBiuras notifier bridge #40691

Conversation

StaffNowa commented Apr 3, 2021 • edited Loading

OskarStark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StaffNowa commented Apr 3, 2021

StaffNowa commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Apr 8, 2021

StaffNowa commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StaffNowa commented Apr 8, 2021

Choose a reason for hiding this comment

OskarStark commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark commented Apr 11, 2021

StaffNowa commented Apr 11, 2021

Choose a reason for hiding this comment

jderusse left a comment

Choose a reason for hiding this comment

StaffNowa commented Apr 14, 2021

OskarStark commented Apr 14, 2021

StaffNowa commented Apr 3, 2021 •

edited

Loading