From d0978e52fb6e0d6aa399aed5cd43b358c56f01b9 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 10:18:28 +0100 Subject: [PATCH 1/5] Use intl provider for int sms notifications (needs tests) --- app/delivery/send_to_providers.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index f6991c0b0..7ff67b50e 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -25,7 +25,7 @@ def send_sms_to_provider(notification): return if notification.status == 'created': - provider = provider_to_use(SMS_TYPE, notification.id) + provider = provider_to_use(SMS_TYPE, notification.id, notification.international) current_app.logger.info( "Starting sending SMS {} to provider at {}".format(notification.id, datetime.utcnow()) ) @@ -120,18 +120,22 @@ def update_notification(notification, provider): dao_update_notification(notification) -def provider_to_use(notification_type, notification_id): - active_providers_in_order = [ - provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active - ] +def provider_to_use(notification_type, notification_id, international=False): + provider = None + # Default to MMG for international SMS + if notification_type == SMS_TYPE: + provider_id = 'mmg' if international else get_current_provider('sms') + provider = get_provider_details_by_identifier('mmg') + elif notification_type == EMAIL_TYPE: + provider = get_current_provider(notification_type) - if not active_providers_in_order: + if not provider: current_app.logger.error( "{} {} failed as no active providers".format(notification_type, notification_id) ) raise Exception("No active {} providers".format(notification_type)) - return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + return clients.get_client_by_name_and_type(provider.identifier, notification_type) def get_logo_url(base_url, branding_path, logo_file): From ba58c55c3b756e449c44265bee9a8d22808fb1c7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:13:58 +0100 Subject: [PATCH 2/5] Filter provider details by international flags --- app/dao/provider_details_dao.py | 15 ++++++++++----- tests/app/dao/test_provider_details_dao.py | 14 +++++++++++--- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index ff4db1a93..1e2c11727 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -60,6 +60,7 @@ def dao_toggle_sms_provider(identifier): @transactional def dao_switch_sms_provider_to_provider_with_identifier(identifier): new_provider = get_provider_details_by_identifier(identifier) + if provider_is_inactive(new_provider): return @@ -69,7 +70,7 @@ def dao_switch_sms_provider_to_provider_with_identifier(identifier): providers_to_update = [] if conflicting_provider: - providers_to_update = switch_providers(conflicting_provider, new_provider) + switch_providers(conflicting_provider, new_provider) else: current_provider = get_current_provider('sms') if not provider_is_primary(current_provider, new_provider, identifier): @@ -79,10 +80,14 @@ def dao_switch_sms_provider_to_provider_with_identifier(identifier): dao_update_provider_details(provider) -def get_provider_details_by_notification_type(notification_type): - return ProviderDetails.query.filter_by( - notification_type=notification_type - ).order_by(asc(ProviderDetails.priority)).all() +def get_provider_details_by_notification_type(notification_type, supports_international=False): + + filters = [ProviderDetails.notification_type == notification_type] + + if supports_international: + filters.append(ProviderDetails.supports_international == supports_international) + + return ProviderDetails.query.filter(*filters).order_by(asc(ProviderDetails.priority)).all() @transactional diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 5092bd1b9..21f74e3d9 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -35,16 +35,24 @@ def test_can_get_all_providers(restore_provider_details): assert len(get_provider_details()) == 5 -def test_can_get_sms_providers(restore_provider_details): +def test_can_get_sms_non_international_providers(restore_provider_details): sms_providers = get_provider_details_by_notification_type('sms') assert len(sms_providers) == 3 assert all('sms' == prov.notification_type for prov in sms_providers) + assert all(not prov.supports_international for prov in sms_providers) + + +def test_can_get_sms_international_providers(restore_provider_details): + sms_providers = get_provider_details_by_notification_type('sms', True) + assert len(sms_providers) == 1 + assert all('sms' == prov.notification_type for prov in sms_providers) + assert all(prov.supports_international for prov in sms_providers) def test_can_get_sms_providers_in_order_of_priority(restore_provider_details): - providers = get_provider_details_by_notification_type('sms') + providers = get_provider_details_by_notification_type('sms', False) - assert providers[0].priority < providers[1].priority < providers[2].priority + assert providers[0].priority < providers[1].priority def test_can_get_email_providers_in_order_of_priority(restore_provider_details): From 109b1727f2276d4fcfe406fe2ea7af451a4b6236 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:14:22 +0100 Subject: [PATCH 3/5] Fish out the international provider if needed. --- app/delivery/send_to_providers.py | 21 +++---- tests/app/delivery/test_send_to_providers.py | 63 +++++++++++++++++++- 2 files changed, 68 insertions(+), 16 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 7ff67b50e..6634daa32 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -121,21 +121,17 @@ def update_notification(notification, provider): def provider_to_use(notification_type, notification_id, international=False): - provider = None - # Default to MMG for international SMS - if notification_type == SMS_TYPE: - provider_id = 'mmg' if international else get_current_provider('sms') - provider = get_provider_details_by_identifier('mmg') - elif notification_type == EMAIL_TYPE: - provider = get_current_provider(notification_type) + active_providers_in_order = [ + p for p in get_provider_details_by_notification_type(notification_type, international) if p.active + ] - if not provider: + if not active_providers_in_order: current_app.logger.error( "{} {} failed as no active providers".format(notification_type, notification_id) ) raise Exception("No active {} providers".format(notification_type)) - return clients.get_client_by_name_and_type(provider.identifier, notification_type) + return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) def get_logo_url(base_url, branding_path, logo_file): @@ -151,11 +147,8 @@ def get_logo_url(base_url, branding_path, logo_file): base_url = parse.urlparse(base_url) netloc = base_url.netloc - if ( - base_url.netloc.startswith('localhost') or - # covers both preview and staging - 'notify.works' in base_url.netloc - ): + # covers both preview and staging + if base_url.netloc.startswith('localhost') or 'notify.works' in base_url.netloc: path = '/static' + branding_path + logo_file else: if base_url.netloc.startswith('www'): diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 2894d75e9..cbd384844 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -7,8 +7,9 @@ import pytest from notifications_utils.recipients import validate_phone_number, format_phone_number import app -from app import mmg_client +from app import mmg_client, firetext_client from app.dao import (provider_details_dao, notifications_dao) +from app.dao.provider_details_dao import dao_switch_sms_provider_to_provider_with_identifier from app.delivery import send_to_providers from app.models import ( Notification, @@ -18,7 +19,7 @@ from app.models import ( KEY_TYPE_TEAM, BRANDING_ORG, BRANDING_BOTH, -) + ProviderDetails) from tests.app.db import create_service, create_template, create_notification @@ -471,3 +472,61 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(no ) assert sample_notification.billable_units == billable_units + + +def test_should_send_sms_to_international_providers( + restore_provider_details, + sample_sms_template_with_html, + sample_user, + mocker +): + mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) + + dao_switch_sms_provider_to_provider_with_identifier('firetext') + + db_notification_uk = create_notification( + template=sample_sms_template_with_html, + to_field="+447234123999", + personalisation={"name": "Jo"}, + status='created', + international=False) + + db_notification_international = create_notification( + template=sample_sms_template_with_html, + to_field="+447234123111", + personalisation={"name": "Jo"}, + status='created', + international=True) + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.firetext_client.send_sms') + + send_to_providers.send_sms_to_provider( + db_notification_uk + ) + + firetext_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123999")), + content=ANY, + reference=str(db_notification_uk.id), + sender=None + ) + + send_to_providers.send_sms_to_provider( + db_notification_international + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123111")), + content=ANY, + reference=str(db_notification_international.id), + sender=None + ) + + notification_uk = Notification.query.filter_by(id=db_notification_uk.id).one() + notification_int = Notification.query.filter_by(id=db_notification_international.id).one() + + assert notification_uk.status == 'sending' + assert notification_uk.sent_by == 'firetext' + assert notification_int.status == 'sending' + assert notification_int.sent_by == 'mmg' From cdd3ad687c07a2cb3b2d5385466107273f418b84 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:14:31 +0100 Subject: [PATCH 4/5] updated the fixture --- tests/app/db.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/app/db.py b/tests/app/db.py index 195a77ea7..38d5eca9d 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -72,7 +72,8 @@ def create_notification( api_key_id=None, key_type=KEY_TYPE_NORMAL, sent_by=None, - client_reference=None + client_reference=None, + international=False ): if created_at is None: created_at = datetime.utcnow() @@ -103,7 +104,8 @@ def create_notification( 'sent_by': sent_by, 'updated_at': updated_at, 'client_reference': client_reference, - 'job_row_number': job_row_number + 'job_row_number': job_row_number, + 'international': international } notification = Notification(**data) dao_create_notification(notification) From bedbd8e21fff019dec3f72c9dc4cdd43132faeab Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 12:15:47 +0100 Subject: [PATCH 5/5] Removed unneeded test case --- tests/app/dao/test_provider_details_dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 21f74e3d9..ed3f0e1f0 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -39,7 +39,6 @@ def test_can_get_sms_non_international_providers(restore_provider_details): sms_providers = get_provider_details_by_notification_type('sms') assert len(sms_providers) == 3 assert all('sms' == prov.notification_type for prov in sms_providers) - assert all(not prov.supports_international for prov in sms_providers) def test_can_get_sms_international_providers(restore_provider_details):