Merge branch 'master' into research-mode-implementation

Conflicts: tests/app/conftest.py
2026-08-11 17:38:02 -04:00 · 2016-06-02 11:53:06 +01:00
parent 5e1613e461 a51d6fae47
commit b8638a891f
9 changed files with 219 additions and 49 deletions
--- a/app/init.py
+++ b/app/init.py
@@ -52,7 +52,7 @@ def create_app(app_name=None):
    statsd_client.init_app(application)
    firetext_client.init_app(application, statsd_client=statsd_client)
    loadtest_client.init_app(application, statsd_client=statsd_client)
-    mmg_client.init_app(application.config, statsd_client=statsd_client)
+    mmg_client.init_app(application, statsd_client=statsd_client)
    aws_ses_client.init_app(application.config['AWS_REGION'], statsd_client=statsd_client)
    notify_celery.init_app(application)
    encryption.init_app(application)
--- a/app/clients/sms/firetext.py
+++ b/app/clients/sms/firetext.py
@@ -1,11 +1,12 @@
 import logging
+
 from monotonic import monotonic
+from requests import request, RequestException, HTTPError
+
 from app.clients.sms import (
    SmsClient,
    SmsClientException
 )
-from flask import current_app
-from requests import request, RequestException, HTTPError
 from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE

 logger = logging.getLogger(__name__)
@@ -55,10 +56,11 @@ class FiretextClient(SmsClient):
    FireText sms client.
    '''

-    def init_app(self, config, statsd_client, *args, **kwargs):
+    def init_app(self, current_app, statsd_client, *args, **kwargs):
        super(SmsClient, self).__init__(*args, **kwargs)
-        self.api_key = config.config.get('FIRETEXT_API_KEY')
-        self.from_number = config.config.get('FIRETEXT_NUMBER')
+        self.current_app = current_app
+        self.api_key = current_app.config.get('FIRETEXT_API_KEY')
+        self.from_number = current_app.config.get('FIRETEXT_NUMBER')
        self.name = 'firetext'
        self.statsd_client = statsd_client

@@ -86,6 +88,14 @@ class FiretextClient(SmsClient):
            if firetext_response['code'] != 0:
                raise FiretextClientException(firetext_response)
            response.raise_for_status()
+            self.current_app.logger.info(
+                "API {} request on {} succeeded with {} '{}'".format(
+                    "POST",
+                    "https://www.firetext.co.uk/api/sendsms",
+                    response.status_code,
+                    firetext_response
+                )
+            )
        except RequestException as e:
            api_error = HTTPError.create(e)
            logger.error(
@@ -100,6 +110,6 @@ class FiretextClient(SmsClient):
            raise api_error
        finally:
            elapsed_time = monotonic() - start_time
-            current_app.logger.info("Firetext request finished in {}".format(elapsed_time))
+            self.current_app.logger.info("Firetext request finished in {}".format(elapsed_time))
            self.statsd_client.timing("notifications.clients.firetext.request-time", elapsed_time)
        return response
--- a/app/clients/sms/mmg.py
+++ b/app/clients/sms/mmg.py
@@ -1,5 +1,4 @@
 import json
-from flask import current_app
 from monotonic import monotonic
 from requests import (request, RequestException, HTTPError)
 from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE)
@@ -57,10 +56,11 @@ class MMGClient(SmsClient):
    MMG sms client
    '''

-    def init_app(self, config, statsd_client, *args, **kwargs):
+    def init_app(self, current_app, statsd_client, *args, **kwargs):
        super(SmsClient, self).__init__(*args, **kwargs)
-        self.api_key = config.get('MMG_API_KEY')
-        self.from_number = config.get('MMG_FROM_NUMBER')
+        self.current_app = current_app
+        self.api_key = current_app.config.get('MMG_API_KEY')
+        self.from_number = current_app.config.get('MMG_FROM_NUMBER')
        self.name = 'mmg'
        self.statsd_client = statsd_client

@@ -84,12 +84,19 @@ class MMGClient(SmsClient):
                               headers={'Content-Type': 'application/json',
                                        'Authorization': 'Basic {}'.format(self.api_key)})
            if response.status_code != 200:
-                error = response.text
-                raise MMGClientException(json.loads(error))
+                raise MMGClientException(response.json())
            response.raise_for_status()
+            self.current_app.logger.info(
+                "API {} request on {} succeeded with {} '{}'".format(
+                    "POST",
+                    "https://www.mmgrp.co.uk/API/json/api.php",
+                    response.status_code,
+                    response.json()
+                )
+            )
        except RequestException as e:
            api_error = HTTPError.create(e)
-            current_app.logger.error(
+            self.current_app.logger.error(
                "API {} request on {} failed with {} '{}'".format(
                    "POST",
                    "https://www.mmgrp.co.uk/API/json/api.php",
@@ -102,5 +109,5 @@ class MMGClient(SmsClient):
        finally:
            elapsed_time = monotonic() - start_time
            self.statsd_client.timing("notifications.clients.mmg.request-time", elapsed_time)
-            current_app.logger.info("MMG request finished in {}".format(elapsed_time))
+            self.current_app.logger.info("MMG request finished in {}".format(elapsed_time))
        return response
--- a/app/template/rest.py
+++ b/app/template/rest.py
@@ -5,7 +5,6 @@ from flask import (
    current_app
 )
 import bleach
-from sqlalchemy.exc import IntegrityError

 from app.dao.templates_dao import (
    dao_update_template,
@@ -59,14 +58,18 @@ def update_template(service_id, template_id):
    fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id)

    current_data = dict(template_schema.dump(fetched_template).data.items())
-    current_data.update(request.get_json())
-    current_data['content'] = _strip_html(current_data['content'])
+    updated_template = dict(template_schema.dump(fetched_template).data.items())
+    updated_template.update(request.get_json())
+    updated_template['content'] = _strip_html(updated_template['content'])
+    # Check if there is a change to make.
+    if _template_has_not_changed(current_data, updated_template):
+        return jsonify(data=updated_template), 200

-    update_dict, errors = template_schema.load(current_data)
+    update_dict, errors = template_schema.load(updated_template)
    if errors:
        return jsonify(result="error", message=errors), 400
    over_limit, json_resp = _content_count_greater_than_limit(
-        current_data['content'],
+        updated_template['content'],
        fetched_template.template_type)
    if over_limit:
        return json_resp, 400
@@ -115,3 +118,10 @@ def get_template_versions(service_id, template_id):

 def _strip_html(content):
    return bleach.clean(content, tags=[], strip=True)
+
+
+def _template_has_not_changed(current_data, updated_template):
+    return all(
+        current_data[key] == updated_template[key]
+        for key in ('name', 'content', 'subject', 'archived')
+    )
--- a/tests/app/clients/test_firetext.py
+++ b/tests/app/clients/test_firetext.py
@@ -1,6 +1,9 @@
-import pytest
+from urllib.parse import parse_qs

-from app.clients.sms.firetext import get_firetext_responses
+import pytest
+import requests_mock
+
+from app.clients.sms.firetext import (get_firetext_responses, FiretextClientException)


 def test_should_return_correct_details_for_delivery():
@@ -31,3 +34,63 @@ def test_should_be_none_if_unrecognised_status_code():
    with pytest.raises(KeyError) as e:
        get_firetext_responses('99')
    assert '99' in str(e.value)
+
+
+def test_send_sms_successful_returns_firetext_response(mocker, mock_firetext_client):
+    to = content = reference = 'foo'
+    response_dict = {
+        'data': [],
+        'description': 'SMS successfully queued',
+        'code': 0,
+        'responseData': 1
+    }
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=200)
+        response = mock_firetext_client.send_sms(to, content, reference)
+
+    response_json = response.json()
+    assert response.status_code == 200
+    assert response_json['code'] == 0
+    assert response_json['description'] == 'SMS successfully queued'
+
+
+def test_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
+    to = '+447234567890'
+    content = 'my message'
+    reference = 'my reference'
+    response_dict = {
+        'code': 0,
+    }
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=200)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert request_mock.call_count == 1
+    assert request_mock.request_history[0].url == 'https://www.firetext.co.uk/api/sendsms/json'
+    assert request_mock.request_history[0].method == 'POST'
+
+    request_args = parse_qs(request_mock.request_history[0].text)
+    assert request_args['apiKey'][0] == 'foo'
+    assert request_args['from'][0] == 'bar'
+    assert request_args['to'][0] == '447234567890'
+    assert request_args['message'][0] == content
+    assert request_args['reference'][0] == reference
+
+
+def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
+    to = content = reference = 'foo'
+    response_dict = {
+        'data': [],
+        'description': 'Some kind of error',
+        'code': 1,
+        'responseData': ''
+    }
+
+    with pytest.raises(FiretextClientException) as exc, requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=200)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert exc.value.code == 1
+    assert exc.value.description == 'Some kind of error'
--- a/tests/app/clients/test_mmg.py
+++ b/tests/app/clients/test_mmg.py
@@ -1,4 +1,7 @@
-from app.clients.sms.mmg import get_mmg_responses
+import pytest
+import requests_mock
+
+from app.clients.sms.mmg import (get_mmg_responses, MMGClientException)


 def test_should_return_correct_details_for_delivery():
@@ -23,3 +26,54 @@ def test_should_be_none_if_unrecognised_status_code():
    assert response_dict['notification_status'] == 'failed'
    assert response_dict['notification_statistics_status'] == 'failure'
    assert not response_dict['success']
+
+
+def test_send_sms_successful_returns_mmg_response(mocker, mock_mmg_client):
+    to = content = reference = 'foo'
+    response_dict = {'Reference': 12345678}
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.mmgrp.co.uk/API/json/api.php', json=response_dict, status_code=200)
+        response = mock_mmg_client.send_sms(to, content, reference)
+
+    response_json = response.json()
+    assert response.status_code == 200
+    assert response_json['Reference'] == 12345678
+
+
+def test_send_sms_calls_mmg_correctly(mocker, mock_mmg_client):
+    to = '+447234567890'
+    content = 'my message'
+    reference = 'my reference'
+    response_dict = {'Reference': 12345678}
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.mmgrp.co.uk/API/json/api.php', json=response_dict, status_code=200)
+        mock_mmg_client.send_sms(to, content, reference)
+
+    assert request_mock.call_count == 1
+    assert request_mock.request_history[0].url == 'https://www.mmgrp.co.uk/API/json/api.php'
+    assert request_mock.request_history[0].method == 'POST'
+
+    request_args = request_mock.request_history[0].json()
+    assert request_args['reqType'] == 'BULK'
+    assert request_args['MSISDN'] == to
+    assert request_args['msg'] == content
+    assert request_args['sender'] == 'bar'
+    assert request_args['cid'] == reference
+    assert request_args['multi'] is True
+
+
+def test_send_sms_raises_if_mmg_rejects(mocker, mock_mmg_client):
+    to = content = reference = 'foo'
+    response_dict = {
+        'Error': 206,
+        'Description': 'Some kind of error'
+    }
+
+    with pytest.raises(MMGClientException) as exc, requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.mmgrp.co.uk/API/json/api.php', json=response_dict, status_code=400)
+        mock_mmg_client.send_sms(to, content, reference)
+
+    assert exc.value.code == 206
+    assert exc.value.description == 'Some kind of error'
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -1,6 +1,10 @@
 import requests_mock
 import pytest
+import uuid
 from datetime import (datetime, date)
+
+import pytest
+
 from app import db
 from app.models import (
    User,
@@ -21,7 +25,8 @@ from app.dao.api_key_dao import save_model_api_key
 from app.dao.jobs_dao import dao_create_job
 from app.dao.notifications_dao import dao_create_notification
 from app.dao.invited_user_dao import save_invited_user
-import uuid
+from app.clients.sms.firetext import FiretextClient
+from app.clients.sms.mmg import MMGClient


@pytest.yield_fixture
@@ -534,3 +539,27 @@ def sample_notification_statistics(notify_db,
    notify_db.session.add(stats)
    notify_db.session.commit()
    return stats
+
+
+@pytest.fixture(scope='function')
+def mock_firetext_client(mocker, statsd_client=None):
+    client = FiretextClient()
+    statsd_client = statsd_client or mocker.Mock()
+    current_app = mocker.Mock(config={
+        'FIRETEXT_API_KEY': 'foo',
+        'FIRETEXT_NUMBER': 'bar'
+    })
+    client.init_app(current_app, statsd_client)
+    return client
+
+
+@pytest.fixture(scope='function')
+def mock_mmg_client(mocker, statsd_client=None):
+    client = MMGClient()
+    statsd_client = statsd_client or mocker.Mock()()
+    current_app = mocker.Mock(config={
+        'MMG_API_KEY': 'foo',
+        'MMG_FROM_NUMBER': 'bar'
+    })
+    client.init_app(current_app, statsd_client)
+    return client
--- a/tests/app/template/test_rest.py
+++ b/tests/app/template/test_rest.py
@@ -3,6 +3,7 @@ import random
 import string
 from app.models import Template
 from tests import create_authorization_header
+from app.dao.templates_dao import dao_get_template_by_id


 def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_user, sample_service):
@@ -155,29 +156,9 @@ def test_must_have_a_subject_on_an_email_template(notify_api, sample_user, sampl
            assert json_resp['message'] == {'subject': ['Invalid template subject']}


-def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_service):
+def test_update_should_update_a_template(notify_api, sample_user, sample_template):
    with notify_api.test_request_context():
        with notify_api.test_client() as client:
-            data = {
-                'name': 'my template',
-                'template_type': 'email',
-                'subject': 'subject',
-                'content': 'template content',
-                'service': str(sample_service.id),
-                'created_by': str(sample_user.id)
-            }
-            data = json.dumps(data)
-            auth_header = create_authorization_header()
-
-            create_response = client.post(
-                '/service/{}/template'.format(sample_service.id),
-                headers=[('Content-Type', 'application/json'), auth_header],
-                data=data
-            )
-            assert create_response.status_code == 201
-            json_resp = json.loads(create_response.get_data(as_text=True))
-            assert json_resp['data']['name'] == 'my template'
-            assert json_resp['data']['version'] == 1
            data = {
                'content': 'my template has new content <script type="text/javascript">alert("foo")</script>',
                'created_by': str(sample_user.id)
@@ -186,7 +167,7 @@ def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_ser
            auth_header = create_authorization_header()

            update_response = client.post(
-                '/service/{}/template/{}'.format(sample_service.id, json_resp['data']['id']),
+                '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id),
                headers=[('Content-Type', 'application/json'), auth_header],
                data=data
            )
@@ -194,10 +175,12 @@ def test_should_be_able_to_update_a_template(notify_api, sample_user, sample_ser
            assert update_response.status_code == 200
            update_json_resp = json.loads(update_response.get_data(as_text=True))
            assert update_json_resp['data']['content'] == 'my template has new content alert("foo")'
+            assert update_json_resp['data']['name'] == sample_template.name
+            assert update_json_resp['data']['template_type'] == sample_template.template_type
            assert update_json_resp['data']['version'] == 2


-def test_should_be_able_to_archive_template(notify_api, sample_user, sample_service, sample_template):
+def test_should_be_able_to_archive_template(notify_api, sample_template):
    with notify_api.test_request_context():
        with notify_api.test_client() as client:
            data = {
@@ -444,3 +427,19 @@ def test_should_return_all_template_versions_for_service_and_template_id(notify_
                    assert x['content'] == original_content + '1'
                else:
                    assert x['content'] == original_content + '2'
+
+
+def test_update_does_not_create_new_version_when_there_is_no_change(notify_api, sample_template):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth_header = create_authorization_header()
+            data = {
+                'template_type': sample_template.template_type,
+                'content': sample_template.content,
+            }
+            resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id),
+                               data=json.dumps(data),
+                               headers=[('Content-Type', 'application/json'), auth_header])
+            assert resp.status_code == 200
+    template = dao_get_template_by_id(sample_template.id)
+    assert template.version == 1
--- a/tests/app/template/test_rest_history.py
+++ b/tests/app/template/test_rest_history.py
@@ -1,8 +1,6 @@
 import json
 from datetime import (datetime, date)
 from flask import url_for
-from app.models import Template
-from freezegun import freeze_time
 from app.dao.templates_dao import dao_update_template
 from tests import create_authorization_header