From 7cb84508397fc6e3ca6b5d7cfcb928c742c3422e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 6 Mar 2016 12:51:45 +0000 Subject: [PATCH 1/3] Use `RecipientCSV` from utils for processing CSVs See https://github.com/alphagov/notifications-utils/pull/9 for details of the changes. --- app/celery/tasks.py | 13 +++++++------ requirements.txt | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d4388a08c..6f1a17f9b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -12,7 +12,7 @@ from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 from datetime import datetime from utils.template import Template -from utils.process_csv import get_rows_from_csv, get_recipient_from_row, first_column_heading +from utils.recipients import RecipientCSV, first_column_heading @notify_celery.task(name="process-job") @@ -22,15 +22,16 @@ def process_job(job_id): job.status = 'in progress' dao_update_job(job) - file = s3.get_job_from_s3(job.bucket_name, job_id) - - for row in get_rows_from_csv(file): + for recipient, personalisation in RecipientCSV( + s3.get_job_from_s3(job.bucket_name, job_id), + template_type=job.template.template_type + ).recipients_and_personalisation: encrypted = encryption.encrypt({ 'template': job.template_id, 'job': str(job.id), - 'to': get_recipient_from_row(row, job.template.template_type), - 'personalisation': row + 'to': recipient, + 'personalisation': personalisation }) if job.template.template_type == 'sms': diff --git a/requirements.txt b/requirements.txt index 254a2f3a5..43f900f98 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,4 +21,4 @@ monotonic==0.3 git+https://github.com/alphagov/notifications-python-client.git@0.2.6#egg=notifications-python-client==0.2.6 -git+https://github.com/alphagov/notifications-utils.git@1.0.0#egg=notifications-utils==1.0.0 +git+https://github.com/alphagov/notifications-utils.git@2.0.0#egg=notifications-utils==2.0.0 From 157b38532795b542db492acf26a55c428a8fee89 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 6 Mar 2016 12:58:28 +0000 Subject: [PATCH 2/3] Use validation of recipients from utils This was added to utils in https://github.com/alphagov/notifications-utils/commit/5914da74f1f948e2fbcd7597b7a26476de1e2eb3 This means that: - we are doing the exact same validation in the API and admin app - we are actually validating phone numbers for the correct format (hence all the changes to the tests) --- app/schemas.py | 19 +++++++++++-------- tests/app/conftest.py | 2 +- tests/app/notifications/test_rest.py | 18 +++++++++--------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 40cb7ff08..016cfadb7 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -6,10 +6,7 @@ from . import models from app.dao.permissions_dao import permission_dao from marshmallow import (post_load, ValidationError, validates, validates_schema) from marshmallow_sqlalchemy import field_for - -mobile_regex = re.compile("^\\+44[\\d]{10}$") - -email_regex = re.compile("(^[^@^\\s]+@[^@^\\.^\\s]+(\\.[^@^\\.^\\s]*)*\.(.+))") +from utils.recipients import validate_email_address, InvalidEmailError, validate_phone_number, InvalidPhoneError # TODO I think marshmallow provides a better integration and error handling. @@ -128,8 +125,10 @@ class SmsNotificationSchema(NotificationSchema): @validates('to') def validate_to(self, value): - if not mobile_regex.match(value): - raise ValidationError('Invalid phone number, must be of format +441234123123') + try: + validate_phone_number(value) + except InvalidPhoneError as error: + raise ValidationError('Invalid phone number: {}'.format(error)) class EmailNotificationSchema(NotificationSchema): @@ -138,7 +137,9 @@ class EmailNotificationSchema(NotificationSchema): @validates('to') def validate_to(self, value): - if not email_regex.match(value): + try: + validate_email_address(value) + except InvalidEmailError: raise ValidationError('Invalid email') @@ -174,7 +175,9 @@ class InvitedUserSchema(BaseSchema): @validates('email_address') def validate_to(self, value): - if not email_regex.match(value): + try: + validate_email_address(value) + except InvalidEmailError: raise ValidationError('Invalid email') diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d8ec894f0..679d12df8 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -277,7 +277,7 @@ def sample_notification(notify_db, if to_field: to = to_field else: - to = '+44709123456' + to = '+447700900855' data = { 'id': notification_id, diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 650cd85ec..967a74d39 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -24,7 +24,7 @@ def test_get_notification_by_id(notify_api, sample_notification): notification = json.loads(response.get_data(as_text=True))['notification'] assert notification['status'] == 'sent' assert notification['template'] == sample_notification.template.id - assert notification['to'] == '+44709123456' + assert notification['to'] == '+447700900855' assert notification['service'] == str(sample_notification.service_id) assert response.status_code == 200 @@ -63,7 +63,7 @@ def test_get_all_notifications(notify_api, sample_notification): notifications = json.loads(response.get_data(as_text=True)) assert notifications['notifications'][0]['status'] == 'sent' assert notifications['notifications'][0]['template'] == sample_notification.template.id - assert notifications['notifications'][0]['to'] == '+44709123456' + assert notifications['notifications'][0]['to'] == '+447700900855' assert notifications['notifications'][0]['service'] == str(sample_notification.service_id) assert response.status_code == 200 @@ -296,7 +296,7 @@ def test_should_reject_bad_phone_numbers(notify_api, sample_template, mocker): assert json_resp['result'] == 'error' assert len(json_resp['message'].keys()) == 1 - assert 'Invalid phone number, must be of format +441234123123' in json_resp['message']['to'] + assert 'Invalid phone number: Must not contain letters or symbols' in json_resp['message']['to'] assert response.status_code == 400 @@ -306,7 +306,7 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock mocker.patch('app.celery.tasks.send_sms.apply_async') data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': 9999 } auth_header = create_authorization_header( @@ -337,7 +337,7 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_templat mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template_with_placeholders.id, 'personalisation': { 'name': 'Jo' @@ -373,7 +373,7 @@ def test_send_notification_with_missing_personalisation( mocker.patch('app.celery.tasks.send_sms.apply_async') data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template_with_placeholders.id, 'personalisation': { 'foo': 'bar' @@ -405,7 +405,7 @@ def test_send_notification_with_too_much_personalisation_data( mocker.patch('app.celery.tasks.send_sms.apply_async') data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template_with_placeholders.id, 'personalisation': { 'name': 'Jo', 'foo': 'bar' @@ -439,7 +439,7 @@ def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample ).update( {'restricted': True} ) - invalid_mob = '+449999999999' + invalid_mob = '+447700900855' data = { 'to': invalid_mob, 'template': sample_template.id @@ -504,7 +504,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template.id } From 832375744185f35cdb7d739a49ccbaefa44ce6e6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 6 Mar 2016 13:21:46 +0000 Subject: [PATCH 3/3] Accept phone numbers in any valid format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This uses the `format_phone_number` method from utils to output phone numbers in a consistent format. It is added to the schemas, so will be applied before the API tries to do anything with a provided phone number. So now the API will accept any of the following: - 07123456789 - 07123 456789 - 07123-456-789 - 00447123456789 - 00 44 7123456789 - +447123456789 - +44 7123 456 789 - +44 (0)7123 456 789 …but the API will always hand off phone numbers to 3rd party APIs in the format - +447123456789 The test for this is slightly convoluted, because template IDs are still database IDs, and can’t consistently be mocked, therefore we have to ignore that part of the call to `encrypt()`. --- app/schemas.py | 13 ++++++++++++- tests/app/notifications/test_rest.py | 3 ++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 016cfadb7..b3aa0ac49 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -6,7 +6,11 @@ from . import models from app.dao.permissions_dao import permission_dao from marshmallow import (post_load, ValidationError, validates, validates_schema) from marshmallow_sqlalchemy import field_for -from utils.recipients import validate_email_address, InvalidEmailError, validate_phone_number, InvalidPhoneError +from utils.recipients import ( + validate_email_address, InvalidEmailError, + validate_phone_number, InvalidPhoneError, + format_phone_number +) # TODO I think marshmallow provides a better integration and error handling. @@ -130,6 +134,13 @@ class SmsNotificationSchema(NotificationSchema): except InvalidPhoneError as error: raise ValidationError('Invalid phone number: {}'.format(error)) + @post_load + def format_phone_number(self, item): + item['to'] = format_phone_number(validate_phone_number( + item['to']) + ) + return item + class EmailNotificationSchema(NotificationSchema): to = fields.Str(required=True) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 967a74d39..5d8ed0b73 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -504,7 +504,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { - 'to': '+447700900855', + 'to': '07700 900 855', 'template': sample_template.id } @@ -521,6 +521,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker headers=[('Content-Type', 'application/json'), auth_header]) notification_id = json.loads(response.data)['notification_id'] + assert app.encryption.encrypt.call_args[0][0]['to'] == '+447700900855' app.celery.tasks.send_sms.apply_async.assert_called_once_with( (str(sample_template.service_id), notification_id,