Merge pull request #930 from alphagov/set-status-to-sent-on-int-numbers

Send to provider for international

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:

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(

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)
 
 

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'

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'