From 0136e1e32d1b69fcaf933977c2f9ef7c2edd1d38 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 15 Dec 2016 17:30:05 +0000 Subject: [PATCH] fix invalid logging the first argument to ANY logger.____ function is ALWAYS cast to a string and used as a format argument for ALL remaining arguments using %s formatting. even `logger.exception`, which just logs as normal and then appends the stack trace. so we shouldn't be passing `e` into logger.exception - just `logger.exception('something went wrong!')` also de-duplicated a test --- app/celery/provider_tasks.py | 12 ++++-------- config.py | 2 +- tests/app/clients/test_firetext.py | 2 +- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ef98ec5e3..b0df268b3 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -43,15 +43,13 @@ def deliver_sms(self, notification_id): send_to_providers.send_sms_to_provider(notification) except Exception as e: try: - current_app.logger.error( + current_app.logger.exception( "RETRY: SMS notification {} failed".format(notification_id) ) - current_app.logger.exception(e) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: - current_app.logger.error( + current_app.logger.exception( "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id), - e ) update_notification_status_by_id(notification_id, 'technical-failure') @@ -69,14 +67,12 @@ def deliver_email(self, notification_id): update_notification_status_by_id(notification_id, 'technical-failure') except Exception as e: try: - current_app.logger.error( + current_app.logger.exception( "RETRY: Email notification {} failed".format(notification_id) ) - current_app.logger.exception(e) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id), - e + "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id) ) update_notification_status_by_id(notification_id, 'technical-failure') diff --git a/config.py b/config.py index 179d8fcf3..5c4105ffc 100644 --- a/config.py +++ b/config.py @@ -137,7 +137,7 @@ class Config(object): REDIS_ENABLED = False - SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 + SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 # 3 days SIMULATED_EMAIL_ADDRESSES = ('simulate-delivered@notifications.service.gov.uk', 'simulate-permanent-failure@notifications.service.gov.uk', diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 80737e23d..5685db48b 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -98,7 +98,7 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): assert '"code": 1' in exc.value.text -def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): +def test_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mock_firetext_client): to = content = reference = 'foo' response_dict = {"something": "gone bad"}