From 2a0f8c88084fb6d6fa087c89de9a8f187590483d Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 26 Apr 2017 15:56:45 +0100 Subject: [PATCH 1/8] Validate International phone numbers - uses new utils methods to validate phone numbers - defaults to International=True on validation. This ensures the validator works on all numbers - Then check if the user can send this message to the number internationally if needed. --- app/notifications/process_notifications.py | 11 ++- app/notifications/rest.py | 29 +++++-- app/notifications/validators.py | 17 +++- app/schema_validation/__init__.py | 2 +- app/schemas.py | 4 +- app/v2/notifications/post_notifications.py | 3 +- requirements.txt | 2 +- tests/app/conftest.py | 4 +- tests/app/delivery/test_send_to_providers.py | 8 +- .../rest/test_send_notification.py | 82 ++++++++++++++++++- tests/app/notifications/test_validators.py | 20 ++++- .../notifications/test_post_notifications.py | 70 +++++++++++++++- 12 files changed, 224 insertions(+), 28 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 31d8b8c1b..033943511 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -4,6 +4,7 @@ from flask import current_app from app import redis_store from app.celery import provider_tasks +from notifications_utils.recipients import validate_and_format_phone_number from notifications_utils.clients import redis from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE @@ -98,6 +99,10 @@ def send_notification_to_queue(notification, research_mode, queue=None): def simulated_recipient(to_address, notification_type): - return (to_address in current_app.config['SIMULATED_SMS_NUMBERS'] - if notification_type == SMS_TYPE - else to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES']) + if notification_type == SMS_TYPE: + formatted_simulated_numbers = [ + validate_and_format_phone_number(number) for number in current_app.config['SIMULATED_SMS_NUMBERS'] + ] + return to_address in formatted_simulated_numbers + else: + return to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES'] diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 57e1efeed..2c225361b 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -2,8 +2,7 @@ from flask import ( Blueprint, jsonify, request, - current_app, - json + current_app ) from app import api_user @@ -14,10 +13,6 @@ from app.dao import ( ) from app.models import KEY_TYPE_TEAM, PRIORITY from app.models import SMS_TYPE -from app.notifications.process_client_response import ( - validate_callback_data, - process_sms_client_response -) from app.notifications.process_notifications import (persist_notification, send_notification_to_queue, simulated_recipient) @@ -35,6 +30,8 @@ from app.schemas import ( from app.service.utils import service_allowed_to_send_to from app.utils import pagination_links, get_template_instance +from notifications_utils.recipients import get_international_phone_info + notifications = Blueprint('notifications', __name__) from app.errors import ( @@ -104,13 +101,15 @@ def send_notification(notification_type): notification_form, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) + if errors: raise InvalidRequest(errors, status_code=400) check_service_message_limit(api_user.key_type, service) - template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification_form['template'], - service_id=service.id) + template = templates_dao.dao_get_template_by_id_and_service_id( + template_id=notification_form['template'], + service_id=service.id) check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) @@ -118,12 +117,14 @@ def send_notification(notification_type): template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {})) _service_allowed_to_send_to(notification_form, service) + if notification_type == SMS_TYPE: + _service_can_send_internationally(service, notification_form['to']) # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(notification_form['to'], notification_type) notification_model = persist_notification(template_id=template.id, template_version=template.version, - recipient=notification_form['to'], + recipient=request.get_json()['to'], service=service, personalisation=notification_form.get('personalisation', None), notification_type=notification_type, @@ -160,6 +161,16 @@ def get_notification_return_data(notification_id, notification, template): return output +def _service_can_send_internationally(service, number): + international_phone_info = get_international_phone_info(number) + + if international_phone_info.international and not service.can_send_international_sms: + raise InvalidRequest( + {'to': ["Cannot send to international mobile numbers"]}, + status_code=400 + ) + + def _service_allowed_to_send_to(notification, service): if not service_allowed_to_send_to(notification['to'], service, api_user.key_type): if api_user.key_type == KEY_TYPE_TEAM: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index aefee34e2..92f15b909 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,5 +1,9 @@ from flask import current_app -from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address +from notifications_utils.recipients import ( + validate_and_format_phone_number, + validate_and_format_email_address, + get_international_phone_info +) from app.dao import services_dao from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM, SMS_TYPE @@ -47,8 +51,17 @@ def service_can_send_to_recipient(send_to, key_type, service): def validate_and_format_recipient(send_to, key_type, service, notification_type): service_can_send_to_recipient(send_to, key_type, service) + if notification_type == SMS_TYPE: - return validate_and_format_phone_number(number=send_to) + international_phone_info = get_international_phone_info(send_to) + + if international_phone_info.international and not service.can_send_international_sms: + raise BadRequestError(message="Cannot send to international mobile numbers") + + return validate_and_format_phone_number( + number=send_to, + international=international_phone_info.international + ) else: return validate_and_format_email_address(email_address=send_to) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 674d83ff8..72767c881 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -11,7 +11,7 @@ def validate(json_to_validate, schema): @format_checker.checks('phone_number', raises=InvalidPhoneError) def validate_schema_phone_number(instance): if instance is not None: - validate_phone_number(instance) + validate_phone_number(instance, international=True) return True @format_checker.checks('email_address', raises=InvalidEmailError) diff --git a/app/schemas.py b/app/schemas.py index 510d7e59e..2a42c7d15 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -324,13 +324,13 @@ class SmsNotificationSchema(NotificationSchema): @validates('to') def validate_to(self, value): try: - validate_phone_number(value) + validate_phone_number(value, international=True) except InvalidPhoneError as error: raise ValidationError('Invalid phone number: {}'.format(error)) @post_load def format_phone_number(self, item): - item['to'] = validate_and_format_phone_number(item['to']) + item['to'] = validate_and_format_phone_number(item['to'], international=True) return item diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5f046edb0..1822e8734 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -27,6 +27,7 @@ def post_notification(notification_type): form = validate(request.get_json(), post_email_request) else: form = validate(request.get_json(), post_sms_request) + service = services_dao.dao_fetch_service_by_id(api_user.service_id) check_service_message_limit(api_user.key_type, service) form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] @@ -41,7 +42,7 @@ def post_notification(notification_type): simulated = simulated_recipient(send_to, notification_type) notification = persist_notification(template_id=template.id, template_version=template.version, - recipient=send_to, + recipient=form_send_to, service=service, personalisation=form.get('personalisation', None), notification_type=notification_type, diff --git a/requirements.txt b/requirements.txt index a3dee4741..c90f21848 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@15.2.1#egg=notifications-utils==15.2.1 +git+https://github.com/alphagov/notifications-utils.git@16.1.0#egg=notifications-utils==16.1.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fab72ae7e..de69e9240 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -123,7 +123,8 @@ def sample_service( user=None, restricted=False, limit=1000, - email_from=None + email_from=None, + can_send_international_sms=False ): if user is None: user = create_user() @@ -136,6 +137,7 @@ def sample_service( 'email_from': email_from, 'created_by': user, 'letter_contact_block': 'London,\nSW1A 1AA', + 'can_send_international_sms': can_send_international_sms } service = Service.query.filter_by(name=service_name).first() if not service: diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 2894d75e9..80b08de4a 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -4,7 +4,7 @@ from collections import namedtuple from unittest.mock import ANY import pytest -from notifications_utils.recipients import validate_phone_number, format_phone_number +from notifications_utils.recipients import validate_and_format_phone_number import app from app import mmg_client @@ -69,7 +69,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), + to=validate_and_format_phone_number("+447234123123"), content="Sample service: Hello Jo\nHere is some HTML & entities", reference=str(db_notification.id), sender=None @@ -151,7 +151,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), + to=validate_and_format_phone_number("+447234123123"), content="Sample service: This is a template:\nwith a newline", reference=str(db_notification.id), sender=None @@ -254,7 +254,7 @@ def test_should_send_sms_sender_from_service_if_present( ) mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), + to=validate_and_format_phone_number("+447234123123"), content="This is a template:\nwith a newline", reference=str(db_notification.id), sender=sample_service.sms_sender diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index d6a4e9c31..8fe9f09b3 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -20,8 +20,8 @@ from tests.app.conftest import ( sample_email_template as create_sample_email_template, sample_template as create_sample_template, sample_service_whitelist as create_sample_service_whitelist, - sample_api_key as create_sample_api_key -) + sample_api_key as create_sample_api_key, + sample_service) from app.models import Template from app.errors import InvalidRequest @@ -1046,3 +1046,81 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori assert response.status_code == 201 mocked.assert_called_once_with([notification_id], queue='priority') + + +def test_should_allow_store_original_number_on_sms_notification(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': '+(44) 7700-900 855', + 'template': str(sample_template.id) + } + + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + response_data = json.loads(response.data)['data'] + notification_id = response_data['notification']['id'] + + mocked.assert_called_once_with([notification_id], queue='send-sms') + assert response.status_code == 201 + assert notification_id + notifications = Notification.query.all() + assert len(notifications) == 1 + assert '+(44) 7700-900 855' == notifications[0].to + + +def test_should_not_allow_international_number_on_sms_notification(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': '20-12-1234-1234', + 'template': str(sample_template.id) + } + + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert not mocked.called + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['result'] == 'error' + assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' + + +def test_should_allow_international_number_on_sms_notification(notify_api, notify_db, notify_db_session, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) + template = create_sample_template(notify_db, notify_db_session, service=service) + + data = { + 'to': '20-12-1234-1234', + 'template': str(template.id) + } + + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 951a43280..c663860b6 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -2,12 +2,14 @@ import pytest from freezegun import freeze_time import app +from app.models import KEY_TYPE_NORMAL from app.notifications.validators import ( check_service_message_limit, check_template_is_for_notification_type, check_template_is_active, service_can_send_to_recipient, - check_sms_content_char_count + check_sms_content_char_count, + validate_and_format_recipient ) from app.v2.errors import ( BadRequestError, @@ -262,3 +264,19 @@ def test_check_sms_content_char_count_fails(char_count, notify_api): assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( notify_api.config['SMS_CHAR_COUNT_LIMIT']) assert e.value.fields == [] + + +@pytest.mark.parametrize('key_type', ['test', 'normal']) +def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms(sample_service, key_type): + with pytest.raises(BadRequestError) as e: + validate_and_format_recipient('20-12-1234-1234', key_type, sample_service, 'sms') + assert e.value.status_code == 400 + assert e.value.message == 'Cannot send to international mobile numbers' + assert e.value.fields == [] + + +@pytest.mark.parametrize('key_type', ['test', 'normal']) +def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_sms(sample_service, key_type): + sample_service.can_send_international_sms = True + result = validate_and_format_recipient('20-12-1234-1234', key_type, sample_service, 'sms') + assert result == '201212341234' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index bf15dd8e3..23bc9ecb1 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -3,7 +3,7 @@ import pytest from flask import json from app.models import Notification from tests import create_authorization_header -from tests.app.conftest import sample_template as create_sample_template +from tests.app.conftest import sample_template as create_sample_template, sample_service @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -224,3 +224,71 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori assert response.status_code == 201 mocked.assert_called_once_with([notification_id], queue='priority') + + +def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(client, sample_service, sample_template): + data = { + 'phone_number': '20-12-1234-1234', + 'template_id': sample_template.id + } + auth_header = create_authorization_header(service_id=sample_service.id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + assert response.headers['Content-type'] == 'application/json' + + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['status_code'] == 400 + assert error_json['errors'] == [ + {"error": "BadRequestError", "message": 'Cannot send to international mobile numbers'} + ] + + +def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client): + + service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) + template = create_sample_template(notify_db, notify_db_session, service=service) + + data = { + 'phone_number': '20-12-1234-1234', + 'template_id': template.id + } + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + assert response.headers['Content-type'] == 'application/json' + + +def test_post_sms_should_persist_supplied_sms_number(notify_api, sample_template_with_placeholders, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+(44) 77009-00855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'} + } + + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert '+(44) 77009-00855' == notifications[0].to + assert resp_json['id'] == str(notification_id) + assert mocked.called From 05145afcecb7ac7a455e9bae19ffbbbc2100c0a0 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 26 Apr 2017 17:26:06 +0100 Subject: [PATCH 2/8] Fix tests for checking simulated recipients --- .../test_process_notification.py | 59 ++++++++++++------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 9e428a0ab..b581f79a5 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -9,10 +9,13 @@ from collections import namedtuple from app.models import Template, Notification, NotificationHistory from app.notifications import SendNotificationToQueueError -from app.notifications.process_notifications import (create_content_for_notification, - persist_notification, - send_notification_to_queue, - simulated_recipient) +from app.notifications.process_notifications import ( + create_content_for_notification, + persist_notification, + send_notification_to_queue, + simulated_recipient +) +from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError from tests.app.conftest import sample_api_key as create_api_key @@ -255,22 +258,36 @@ def test_send_notification_to_queue_throws_exception_deletes_notification(sample assert NotificationHistory.query.count() == 0 -@pytest.mark.parametrize("to_address, notification_type, expected", - [("+447700900000", "sms", True), - ("+447700900111", "sms", True), - ("+447700900222", "sms", True), - ("simulate-delivered@notifications.service.gov.uk", "email", True), - ("simulate-delivered-2@notifications.service.gov.uk", "email", True), - ("simulate-delivered-3@notifications.service.gov.uk", "email", True), - ("07515896969", "sms", False), - ("valid_email@test.com", "email", False)]) +@pytest.mark.parametrize("to_address, notification_type, expected", [ + ("+447700900000", "sms", True), + ("+447700900111", "sms", True), + ("+447700900222", "sms", True), + ("07700900000", "sms", True), + ("7700900111", "sms", True), + ("simulate-delivered@notifications.service.gov.uk", "email", True), + ("simulate-delivered-2@notifications.service.gov.uk", "email", True), + ("simulate-delivered-3@notifications.service.gov.uk", "email", True), + ("07515896969", "sms", False), + ("valid_email@test.com", "email", False) +]) def test_simulated_recipient(notify_api, to_address, notification_type, expected): - # The values where the expected = 'research-mode' are listed in the config['SIMULATED_EMAIL_ADDRESSES'] - # and config['SIMULATED_SMS_NUMBERS']. These values should result in using the research mode queue. - # SIMULATED_EMAIL_ADDRESSES = ('simulate-delivered@notifications.service.gov.uk', - # 'simulate-delivered-2@notifications.service.gov.uk', - # 'simulate-delivered-2@notifications.service.gov.uk') - # SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') + """ + The values where the expected = 'research-mode' are listed in the config['SIMULATED_EMAIL_ADDRESSES'] + and config['SIMULATED_SMS_NUMBERS']. These values should result in using the research mode queue. + SIMULATED_EMAIL_ADDRESSES = ( + 'simulate-delivered@notifications.service.gov.uk', + 'simulate-delivered-2@notifications.service.gov.uk', + 'simulate-delivered-2@notifications.service.gov.uk' + ) + SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') + """ + formatted_address = None - actual = simulated_recipient(to_address, notification_type) - assert actual == expected + if notification_type == 'email': + formatted_address = validate_and_format_email_address(to_address) + else: + formatted_address = validate_and_format_phone_number(to_address) + + is_simulated_address = simulated_recipient(formatted_address, notification_type) + + assert is_simulated_address == expected From 2a1c3c248fab20f69e96b5454512b6601e9683ff Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 26 Apr 2017 21:21:00 +0100 Subject: [PATCH 3/8] Bumped utils --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index c90f21848..7994f02b8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@16.1.0#egg=notifications-utils==16.1.0 +git+https://github.com/alphagov/notifications-utils.git@16.1.1#egg=notifications-utils==16.1.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 86a5445fb60f161ba32c044ecff9cbb0e72cb074 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 09:26:12 +0100 Subject: [PATCH 4/8] Bumped utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7994f02b8..a757b27b6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@16.1.1#egg=notifications-utils==16.1.1 +git+https://github.com/alphagov/notifications-utils.git@16.1.2#egg=notifications-utils==16.1.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 56a82bb593a925a92b8f6b879a3fcd7ddc73d515 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 09:45:36 +0100 Subject: [PATCH 5/8] Bumped utils version --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index a757b27b6..3bc64fbc2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@16.1.2#egg=notifications-utils==16.1.2 +git+https://github.com/alphagov/notifications-utils.git@16.1.3#egg=notifications-utils==16.1.3 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 83dc7c2bb7af8832a21ec397ae7d984d7c833870 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 09:58:37 +0100 Subject: [PATCH 6/8] Little test updates --- .../rest/test_send_notification.py | 110 +++++++++--------- 1 file changed, 52 insertions(+), 58 deletions(-) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 8fe9f09b3..dbdea24b9 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1048,79 +1048,73 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori mocked.assert_called_once_with([notification_id], queue='priority') -def test_should_allow_store_original_number_on_sms_notification(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +def test_should_allow_store_original_number_on_sms_notification(client, sample_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - data = { - 'to': '+(44) 7700-900 855', - 'template': str(sample_template.id) - } + data = { + 'to': '+(44) 7700-900 855', + 'template': str(sample_template.id) + } - auth_header = create_authorization_header(service_id=sample_template.service_id) + auth_header = create_authorization_header(service_id=sample_template.service_id) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - response_data = json.loads(response.data)['data'] - notification_id = response_data['notification']['id'] + response_data = json.loads(response.data)['data'] + notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-sms') - assert response.status_code == 201 - assert notification_id - notifications = Notification.query.all() - assert len(notifications) == 1 - assert '+(44) 7700-900 855' == notifications[0].to + mocked.assert_called_once_with([notification_id], queue='send-sms') + assert response.status_code == 201 + assert notification_id + notifications = Notification.query.all() + assert len(notifications) == 1 + assert '+(44) 7700-900 855' == notifications[0].to -def test_should_not_allow_international_number_on_sms_notification(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +def test_should_not_allow_international_number_on_sms_notification(client, sample_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - data = { - 'to': '20-12-1234-1234', - 'template': str(sample_template.id) - } + data = { + 'to': '20-12-1234-1234', + 'template': str(sample_template.id) + } - auth_header = create_authorization_header(service_id=sample_template.service_id) + auth_header = create_authorization_header(service_id=sample_template.service_id) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - assert not mocked.called - assert response.status_code == 400 - error_json = json.loads(response.get_data(as_text=True)) - assert error_json['result'] == 'error' - assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' + assert not mocked.called + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['result'] == 'error' + assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' -def test_should_allow_international_number_on_sms_notification(notify_api, notify_db, notify_db_session, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") +def test_should_allow_international_number_on_sms_notification(client, notify_db, notify_db_session, mocker): + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) - template = create_sample_template(notify_db, notify_db_session, service=service) + service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) + template = create_sample_template(notify_db, notify_db_session, service=service) - data = { - 'to': '20-12-1234-1234', - 'template': str(template.id) - } + data = { + 'to': '20-12-1234-1234', + 'template': str(template.id) + } - auth_header = create_authorization_header(service_id=service.id) + auth_header = create_authorization_header(service_id=service.id) - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 201 + assert response.status_code == 201 From 84860b2a1de02febac162bb8820215fe0f559161 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 10:41:31 +0100 Subject: [PATCH 7/8] Adds a debug line to try and debug jenkins --- tests/app/v2/notifications/test_post_notifications.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 23bc9ecb1..42a53192e 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -264,6 +264,7 @@ def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) + print(json.loads(response.get_data(as_text=True))) assert response.status_code == 201 assert response.headers['Content-type'] == 'application/json' From 99081488f14b3f78cef3bfd89394b295a8651362 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:47:08 +0100 Subject: [PATCH 8/8] Mock out some SQS calls --- tests/app/v2/notifications/test_post_notifications.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 42a53192e..7a0b026c5 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -200,6 +200,9 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori notification_type, key_send_to, send_to): + + mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) + sample = create_sample_template( notify_db, notify_db_session, @@ -248,11 +251,13 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(client ] -def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client): +def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client, mocker): service = sample_service(notify_db, notify_db_session, can_send_international_sms=True) template = create_sample_template(notify_db, notify_db_session, service=service) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { 'phone_number': '20-12-1234-1234', 'template_id': template.id