From 53f0e9334a67b3c9f3d6bd7b491bd67531ae3a89 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 8 May 2019 15:45:19 +0100 Subject: [PATCH 1/3] Refactor send_to_providers. - Remove some redundant code for research mode. - The international parameter in update_notification_to_sending is not needed. - Update unit tests and removed duplicates --- app/delivery/send_to_providers.py | 22 +--- tests/app/delivery/test_send_to_providers.py | 106 ++++--------------- 2 files changed, 24 insertions(+), 104 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index fa9ccfba9..ba7fd5764 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -55,18 +55,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( @@ -83,7 +74,7 @@ def send_sms_to_provider(notification): 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) @@ -145,13 +136,10 @@ def send_email_to_provider(notification): 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 - else: - notification.status = NOTIFICATION_SENDING + notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING 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 ae8b06782..fbfd6dec9 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/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 @@ -514,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): - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.delivery.send_to_providers.send_sms_response') - +def test_should_not_update_notification_if_research_mode_on_exception( + sample_service, sample_notification, mocker +): + 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) - notify_db.session.commit() + sample_notification.billable_units = 0 + + 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', [ @@ -538,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_service, - sample_notification, + sample_template, 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 - notify_db.session.add(sample_service) - notify_db.session.commit() - - sample_notification.key_type = key_type + sample_template.service.research_mode = True 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( - notify_db, - sample_service, sample_notification, mocker, ): @@ -603,7 +577,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, @@ -629,7 +603,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'] @@ -644,48 +618,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'), From 77c35da5caecabfb3494cd280465de2841cc7a0d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 8 May 2019 15:55:45 +0100 Subject: [PATCH 2/3] Refactor send_email_to_provider - Remove debug statement --- app/delivery/send_to_providers.py | 10 +++------- tests/app/delivery/test_send_to_providers.py | 3 ++- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index ba7fd5764..bcf699ff2 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -90,9 +90,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( @@ -107,11 +105,9 @@ def send_email_to_provider(notification): ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: - reference = str(create_uuid()) - notification.billable_units = 0 - notification.reference = reference + notification.reference = str(create_uuid()) 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']) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index fbfd6dec9..e3002a8c3 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -319,7 +319,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 From fc61fd2086a57e6363abad17202793c54ce90b41 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 8 May 2019 16:31:51 +0100 Subject: [PATCH 3/3] Remove updating provider on failure - we aren't sure yet who is sending the message the callback code sets that property as a fallback anyway. Remove debug statements. --- app/delivery/send_to_providers.py | 13 +------------ tests/app/delivery/test_send_to_providers.py | 3 +-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index bcf699ff2..b4475623d 100644 --- a/app/delivery/send_to_providers.py +++ b/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( @@ -68,7 +64,6 @@ 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 @@ -76,9 +71,6 @@ def send_sms_to_provider(notification): notification.billable_units = template.fragment_count 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) @@ -125,9 +117,6 @@ 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) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index e3002a8c3..8d4334da9 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -539,7 +539,7 @@ def test_should_update_billable_units_according_to_research_mode_and_key_type( 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( sample_notification, mocker, ): @@ -553,7 +553,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