DRY-up overriding shortcode with sender

This avoids duplicating the logic when we add a new provider.

app/clients/sms/__init__.py

@@ -23,6 +23,7 @@ class SmsClient(Client):
     def init_app(self, current_app, statsd_client):
         self.current_app = current_app
         self.statsd_client = statsd_client
+        self.from_number = self.current_app.config.get('FROM_NUMBER')
 
     def record_outcome(self, success):
         log_message = "Provider request for {} {}".format(
@@ -39,6 +40,7 @@ class SmsClient(Client):
 
     def send_sms(self, to, content, reference, international, sender=None):
         start_time = monotonic()
+        sender = self.from_number if sender is None else sender
 
         try:
             response = self.try_send_sms(to, content, reference, international, sender)

app/clients/sms/firetext.py

@@ -65,17 +65,16 @@ class FiretextClient(SmsClient):
         super().init_app(*args, **kwargs)
         self.api_key = self.current_app.config.get('FIRETEXT_API_KEY')
         self.international_api_key = self.current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY')
-        self.from_number = self.current_app.config.get('FROM_NUMBER')
         self.url = self.current_app.config.get('FIRETEXT_URL')
 
     @property
     def name(self):
         return 'firetext'
 
-    def try_send_sms(self, to, content, reference, international, sender=None):
+    def try_send_sms(self, to, content, reference, international, sender):
         data = {
             "apiKey": self.international_api_key if international else self.api_key,
-            "from": self.from_number if sender is None else sender,
+            "from": sender,
             "to": to.replace('+', ''),
             "message": content,
             "reference": reference

app/clients/sms/mmg.py

@@ -71,19 +71,18 @@ class MMGClient(SmsClient):
     def init_app(self, *args, **kwargs):
         super().init_app(*args, **kwargs)
         self.api_key = self.current_app.config.get('MMG_API_KEY')
-        self.from_number = self.current_app.config.get('FROM_NUMBER')
         self.mmg_url = self.current_app.config.get('MMG_URL')
 
     @property
     def name(self):
         return 'mmg'
 
-    def try_send_sms(self, to, content, reference, international, sender=None):
+    def try_send_sms(self, to, content, reference, international, sender):
         data = {
             "reqType": "BULK",
             "MSISDN": to,
             "msg": content,
-            "sender": self.from_number if sender is None else sender,
+            "sender": sender,
             "cid": reference,
             "multi": True
         }

tests/app/clients/test_firetext.py

@@ -47,7 +47,7 @@ def test_try_send_sms_successful_returns_firetext_response(mocker, mock_firetext
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        response = mock_firetext_client.try_send_sms(to, content, reference, False)
+        response = mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
     response_json = response.json()
     assert response.status_code == 200
@@ -65,7 +65,7 @@ def test_try_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.try_send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'bar')
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/firetext'
@@ -89,7 +89,7 @@ def test_try_send_sms_calls_firetext_correctly_for_international(mocker, mock_fi
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.try_send_sms(to, content, reference, True)
+        mock_firetext_client.try_send_sms(to, content, reference, True, 'bar')
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/firetext'
@@ -114,7 +114,7 @@ def test_try_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.try_send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 200
     assert '"description": "Some kind of error"' in exc.value.text
@@ -127,36 +127,19 @@ def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mo
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=400)
-        mock_firetext_client.try_send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 400
     assert exc.value.text == '{"something": "gone bad"}'
     assert type(exc.value.exception) == HTTPError
 
 
-def test_try_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetext_client):
-    to = '+447234567890'
-    content = 'my message'
-    reference = 'my reference'
-    response_dict = {
-        'code': 0,
-    }
-    sender = 'fromservice'
-
-    with requests_mock.Mocker() as request_mock:
-        request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.try_send_sms(to, content, reference, False, sender=sender)
-
-    request_args = parse_qs(request_mock.request_history[0].text)
-    assert request_args['from'][0] == 'fromservice'
-
-
 def test_try_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client):
     to = content = reference = 'foo'
 
     with pytest.raises(FiretextClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/firetext', exc=ConnectTimeout)
-        mock_firetext_client.try_send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'
@@ -167,7 +150,7 @@ def test_try_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_f
 
     with pytest.raises(FiretextClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/firetext', exc=ReadTimeout)
-        mock_firetext_client.try_send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_mmg.py

@@ -44,7 +44,7 @@ def test_try_send_sms_successful_returns_mmg_response(notify_api, mocker):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        response = mmg_client.try_send_sms(to, content, reference, False)
+        response = mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
     response_json = response.json()
     assert response.status_code == 200
@@ -59,7 +59,7 @@ def test_try_send_sms_calls_mmg_correctly(notify_api, mocker):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        mmg_client.try_send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'testing')
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/mmg'
@@ -83,7 +83,7 @@ def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=400)
-        mmg_client.try_send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 400
     assert '"Error": 206' in exc.value.text
@@ -91,28 +91,13 @@ def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker):
     assert type(exc.value.exception) == HTTPError
 
 
-def test_try_send_sms_override_configured_shortcode_with_sender(notify_api, mocker):
-    to = '+447234567890'
-    content = 'my message'
-    reference = 'my reference'
-    response_dict = {'Reference': 12345678}
-    sender = 'fromservice'
-
-    with requests_mock.Mocker() as request_mock:
-        request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        mmg_client.try_send_sms(to, content, reference, False, sender=sender)
-
-    request_args = request_mock.request_history[0].json()
-    assert request_args['sender'] == 'fromservice'
-
-
 def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
     to = content = reference = 'foo'
     response_dict = 'NOT AT ALL VALID JSON {"key" : "value"}}'
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', text=response_dict, status_code=200)
-        mmg_client.try_send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value)  # noqa
     assert exc.value.status_code == 200
@@ -124,7 +109,7 @@ def test_try_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
 
     with pytest.raises(MMGClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/mmg', exc=ConnectTimeout)
-        mmg_client.try_send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'
@@ -135,7 +120,7 @@ def test_try_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
 
     with pytest.raises(MMGClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/mmg', exc=ReadTimeout)
-        mmg_client.try_send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_sms.py

@@ -27,7 +27,7 @@ def test_send_sms(fake_client, mocker):
     )
 
     mock_send.assert_called_with(
-        'to', 'content', 'reference', False, None
+        'to', 'content', 'reference', False, 'testing'
     )
 
 
@@ -43,3 +43,22 @@ def test_send_sms_error(fake_client, mocker):
             reference='reference',
             international=False,
         )
+
+
+def test_send_sms_override_configured_shortcode_with_sender(
+    fake_client,
+    mocker
+):
+    mock_send = mocker.patch.object(fake_client, 'try_send_sms')
+
+    fake_client.send_sms(
+        to='to',
+        content='content',
+        reference='reference',
+        international=False,
+        sender='sender'
+    )
+
+    mock_send.assert_called_with(
+        'to', 'content', 'reference', False, 'sender'
+    )