Merge pull request #2496 from alphagov/refactor-update-notifications

Refactor update notifications

app/delivery/send_to_providers.py

@@ -7,7 +7,6 @@ from notifications_utils.recipients import (
     validate_and_format_email_address
 )
 from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTemplate, SMSMessageTemplate
-from requests.exceptions import HTTPError
 
 from app import clients, statsd_client, create_uuid
 from app.dao.notifications_dao import (
@@ -26,7 +25,6 @@ from app.models import (
     BRANDING_BOTH,
     BRANDING_ORG_BANNER,
     EMAIL_TYPE,
-    NOTIFICATION_CREATED,
     NOTIFICATION_TECHNICAL_FAILURE,
     NOTIFICATION_SENT,
     NOTIFICATION_SENDING
@@ -42,9 +40,7 @@ def send_sms_to_provider(notification):
 
     if notification.status == 'created':
         provider = provider_to_use(SMS_TYPE, notification.id, notification.international)
-        current_app.logger.debug(
+
-            "Starting sending SMS {} to provider at {}".format(notification.id, datetime.utcnow())
-        )
         template_model = dao_get_template_by_id(notification.template_id, notification.template_version)
 
         template = SMSMessageTemplate(
@@ -55,18 +51,9 @@ def send_sms_to_provider(notification):
         )
 
         if service.research_mode or notification.key_type == KEY_TYPE_TEST:
-            notification.billable_units = 0
+            send_sms_response(provider.get_name(), str(notification.id), notification.to)
             update_notification_to_sending(notification, provider)
-            try:
+
-                send_sms_response(provider.get_name(), str(notification.id), notification.to)
-            except HTTPError:
-                # when we retry, we only do anything if the notification is in created - it's currently in sending,
-                # so set it back so that we actually attempt the callback again
-                notification.sent_at = None
-                notification.sent_by = None
-                notification.status = NOTIFICATION_CREATED
-                dao_update_notification(notification)
-                raise
         else:
             try:
                 provider.send_sms(
@@ -77,17 +64,13 @@ def send_sms_to_provider(notification):
                 )
             except Exception as e:
                 notification.billable_units = template.fragment_count
-                notification.sent_by = provider.get_name()
                 dao_update_notification(notification)
                 dao_toggle_sms_provider(provider.name)
                 raise e
             else:
                 notification.billable_units = template.fragment_count
-                update_notification_to_sending(notification, provider, notification.international)
+                update_notification_to_sending(notification, provider)
 
-        current_app.logger.debug(
-            "SMS {} sent to provider {} at {}".format(notification.id, provider.get_name(), notification.sent_at)
-        )
         delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
         statsd_client.timing("sms.total-time", delta_milliseconds)
 
@@ -99,9 +82,7 @@ def send_email_to_provider(notification):
         return
     if notification.status == 'created':
         provider = provider_to_use(EMAIL_TYPE, notification.id)
-        current_app.logger.debug(
+
-            "Starting sending EMAIL {} to provider at {}".format(notification.id, datetime.utcnow())
-        )
         template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__
 
         html_email = HTMLEmailTemplate(
@@ -116,11 +97,9 @@ def send_email_to_provider(notification):
         )
 
         if service.research_mode or notification.key_type == KEY_TYPE_TEST:
-            reference = str(create_uuid())
+            notification.reference = str(create_uuid())
-            notification.billable_units = 0
-            notification.reference = reference
             update_notification_to_sending(notification, provider)
-            send_email_response(reference, notification.to)
+            send_email_response(notification.reference, notification.to)
         else:
             from_address = '"{}" <{}@{}>'.format(service.name, service.email_from,
                                                  current_app.config['NOTIFY_EMAIL_DOMAIN'])
@@ -138,20 +117,14 @@ def send_email_to_provider(notification):
             notification.reference = reference
             update_notification_to_sending(notification, provider)
 
-        current_app.logger.debug(
-            "Email {} sent to provider at {}".format(notification.id, notification.sent_at)
-        )
         delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
         statsd_client.timing("email.total-time", delta_milliseconds)
 
 
-def update_notification_to_sending(notification, provider, international=False):
+def update_notification_to_sending(notification, provider):
     notification.sent_at = datetime.utcnow()
     notification.sent_by = provider.get_name()
-    if international:
+    notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING
-        notification.status = NOTIFICATION_SENT
-    else:
-        notification.status = NOTIFICATION_SENDING
     dao_update_notification(notification)
 
 

tests/app/delivery/test_send_to_providers.py

@@ -243,28 +243,6 @@ def test_should_leave_as_created_if_fake_callback_function_fails(sample_notifica
     assert sample_notification.sent_by is None
 
 
-@pytest.mark.parametrize('research_mode,key_type', [
-    (True, KEY_TYPE_NORMAL),
-    (False, KEY_TYPE_TEST)
-])
-def test_should_set_billable_units_to_zero_in_research_mode_or_test_key(
-        notify_db, sample_service, sample_notification, mocker, research_mode, key_type):
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
-
-    if research_mode:
-        sample_service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-    sample_notification.key_type = key_type
-
-    send_to_providers.send_sms_to_provider(
-        sample_notification
-    )
-    assert notifications_dao.get_notification_by_id(sample_notification.id).billable_units == 0
-
-
 def test_should_not_send_to_provider_when_status_is_not_created(
     sample_template,
     mocker
@@ -340,7 +318,8 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_
     notification = create_notification(
         template=sample_email_template,
         to_field="john@smith.com",
-        key_type=key_type
+        key_type=key_type,
+        billable_units=0
     )
     sample_service.research_mode = research_mode
 
@@ -513,19 +492,22 @@ def test_get_logo_url_works_for_different_environments(base_url, expected_url):
     assert logo_url == expected_url
 
 
-def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker):
+def test_should_not_update_notification_if_research_mode_on_exception(
-    mocker.patch('app.mmg_client.send_sms')
+        sample_service, sample_notification, mocker
-    mocker.patch('app.delivery.send_to_providers.send_sms_response')
+):
-
+    mocker.patch('app.delivery.send_to_providers.send_sms_response', side_effect=Exception())
+    update_mock = mocker.patch('app.delivery.send_to_providers.update_notification_to_sending')
     sample_service.research_mode = True
-    notify_db.session.add(sample_service)
+    sample_notification.billable_units = 0
-    notify_db.session.commit()
+
+    with pytest.raises(Exception):
+        send_to_providers.send_sms_to_provider(
+            sample_notification
+        )
 
-    send_to_providers.send_sms_to_provider(
-        sample_notification
-    )
     persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)
     assert persisted_notification.billable_units == 0
+    assert not update_mock.called
 
 
 @pytest.mark.parametrize('research_mode,key_type, billable_units', [
@@ -537,33 +519,26 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic
     (False, KEY_TYPE_TEAM, 1)
 ])
 def test_should_update_billable_units_according_to_research_mode_and_key_type(
-    notify_db,
+    sample_template,
-    sample_service,
-    sample_notification,
     mocker,
     research_mode,
     key_type,
     billable_units
 ):
+    notification = create_notification(template=sample_template, billable_units=0, status='created', key_type=key_type)
     mocker.patch('app.mmg_client.send_sms')
     mocker.patch('app.delivery.send_to_providers.send_sms_response')
 
     if research_mode:
-        sample_service.research_mode = True
+        sample_template.service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-
-    sample_notification.key_type = key_type
 
     send_to_providers.send_sms_to_provider(
-        sample_notification
+        notification
     )
-    assert sample_notification.billable_units == billable_units
+    assert notification.billable_units == billable_units
 
 
-def test_should_set_notification_billable_units_and_provider_if_sending_to_provider_fails(
+def test_should_set_notification_billable_units_if_sending_to_provider_fails(
-    notify_db,
-    sample_service,
     sample_notification,
     mocker,
 ):
@@ -577,7 +552,6 @@ def test_should_set_notification_billable_units_and_provider_if_sending_to_provi
         send_to_providers.send_sms_to_provider(sample_notification)
 
     assert sample_notification.billable_units == 1
-    assert sample_notification.sent_by == 'mmg'
     assert mock_toggle_provider.called
 
 
@@ -602,7 +576,7 @@ def test_should_send_sms_to_international_providers(
 
     db_notification_international = create_notification(
         template=sample_sms_template_with_html,
-        to_field="+447234123111",
+        to_field="+6011-17224412",
         personalisation={"name": "Jo"},
         status='created',
         international=True,
@@ -628,7 +602,7 @@ def test_should_send_sms_to_international_providers(
     )
 
     mmg_client.send_sms.assert_called_once_with(
-        to="447234123111",
+        to="601117224412",
         content=ANY,
         reference=str(db_notification_international.id),
         sender=current_app.config['FROM_NUMBER']
@@ -643,48 +617,6 @@ def test_should_send_sms_to_international_providers(
     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'
-
-
 @pytest.mark.parametrize('sms_sender, expected_sender, prefix_sms, expected_content', [
     ('foo', 'foo', False, 'bar'),
     ('foo', 'foo', True, 'Sample service: bar'),