diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index e761956f3..37994691d 100644 --- a/app/delivery/send_to_providers.py +++ b/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( - to=notification.normalised_to, - content=str(template), - reference=str(notification.id), - sender=notification.reply_to_text, - international=notification.international - ) + # End DB session here so that we don't have a connection stuck open waiting on the call + # to one of the SMS providers + # We don't want to tie our DB connections being open to the performance of our SMS + # providers as a slow down of our providers can cause us to run out of DB connections + # Therefore we pull all the data from our DB models into `send_sms_kwargs`now before + # 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) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index cf1ef1552..9fde06690 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/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'),