diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index fa9ccfba9..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( @@ -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.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']) @@ -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 - 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 d04d05206..2f331896e 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 @@ -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): - 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', [ @@ -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_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, +def test_should_set_notification_billable_units_if_sending_to_provider_fails( 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'),