Merge pull request #2 from mpociot/code-review

Code review

Author: mbardelmeijer

README.md

@@ -1,13 +1,23 @@
 # CMSMS notifications channel for Laravel 5.3
 
+[![Latest Version on Packagist](https://img.shields.io/packagist/v/laravel-notification-channels/cmsms.svg?style=flat-square)](https://packagist.org/packages/laravel-notification-channels/cmsms)
+[![Software License](https://img.shields.io/badge/license-MIT-brightgreen.svg?style=flat-square)](LICENSE.md)
+[![Build Status](https://img.shields.io/travis/laravel-notification-channels/cmsms/master.svg?style=flat-square)](https://travis-ci.org/laravel-notification-channels/cmsms)
+[![StyleCI](https://styleci.io/repos/xxxx/shield)](https://styleci.io/repos/xxxx)
+[![SensioLabsInsight](https://img.shields.io/sensiolabs/i/xxxxx.svg?style=flat-square)](https://insight.sensiolabs.com/projects/xxxx)
+[![Quality Score](https://img.shields.io/scrutinizer/g/laravel-notification-channels/cmsms.svg?style=flat-square)](https://scrutinizer-ci.com/g/laravel-notification-channels/cmsms)
+[![Code Coverage](https://img.shields.io/scrutinizer/coverage/g/laravel-notification-channels/cmsms/master.svg?style=flat-square)](https://scrutinizer-ci.com/g/laravel-notification-channels/cmsms/?branch=master)
+[![Total Downloads](https://img.shields.io/packagist/dt/laravel-notification-channels/cmsms.svg?style=flat-square)](https://packagist.org/packages/laravel-notification-channels/cmsms)
+
 This package makes it easy to send [CMSMS messages](https://dashboard.onlinesmsgateway.com/docs) with Laravel 5.3.
 
 ## Contents
 
 - [Requirements](#requirements)
 - [Installation](#installation)
-- [Setting up your CMSMS account](#setting-up-your-CMSMS-account)
+- [Setting up your CMSMS account](#setting-up-your-cmsms-account)
 - [Usage](#usage)
+	- [Available message methods](#available-message-methods)
 - [Changelog](#changelog)
 - [Testing](#testing)
 - [Security](#security)
@@ -52,7 +62,7 @@ Add your CMSMS Product Token and default originator (name or number of sender) t
 ...
 ```
 
-Notice: The originator can contain a maximum of 11 alfanumeric characters.
+Notice: The originator can contain a maximum of 11 alphanumeric characters.
 
 ## Usage
 
@@ -72,13 +82,29 @@ class VpsServerOrdered extends Notification
 
     public function toCmsms($notifiable)
     {
-        return (new CmsmsMessage("Your {$notifiable->service} was ordered!"));
+        return CmsmsMessage::create("Your {$notifiable->service} was ordered!");
     }
 }
 ```
 
+
+In order to let your Notification know which phone numer you are targeting, add the `routeNotificationForCmsms` method to your Notifiable model.
+
 **Important note**: CMCMS requires the recipients phone number to be in international format. For instance: 0031612345678
 
+```php
+public function routeNotificationForCmsms()
+{
+    return '0031612345678';
+}
+```
+
+### Available message methods
+
+- `body('')`: Accepts a string value for the message body.
+- `originator('')`: Accepts a string value between 1 and 11 characters, used as the message sender name.
+- `reference('')`: Accepts a string value for your message reference. This information will be returned in a status report so you can match the message and it's status. Restrictions: 1 - 32 alphanumeric characters and reference will not work for demo accounts.
+
 ## Changelog
 
 Please see [CHANGELOG](CHANGELOG.md) for more information what has changed recently.

composer.json

@@ -19,7 +19,8 @@
     },
     "require-dev": {
         "mockery/mockery": "^0.9.5",
-        "phpunit/phpunit": "4.*"
+        "phpunit/phpunit": "4.*",
+        "orchestra/testbench": "^3.3"
     },
     "autoload": {
         "psr-4": {
@@ -36,7 +37,5 @@
     },
     "config": {
         "sort-packages": true
-    },
-    "minimum-stability": "dev",
-    "prefer-stable": true
+    }
 }

src/CmsmsChannel.php

@@ -6,9 +6,7 @@
 
 class CmsmsChannel
 {
-    /**
-     * @var \NotificationChannels\Cmsms\CmsmsClient
-     */
+    /** @var CmsmsClient */
     protected $client;
 
     /**
@@ -20,19 +18,25 @@ public function __construct(CmsmsClient $client)
     }
 
     /**
+     * Send the given notification.
+     *
      * @param mixed $notifiable
      * @param \Illuminate\Notifications\Notification $notification
      *
      * @throws \NotificationChannels\Cmsms\Exceptions\CouldNotSendNotification
      */
     public function send($notifiable, Notification $notification)
     {
+        if (! $recipient = $notifiable->routeNotificationFor('Cmsms')) {
+            return;
+        }
+
         $message = $notification->toCmsms($notifiable);
 
         if (is_string($message)) {
             $message = CmsmsMessage::create($message);
         }
 
-        $this->client->send($message);
+        $this->client->send($message, $recipient);
     }
 }

src/CmsmsClient.php

@@ -2,23 +2,19 @@
 
 namespace NotificationChannels\Cmsms;
 
-use Exception;
 use GuzzleHttp\Client as GuzzleClient;
+use Illuminate\Support\Arr;
 use NotificationChannels\Cmsms\Exceptions\CouldNotSendNotification;
 use SimpleXMLElement;
 
 class CmsmsClient
 {
     const GATEWAY_URL = 'https://sgw01.cm.nl/gateway.ashx';
 
-    /**
-     * @var GuzzleClient
-     */
+    /** @var GuzzleClient */
     protected $client;
 
-    /**
-     * @var string
-     */
+    /** @var string */
     protected $productToken;
 
     /**
@@ -33,18 +29,17 @@ public function __construct(GuzzleClient $client, $productToken)
 
     /**
      * @param CmsmsMessage $message
+     * @param string $recipient
      * @throws CouldNotSendNotification
      */
-    public function send(CmsmsMessage $message)
+    public function send(CmsmsMessage $message, $recipient)
     {
-        if (empty($message->originator)) {
-            $message->setOriginator(config('services.cmsms.originator'));
+        if (is_null(Arr::get($message->toXmlArray(), 'FROM'))) {
+            $message->originator(config('services.cmsms.originator'));
         }
 
-        $messageXml = $this->buildMessageXml($message);
-
         $response = $this->client->request('POST', static::GATEWAY_URL, [
-            'body' => $messageXml,
+            'body' => $this->buildMessageXml($message, $recipient),
             'headers' => [
                 'Content-Type' => 'application/xml',
             ],
@@ -60,19 +55,21 @@ public function send(CmsmsMessage $message)
 
     /**
      * @param CmsmsMessage $message
+     * @param string $recipient
      * @return string
      */
-    protected function buildMessageXml(CmsmsMessage $message)
+    protected function buildMessageXml(CmsmsMessage $message, $recipient)
     {
         $xml = new SimpleXMLElement('<MESSAGES/>');
 
-        $authentication = $xml->addChild('AUTHENTICATION');
-        $authentication->addChild('PRODUCTTOKEN', $this->productToken);
+        $xml->addChild('AUTHENTICATION')
+            ->addChild('PRODUCTTOKEN', $this->productToken);
 
         $msg = $xml->addChild('MSG');
         foreach ($message->toXmlArray() as $name => $value) {
             $msg->addChild($name, $value);
         }
+        $msg->addChild('TO', $recipient);
 
         return $xml->asXML();
     }

src/CmsmsMessage.php

@@ -6,41 +6,28 @@
 
 class CmsmsMessage
 {
-    /**
-     * @var string
-     */
-    public $body;
+    /** @var string  */
+    protected $body;
 
-    /**
-     * @var string
-     */
-    public $originator;
+    /** @var string */
+    protected $originator;
 
-    /**
-     * @var string
-     */
-    public $recipient;
-
-    /**
-     * @var string
-     */
-    public $reference;
+    /** @var string */
+    protected $reference;
 
     /**
      * @param string $body
      */
     public function __construct($body = '')
     {
-        if (!empty($body)) {
-            $this->setBody($body);
-        }
+        $this->body($body);
     }
 
     /**
      * @param string $body
      * @return $this
      */
-    public function setBody($body)
+    public function body($body)
     {
         $this->body = trim($body);
 
@@ -50,21 +37,15 @@ public function setBody($body)
     /**
      * @param string|int $originator
      * @return $this
+     * @throws InvalidMessage
      */
-    public function setOriginator($originator)
+    public function originator($originator)
     {
-        $this->originator = (string) $originator;
-
-        return $this;
-    }
+        if (empty($originator) || strlen($originator) > 11) {
+            throw InvalidMessage::invalidOriginator($originator);
+        }
 
-    /**
-     * @param string|int $recipient
-     * @return $this
-     */
-    public function setRecipient($recipient)
-    {
-        $this->recipient = (string) $recipient;
+        $this->originator = (string) $originator;
 
         return $this;
     }
@@ -74,7 +55,7 @@ public function setRecipient($recipient)
      * @return $this
      * @throws InvalidMessage
      */
-    public function setReference($reference)
+    public function reference($reference)
     {
         if (empty($reference) || strlen($reference) > 32 || !ctype_alnum($reference)) {
             throw InvalidMessage::invalidReference($reference);
@@ -93,7 +74,6 @@ public function toXmlArray()
         return array_filter([
             'BODY' => $this->body,
             'FROM' => $this->originator,
-            'TO' => $this->recipient,
             'REFERENCE' => $this->reference,
         ]);
     }

src/Exceptions/CouldNotSendNotification.php

@@ -10,7 +10,7 @@ class CouldNotSendNotification extends Exception
      * @param string $error
      * @return static
      */
-    public static function serviceRespondedWithAnError(string $error)
+    public static function serviceRespondedWithAnError($error)
     {
         return new static("CMSMS service responded with an error: {$error}'");
     }

src/Exceptions/InvalidMessage.php

@@ -14,4 +14,13 @@ public static function invalidReference($reference)
     {
         return new static("The reference on the CMSMS message may only contain 1 - 32 alphanumeric characters. Was given '{$reference}'");
     }
+
+    /**
+     * @param string $originator
+     * @return static
+     */
+    public static function invalidOriginator($originator)
+    {
+        return new static("The originator on the CMSMS message may only contain 1 - 11 characters. Was given '{$originator}'");
+    }
 }

tests/CmsmsChannelTest.php

@@ -57,6 +57,6 @@ class TestNotification extends Notification
 {
     public function toCmsms($notifiable)
     {
-        return (new CmsmsMessage('Message content'))->setOriginator('APPNAME')->setRecipient('0031612345678');
+        return (new CmsmsMessage('Message content'))->originator('APPNAME');
     }
 }

tests/CmsmsClientTest.php

@@ -8,15 +8,18 @@
 use NotificationChannels\Cmsms\CmsmsClient;
 use NotificationChannels\Cmsms\CmsmsMessage;
 use NotificationChannels\Cmsms\Exceptions\CouldNotSendNotification;
+use Orchestra\Testbench\TestCase;
 use PHPUnit_Framework_TestCase;
 
-class CmsmsClientTest extends PHPUnit_Framework_TestCase
+class CmsmsClientTest extends TestCase
 {
     public function setUp()
     {
+        parent::setUp();
+        $this->app['config']['services.cmsms.originator'] = 'My App';
         $this->guzzle = Mockery::mock(new Client());
         $this->client = Mockery::mock(new CmsmsClient($this->guzzle, '00000FFF-0000-F0F0-F0f0-FFFFFFFFFFFF'));
-        $this->message = (new CmsmsMessage('Message content'))->setOriginator('APPNAME')->setRecipient('0031612345678');
+        $this->message = (new CmsmsMessage('Message content'))->originator('APPNAME');
     }
 
     public function tearDown()
@@ -38,9 +41,25 @@ public function it_can_send_message()
         $this->guzzle
             ->shouldReceive('request')
             ->once()
-            ->andReturn(new Response(200, [], 'error'));
+            ->andReturn(new Response(200, [], ''));
+
+        $this->client->send($this->message, '00301234');
+    }
+
+    /** @test */
+    public function it_sets_a_default_originator_if_none_is_set()
+    {
+        $message = Mockery::mock(new CmsmsMessage('Message body'));
+        $message->shouldReceive('originator')
+                ->once()
+                ->with($this->app['config']['services.cmsms.originator']);
+
+        $this->guzzle
+            ->shouldReceive('request')
+            ->once()
+            ->andReturn(new Response(200, [], ''));
 
-        $this->client->send($this->message);
+        $this->client->send($message, '00301234');
     }
 
     /** @test */
@@ -53,6 +72,6 @@ public function it_throws_exception_on_error_response()
             ->once()
             ->andReturn(new Response(200, [], 'error'));
 
-        $this->client->send($this->message);
+        $this->client->send($this->message, '00301234');
     }
 }