diff --git a/app/__init__.py b/app/__init__.py index be7bd43ed..77bf25a47 100644 --- 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) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 2d77c777d..2dfba89f0 100644 --- 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 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 70148e2c3..5d4698ba8 100644 --- 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 diff --git a/app/template/rest.py b/app/template/rest.py index 752417e0f..168d483ea 100644 --- 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') + ) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 96627068f..7804eaee9 100644 --- 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' diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 60210afd8..aa5bb9f22 100644 --- 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' diff --git a/tests/app/conftest.py b/tests/app/conftest.py index a0e002f17..8dc6cae4b 100644 --- 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 diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index b7ea2d47e..45e7300e6 100644 --- 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 ', '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 diff --git a/tests/app/template/test_rest_history.py b/tests/app/template/test_rest_history.py index 5b0c919b6..482b04aad 100644 --- 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