From 252c3235d71a1c9e48c4a75af9b480c4ddfb3844 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 12:41:10 +0100 Subject: [PATCH 1/4] Persist intl fields on notification --- app/notifications/process_notifications.py | 48 ++++++++----- .../test_process_notification.py | 67 ++++++++++++++++++- 2 files changed, 95 insertions(+), 20 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 033943511..e8510ba63 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -2,9 +2,13 @@ from datetime import datetime from flask import current_app +from notifications_utils.recipients import ( + get_international_phone_info, + validate_and_format_phone_number +) + 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 @@ -25,22 +29,23 @@ def check_placeholders(template_object): raise BadRequestError(fields=[{'template': message}], message=message) -def persist_notification(template_id, - template_version, - recipient, - service, - personalisation, - notification_type, - api_key_id, - key_type, - created_at=None, - job_id=None, - job_row_number=None, - reference=None, - client_reference=None, - notification_id=None, - simulated=False): - # if simulated create a Notification model to return but do not persist the Notification to the dB +def persist_notification( + template_id, + template_version, + recipient, + service, + personalisation, + notification_type, + api_key_id, + key_type, + created_at=None, + job_id=None, + job_row_number=None, + reference=None, + client_reference=None, + notification_id=None, + simulated=False +): notification = Notification( id=notification_id, template_id=template_id, @@ -58,6 +63,15 @@ def persist_notification(template_id, client_reference=client_reference, reference=reference ) + + if notification_type == SMS_TYPE: + formatted_recipient = validate_and_format_phone_number(recipient, international=True) + recipient_info = get_international_phone_info(formatted_recipient) + notification.international = recipient_info.international + notification.phone_prefix = recipient_info.country_prefix + notification.rate_multiplier = recipient_info.billable_units + + # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: dao_create_notification(notification) if key_type != KEY_TYPE_TEST: diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 766e3f250..f697047e7 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,7 +1,7 @@ import datetime import uuid - import pytest + from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time @@ -199,8 +199,8 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None assert persisted_notification.international is False - assert persisted_notification.phone_prefix is None - assert persisted_notification.rate_multiplier is None + assert persisted_notification.phone_prefix == '44' + assert persisted_notification.rate_multiplier == 1 @freeze_time("2016-01-01 11:09:00.061258") @@ -297,3 +297,64 @@ def test_simulated_recipient(notify_api, to_address, notification_type, expected is_simulated_address = simulated_recipient(formatted_address, notification_type) assert is_simulated_address == expected + + +@pytest.mark.parametrize('recipient, expected_international, expected_prefix, expected_units', [ + ('7900900123', False, '44', 1), # UK + ('+447900900123', False, '44', 1), # UK + ('07700900222', False, '44', 1), # UK + ('73122345678', True, '7', 1), # Russia + ('360623400400', True, '36', 3)] # Hungary +) +def test_persist_notification_with_international_info_stores_correct_info( + sample_job, + sample_api_key, + mocker, + recipient, + expected_international, + expected_prefix, + expected_units +): + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient=recipient, + service=sample_job.service, + personalisation=None, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + job_id=sample_job.id, + job_row_number=10, + client_reference="ref from client" + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.international is expected_international + assert persisted_notification.phone_prefix == expected_prefix + assert persisted_notification.rate_multiplier == expected_units + + +def test_persist_notification_with_international_info_does_not_store_for_email( + sample_job, + sample_api_key, + mocker +): + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient='foo@bar.com', + service=sample_job.service, + personalisation=None, + notification_type='email', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + job_id=sample_job.id, + job_row_number=10, + client_reference="ref from client" + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.international is False + assert persisted_notification.phone_prefix is None + assert persisted_notification.rate_multiplier is None From 868e102f8a59047730cf56a4d4cc7ca1e8a26d80 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 12:42:00 +0100 Subject: [PATCH 2/4] Make rate_multiplier on notification return non-decimal --- app/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 44487c334..56979985f 100644 --- a/app/models.py +++ b/app/models.py @@ -670,7 +670,7 @@ class Notification(db.Model): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Numeric(), nullable=True) + rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) @property def personalisation(self): @@ -847,7 +847,7 @@ class NotificationHistory(db.Model, HistoryModel): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Numeric(), nullable=True) + rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) @classmethod def from_original(cls, notification): From 3de93cbcd0eabe309156954691786231ccd7aaa2 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 12:42:45 +0100 Subject: [PATCH 3/4] Assume that we will only receive a UK number: * We currently don't validate the number so this test * will fail assuming an invalid number was passed. * Since we do validation on the front end, for now * we'll assume a valid number. This does need to be * looked at in future. --- tests/app/user/test_rest_verify.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 1e08115a9..08f510bbd 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -230,7 +230,7 @@ def test_send_user_code_for_sms_with_optional_to_field(client, """ Tests POST endpoint /user//sms-code with optional to field """ - to_number = '+441119876757' + to_number = '+447119876757' mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') auth_header = create_authorization_header() From 164ba0c6b31d2829907dc553af575b5199c4e798 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 13:50:51 +0100 Subject: [PATCH 4/4] as_decimal defaults to False --- app/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 56979985f..82d9efb43 100644 --- a/app/models.py +++ b/app/models.py @@ -670,7 +670,7 @@ class Notification(db.Model): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) + rate_multiplier = db.Column(db.Float(), nullable=True) @property def personalisation(self): @@ -847,7 +847,7 @@ class NotificationHistory(db.Model, HistoryModel): international = db.Column(db.Boolean, nullable=False, default=False) phone_prefix = db.Column(db.String, nullable=True) - rate_multiplier = db.Column(db.Float(as_decimal=False), nullable=True) + rate_multiplier = db.Column(db.Float(), nullable=True) @classmethod def from_original(cls, notification):