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/app/schemas.py b/app/schemas.py index 40cb7ff08..b3aa0ac49 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -6,10 +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 - -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, + format_phone_number +) # TODO I think marshmallow provides a better integration and error handling. @@ -128,8 +129,17 @@ 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)) + + @post_load + def format_phone_number(self, item): + item['to'] = format_phone_number(validate_phone_number( + item['to']) + ) + return item class EmailNotificationSchema(NotificationSchema): @@ -138,7 +148,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 +186,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/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 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..5d8ed0b73 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': '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,