[Notifier] Add Mobyt bridge #36648

Deamon · 2020-04-30T21:10:47Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#33687
License	MIT
Doc PR	symfony/symfony-docs#13606
recipe PR	symfony/recipes#768

Add Mobyt notifier bridge.

In this SMS Provider, you can choose a sort of "quality service" to send the message.

I updated src/Symfony/Component/Notifier/Message/SmsMessage.php to add the notification in order to be able to use the notification importance when creating options.

src/Symfony/Component/Notifier/Bridge/Mobyt/MobytOptions.php

Deamon · 2020-06-12T15:27:50Z

Rebased few minutes ago on master.
Fixed conflicts and fixed notifier_transport that changed from xml to php.

fabpot · 2020-08-06T06:05:32Z

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

+        return $message;
+    }
+
+    public function getNotification(): ?Notification


This should be reverted

Hi @fabpot, you mean this function only or the entire modification of the file ?

The whole change in the SmsMessage class.

Question about that, I need to know the notification level inside the Moby Transport class to select a "type_quality" parameter (which implies cost differences).

Is there an other way ?

I would use the less costly by default and let's talk about this issue in another PR. That would allow to merge this one and think about the issue is a more broader sense instead of just for this specific provider.

Thank for you feedback. Fix is on the way.

fabpot · 2020-08-10T14:24:27Z

@Deamon Can you submit a PR on symfony/recipes (you can have a look at other notifier bridge recipes to get some inspiration)?

Deamon · 2020-08-10T14:33:11Z

Hello @fabpot, thanks for your comment,
Here is the pr: symfony/recipes#768

There is also a documentation pr waiting.

fabpot · 2020-08-10T15:44:31Z

@daemon Can you also rebase this PR on current master and take my comment into account? Thank you.

fabpot · 2020-08-10T15:45:24Z

src/Symfony/Component/Notifier/Bridge/Mobyt/MobytTransport.php

+        return $message instanceof SmsMessage;
+    }
+
+    protected function doSend(MessageInterface $message): void


This must return a SentMessage now.

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

fabpot · 2020-08-10T15:45:52Z

src/Symfony/Component/Notifier/Bridge/Mobyt/composer.json

+        "ext-json": "*",
+        "php": "^7.2.5",
+        "symfony/http-client": "^4.3|^5.0",
+        "symfony/notifier": "^5.0"


Should be ^5.2

Deamon · 2020-08-12T10:01:52Z

@fabpot, all changes have been pushed

fabpot · 2020-08-12T10:09:07Z

src/Symfony/Component/Notifier/Bridge/Mobyt/MobytOptions.php

+    {
+        $options = $this->options;
+        unset($options['message']);
+        unset($options['recipient']);


Could be merged with the previous line.

fabpot · 2020-08-12T10:09:25Z

src/Symfony/Component/Notifier/Bridge/Mobyt/.gitattributes

@@ -0,0 +1,4 @@
+/Tests export-ignore
+/phpunit.xml.dist export-ignore
+/.gitattributes export-ignore


Can be removed to be consistent with the other similar files in core.

fabpot · 2020-08-12T10:11:15Z

src/Symfony/Component/Notifier/Bridge/Mobyt/MobytTransport.php

+
+            throw new TransportException(sprintf('Unable to send the SMS: "%s".', $error['result']), $response);
+        }
+    }


You must return a SentMessage instance. Can you also test with a real account to be sure that everything works as expected?

Deamon · 2020-08-12T16:13:34Z

@fabpot your last feedback have been resolved.

the code is teste in v5.1 only, I couldn't get rid of the following error using dev-master branch:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!
!!  In PhpDumper.php line 1838:
!!
!!    Cannot dump definition because of invalid class name ('chatter.transport_factory').
!!
!!
!!
Script @auto-scripts was called via post-update-cmd
exit status 1

tried several ways but none worked for me.

and yes, the sentMessage part isn't tested because class doesn't exists in 5.1

fabpot · 2020-08-13T06:02:06Z

Thank you @Deamon.

This PR was merged into the master branch. Discussion ---------- [Notifier] Add Mobyt Notifier Hi all, here is the doc for symfony/symfony#36648 Commits ------- 63ea8b7 Add Mobyt Notifier doc

carsonbot added Status: Needs Review Notifier Feature labels Apr 30, 2020

Deamon mentioned this pull request Apr 30, 2020

[Notifier] Add Mobyt Notifier symfony/symfony-docs#13606

Merged

Deamon force-pushed the add-mobyt-notifier-bridge branch 2 times, most recently from a4d1cf1 to 512440f Compare April 30, 2020 22:26

Deamon mentioned this pull request Apr 30, 2020

Add Mobyt Notifier recipe symfony/recipes#768

Merged

OskarStark reviewed May 1, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Mobyt/MobytOptions.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Mobyt/MobytOptions.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone May 1, 2020

Deamon force-pushed the add-mobyt-notifier-bridge branch from 512440f to 3b5cdbb Compare May 2, 2020 20:26

Deamon force-pushed the add-mobyt-notifier-bridge branch from 3b5cdbb to 59acfa2 Compare June 12, 2020 15:24

fabpot requested changes Aug 6, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 6, 2020

fabpot reviewed Aug 10, 2020

View reviewed changes

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

fabpot reviewed Aug 10, 2020

View reviewed changes

Deamon force-pushed the add-mobyt-notifier-bridge branch from 59acfa2 to cee47a1 Compare August 12, 2020 09:20

fabpot requested changes Aug 12, 2020

View reviewed changes

Add Mobyt Notifier bridge

bf594b7

Deamon force-pushed the add-mobyt-notifier-bridge branch from cee47a1 to bf594b7 Compare August 12, 2020 16:10

fabpot approved these changes Aug 13, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Aug 13, 2020

fabpot merged commit acda2dc into symfony:master Aug 13, 2020

Deamon mentioned this pull request Aug 13, 2020

[Notifier] Add notification property to SmsMessage class #37818

Closed

Deamon deleted the add-mobyt-notifier-bridge branch August 14, 2020 06:46

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifier] Add Mobyt bridge #36648

[Notifier] Add Mobyt bridge #36648

Deamon commented Apr 30, 2020 •

edited

Loading

Deamon commented Jun 12, 2020

fabpot Aug 6, 2020

Deamon Aug 12, 2020

fabpot Aug 12, 2020

Deamon Aug 12, 2020

fabpot Aug 12, 2020

Deamon Aug 12, 2020

fabpot commented Aug 10, 2020

Deamon commented Aug 10, 2020 •

edited

Loading

fabpot commented Aug 10, 2020

fabpot Aug 10, 2020

fabpot Aug 10, 2020

Deamon commented Aug 12, 2020

fabpot Aug 12, 2020

fabpot Aug 12, 2020

fabpot Aug 12, 2020

Deamon commented Aug 12, 2020 •

edited

Loading

fabpot commented Aug 13, 2020

[Notifier] Add Mobyt bridge #36648

[Notifier] Add Mobyt bridge #36648

Conversation

Deamon commented Apr 30, 2020 • edited Loading

Deamon commented Jun 12, 2020

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

Choose a reason for hiding this comment

fabpot commented Aug 10, 2020

Deamon commented Aug 10, 2020 • edited Loading

fabpot commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deamon commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deamon commented Aug 12, 2020 • edited Loading

fabpot commented Aug 13, 2020

Deamon commented Apr 30, 2020 •

edited

Loading

Deamon commented Aug 10, 2020 •

edited

Loading

Deamon commented Aug 12, 2020 •

edited

Loading