Merge pull request #3408 from alphagov/db-connection-close

Close DB connection whilst making HTTP to SMS providers

app/delivery/send_to_providers.py

@@ -10,7 +10,7 @@ from notifications_utils.template import (
     SMSMessageTemplate,
 )
 
-from app import create_uuid, notification_provider_clients, statsd_client
+from app import create_uuid, db, notification_provider_clients, statsd_client
 from app.celery.research_mode_tasks import (
     send_email_response,
     send_sms_response,
@@ -64,13 +64,21 @@ def send_sms_to_provider(notification):
 
         else:
             try:
-                provider.send_sms(
+                # End DB session here so that we don't have a connection stuck open waiting on the call
-                    to=notification.normalised_to,
+                # to one of the SMS providers
-                    content=str(template),
+                # We don't want to tie our DB connections being open to the performance of our SMS
-                    reference=str(notification.id),
+                # providers as a slow down of our providers can cause us to run out of DB connections
-                    sender=notification.reply_to_text,
+                # Therefore we pull all the data from our DB models into `send_sms_kwargs`now before
-                    international=notification.international
+                # closing the session (as otherwise it would be reopened immediately)
-                )
+                send_sms_kwargs = {
+                    'to': notification.normalised_to,
+                    'content': str(template),
+                    'reference': str(notification.id),
+                    'sender': notification.reply_to_text,
+                    'international': notification.international,
+                }
+                db.session.close()  # no commit needed as no changes to objects have been made above
+                provider.send_sms(**send_sms_kwargs)
             except Exception as e:
                 notification.billable_units = template.fragment_count
                 dao_update_notification(notification)

tests/app/delivery/test_send_to_providers.py

@@ -218,6 +218,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
     mocker.patch('app.mmg_client.send_sms')
 
     version_on_notification = sample_template.version
+    expected_template_id = sample_template.id
 
     # Change the template
     from app.dao.templates_dao import (
@@ -241,11 +242,13 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
         international=False
     )
 
+    t = dao_get_template_by_id(expected_template_id)
+
     persisted_notification = notifications_dao.get_notification_by_id(db_notification.id)
     assert persisted_notification.to == db_notification.to
-    assert persisted_notification.template_id == sample_template.id
+    assert persisted_notification.template_id == expected_template_id
     assert persisted_notification.template_version == version_on_notification
-    assert persisted_notification.template_version != sample_template.version
+    assert persisted_notification.template_version != t.version
     assert persisted_notification.status == 'sending'
     assert not persisted_notification.personalisation
 
@@ -349,6 +352,7 @@ def test_send_sms_should_use_service_sms_sender(
 
     sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456', is_default=False)
     db_notification = create_notification(template=sample_template, reply_to_text=sms_sender.sms_sender)
+    expected_sender_name = sms_sender.sms_sender
 
     send_to_providers.send_sms_to_provider(
         db_notification,
@@ -358,7 +362,7 @@ def test_send_sms_should_use_service_sms_sender(
         to=ANY,
         content=ANY,
         reference=ANY,
-        sender=sms_sender.sms_sender,
+        sender=expected_sender_name,
         international=False
     )
 
@@ -653,16 +657,6 @@ def test_should_send_sms_to_international_providers(
     get_provider_details_by_identifier('firetext').priority = 100
     get_provider_details_by_identifier('mmg').priority = 0
 
-    notification_uk = create_notification(
-        template=sample_template,
-        to_field="+447234123999",
-        personalisation={"name": "Jo"},
-        status='created',
-        international=False,
-        reply_to_text=sample_template.service.get_default_sms_sender(),
-        normalised_to="447234123999"
-    )
-
     notification_international = create_notification(
         template=sample_template,
         to_field="+6011-17224412",
@@ -672,17 +666,6 @@ def test_should_send_sms_to_international_providers(
         reply_to_text=sample_template.service.get_default_sms_sender(),
         normalised_to='601117224412'
     )
-    send_to_providers.send_sms_to_provider(
-        notification_uk
-    )
-
-    firetext_client.send_sms.assert_called_once_with(
-        to="447234123999",
-        content=ANY,
-        reference=str(notification_uk.id),
-        sender=current_app.config['FROM_NUMBER'],
-        international=False
-    )
 
     send_to_providers.send_sms_to_provider(
         notification_international
@@ -696,12 +679,48 @@ def test_should_send_sms_to_international_providers(
         international=True
     )
 
-    assert notification_uk.status == 'sending'
-    assert notification_uk.sent_by == 'firetext'
     assert notification_international.status == 'sent'
     assert notification_international.sent_by == 'mmg'
 
 
+def test_should_send_non_international_sms_to_default_provider(
+    sample_template,
+    sample_user,
+    mocker
+):
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.firetext_client.send_sms')
+
+    # set firetext to active
+    get_provider_details_by_identifier('firetext').priority = 100
+    get_provider_details_by_identifier('mmg').priority = 0
+
+    notification_uk = create_notification(
+        template=sample_template,
+        to_field="+447234123999",
+        personalisation={"name": "Jo"},
+        status='created',
+        international=False,
+        reply_to_text=sample_template.service.get_default_sms_sender(),
+        normalised_to="447234123999"
+    )
+
+    send_to_providers.send_sms_to_provider(
+        notification_uk
+    )
+
+    firetext_client.send_sms.assert_called_once_with(
+        to="447234123999",
+        content=ANY,
+        reference=str(notification_uk.id),
+        sender=current_app.config['FROM_NUMBER'],
+        international=False
+    )
+
+    assert notification_uk.status == 'sending'
+    assert notification_uk.sent_by == 'firetext'
+
+
 @pytest.mark.parametrize('sms_sender, expected_sender, prefix_sms, expected_content', [
     ('foo', 'foo', False, 'bar'),
     ('foo', 'foo', True, 'Sample service: bar'),