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: diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 29632e69d..377b47e3c 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 @@ -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/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 6634daa32..f9ca7861f 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, international=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..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 @@ -352,6 +353,30 @@ 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): + 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): + 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): 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..0e35ad413 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -528,5 +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( + 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( + 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'