From 349fb3529e6d0d8b7d251c81a8f4f993843e3d83 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 27 Apr 2017 13:22:17 +0100 Subject: [PATCH 1/4] Handle case for international SMS - use correct phone validator, and also set status correctly. This relies on some other code so this commit has placeholder failing tests to be populated when other PRs are merged. --- app/dao/notifications_dao.py | 4 ++-- app/delivery/send_to_providers.py | 14 +++++++++----- tests/app/dao/test_notification_dao.py | 8 ++++++++ tests/app/delivery/test_send_to_providers.py | 9 +++++++++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 29632e69d..6a759f261 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -26,8 +26,8 @@ from app.models import ( NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, - LETTER_TYPE -) + LETTER_TYPE, + NOTIFICATION_SENT) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 6634daa32..4eb2f61e9 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -15,7 +15,8 @@ from app.dao.provider_details_dao import ( ) from app.celery.research_mode_tasks import send_sms_response, send_email_response from app.dao.templates_dao import dao_get_template_by_id -from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE +from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE, \ + NOTIFICATION_SENT, NOTIFICATION_SENDING def send_sms_to_provider(notification): @@ -44,7 +45,7 @@ def send_sms_to_provider(notification): else: try: provider.send_sms( - to=validate_and_format_phone_number(notification.to), + to=validate_and_format_phone_number(notification.to, internationl=notification.international), content=str(template), reference=str(notification.id), sender=service.sms_sender @@ -54,7 +55,7 @@ def send_sms_to_provider(notification): raise e else: notification.billable_units = template.fragment_count - update_notification(notification, provider) + update_notification(notification, provider, notification.international) current_app.logger.info( "SMS {} sent to provider {} at {}".format(notification.id, provider.get_name(), notification.sent_at) @@ -113,10 +114,13 @@ def send_email_to_provider(notification): statsd_client.timing("email.total-time", delta_milliseconds) -def update_notification(notification, provider): +def update_notification(notification, provider, international=False): notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name() - notification.status = 'sending' + if international: + notification.status = NOTIFICATION_SENT + else: + notification.status = NOTIFICATION_SENDING dao_update_notification(notification) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 99d1200ec..e43f5f5de 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -352,6 +352,14 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): assert updated.status == 'failed' +def test_should_not_update_status_by_reference_if_in_sent_status(notify_db, notify_db_session): + assert 1 == 2 + + +def test_should_not_update_status_by_id_if_in_sent_status(notify_db, notify_db_session): + assert 1 == 2 + + def test_should_not_update_status_by_reference_if_not_sending(notify_db, notify_db_session): notification = sample_notification(notify_db, notify_db_session, status='created', reference='reference') assert Notification.query.get(notification.id).status == 'created' diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 063d86e71..3965c2af3 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -474,6 +474,7 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(no assert sample_notification.billable_units == billable_units +<<<<<<< HEAD def test_should_send_sms_to_international_providers( restore_provider_details, sample_sms_template_with_html, @@ -530,3 +531,11 @@ def test_should_send_sms_to_international_providers( assert notification_uk.sent_by == 'firetext' assert notification_int.status == 'sending' assert notification_int.sent_by == 'mmg' + + +def test_should_send_international_sms_with_formatted_phone_number(): + assert 1 == 2 + + +def test_should_set_international_phone_number_to_sent_status(): + assert 1 == 2 From c5bd685cef12d7c296b865c23ee249ab47e01bb5 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 16:55:39 +0100 Subject: [PATCH 2/4] Don't update sent notifications (dao) --- app/dao/notifications_dao.py | 18 ++++++++++++------ tests/app/dao/test_notification_dao.py | 21 +++++++++++++++++++-- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6a759f261..377b47e3c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -175,11 +175,14 @@ def _update_notification_status(notification, status): def update_notification_status_by_id(notification_id, status): notification = Notification.query.with_lockmode("update").filter( Notification.id == notification_id, - or_(Notification.status == NOTIFICATION_CREATED, + or_( + Notification.status == NOTIFICATION_CREATED, Notification.status == NOTIFICATION_SENDING, - Notification.status == NOTIFICATION_PENDING)).first() + Notification.status == NOTIFICATION_PENDING, + Notification.status == NOTIFICATION_SENT + )).first() - if not notification: + if not notification or notification.status == NOTIFICATION_SENT: return None return _update_notification_status( @@ -193,10 +196,13 @@ def update_notification_status_by_id(notification_id, status): def update_notification_status_by_reference(reference, status): notification = Notification.query.filter( Notification.reference == reference, - or_(Notification.status == NOTIFICATION_SENDING, - Notification.status == NOTIFICATION_PENDING)).first() + or_( + Notification.status == NOTIFICATION_SENDING, + Notification.status == NOTIFICATION_PENDING, + Notification.status == NOTIFICATION_SENT + )).first() - if not notification: + if not notification or notification.status == NOTIFICATION_SENT: return None return _update_notification_status( diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index e43f5f5de..8f67d2335 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -15,6 +15,7 @@ from app.models import ( TemplateStatistics, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, + NOTIFICATION_SENT, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST @@ -353,11 +354,27 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): def test_should_not_update_status_by_reference_if_in_sent_status(notify_db, notify_db_session): - assert 1 == 2 + notification = sample_notification( + notify_db, + notify_db_session, + status=NOTIFICATION_SENT, + reference='foo' + ) + + update_notification_status_by_reference('foo', 'failed') + assert Notification.query.get(notification.id).status == NOTIFICATION_SENT def test_should_not_update_status_by_id_if_in_sent_status(notify_db, notify_db_session): - assert 1 == 2 + notification = sample_notification( + notify_db, + notify_db_session, + status=NOTIFICATION_SENT + ) + + update_notification_status_by_id(notification.id, 'failed') + + assert Notification.query.get(notification.id).status == NOTIFICATION_SENT def test_should_not_update_status_by_reference_if_not_sending(notify_db, notify_db_session): From e450f15b2b7b1cbe3d739920e7c5b329e7bc03ed Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 16:57:04 +0100 Subject: [PATCH 3/4] Fix typo and update test now that we expect sent for international --- app/delivery/send_to_providers.py | 2 +- tests/app/delivery/test_send_to_providers.py | 45 +++++++++++++++++--- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 4eb2f61e9..f9ca7861f 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -45,7 +45,7 @@ def send_sms_to_provider(notification): else: try: provider.send_sms( - to=validate_and_format_phone_number(notification.to, internationl=notification.international), + to=validate_and_format_phone_number(notification.to, international=notification.international), content=str(template), reference=str(notification.id), sender=service.sms_sender diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 3965c2af3..0e35ad413 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -474,7 +474,6 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type(no assert sample_notification.billable_units == billable_units -<<<<<<< HEAD def test_should_send_sms_to_international_providers( restore_provider_details, sample_sms_template_with_html, @@ -529,13 +528,47 @@ def test_should_send_sms_to_international_providers( assert notification_uk.status == 'sending' assert notification_uk.sent_by == 'firetext' - assert notification_int.status == 'sending' + assert notification_int.status == 'sent' assert notification_int.sent_by == 'mmg' -def test_should_send_international_sms_with_formatted_phone_number(): - assert 1 == 2 +def test_should_send_international_sms_with_formatted_phone_number( + notify_db, + sample_template, + mocker +): + notification = create_notification( + template=sample_template, + to_field="+6011-17224412", + international=True + ) + + send_notification_mock = mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.send_sms_response') + + send_to_providers.send_sms_to_provider( + notification + ) + + assert send_notification_mock.called is True -def test_should_set_international_phone_number_to_sent_status(): - assert 1 == 2 +def test_should_set_international_phone_number_to_sent_status( + notify_db, + sample_template, + mocker +): + notification = create_notification( + template=sample_template, + to_field="+6011-17224412", + international=True + ) + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.send_sms_response') + + send_to_providers.send_sms_to_provider( + notification + ) + + assert notification.status == 'sent' From b5d4acb75853a680d2d1052088b9ad28b8a80e62 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 27 Apr 2017 16:58:00 +0100 Subject: [PATCH 4/4] Make message more accurate --- app/celery/provider_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index b0df268b3..e00591f09 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -44,7 +44,7 @@ def deliver_sms(self, notification_id): except Exception as e: try: current_app.logger.exception( - "RETRY: SMS notification {} failed".format(notification_id) + "SMS notification delivery for id: {} failed".format(notification_id) ) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: