diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index f9dc99f11..947c4e782 100644 --- a/app/celery/process_sms_client_response_tasks.py +++ b/app/celery/process_sms_client_response_tasks.py @@ -24,7 +24,7 @@ sms_response_mapper = { @notify_celery.task(bind=True, name="process-sms-client-response", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") -def process_sms_client_response(self, status, provider_reference, client_name, code=None): +def process_sms_client_response(self, status, provider_reference, client_name, detailed_status_code=None): # validate reference try: uuid.UUID(provider_reference, version=4) @@ -36,7 +36,7 @@ def process_sms_client_response(self, status, provider_reference, client_name, c # validate status try: - notification_status, detailed_status = response_parser(status, code) + notification_status, detailed_status = response_parser(status, detailed_status_code) current_app.logger.info( f'{client_name} callback returned ' f'status of {notification_status}: {detailed_status} for reference: {provider_reference}' @@ -53,17 +53,17 @@ def process_sms_client_response(self, status, provider_reference, client_name, c notification_status=notification_status, client_name=client_name, provider_reference=provider_reference, - code=code + detailed_status_code=detailed_status_code ) -def _process_for_status(notification_status, client_name, provider_reference, code=None): +def _process_for_status(notification_status, client_name, provider_reference, detailed_status_code=None): # record stats notification = notifications_dao.update_notification_status_by_id( notification_id=provider_reference, status=notification_status, sent_by=client_name.lower(), - code=code + detailed_status_code=detailed_status_code ) if not notification: return diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index f647a66d2..a0b1cb783 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -33,13 +33,15 @@ firetext_codes = { } -def get_firetext_responses(status, code=None): - substatus = firetext_codes[code]['reason'] if firetext_codes.get(code, None) else None - return firetext_responses[status], substatus +def get_firetext_responses(status, detailed_status_code=None): + detailed_status = firetext_codes[detailed_status_code]['reason'] if firetext_codes.get( + detailed_status_code, None + ) else None + return (firetext_responses[status], detailed_status) -def get_message_status_and_reason_from_firetext_code(code): - return firetext_codes[code]['status'], firetext_codes[code]['reason'] +def get_message_status_and_reason_from_firetext_code(detailed_status_code): + return firetext_codes[detailed_status_code]['status'], firetext_codes[detailed_status_code]['reason'] class FiretextClientResponseException(SmsClientResponseException): diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 2c3db3d40..443809e75 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -45,8 +45,8 @@ mmg_response_map = { } -def get_mmg_responses(status, substatus=None): - return (mmg_response_map[status]["status"], mmg_response_map[status]["substatus"].get(substatus, None)) +def get_mmg_responses(status, detailed_status_code=None): + return (mmg_response_map[status]["status"], mmg_response_map[status]["substatus"].get(detailed_status_code, None)) class MMGClientResponseException(SmsClientResponseException): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 17318e1ab..6629a118c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -89,19 +89,19 @@ def dao_create_notification(notification): db.session.add(notification) -def _decide_permanent_temporary_failure(status, notification, code=None): +def _decide_permanent_temporary_failure(status, notification, detailed_status_code=None): # If we get failure status from Firetext, we want to know if this is temporary or permanent failure. # So we check the failure code to learn that. # If there is no failure code, or we do not recognise the failure code, we do the following: # if notifitcation goes form status pending to status failure, we mark it as temporary failure; # if notification goes straight to status failure, we mark it as permanent failure. - if status == NOTIFICATION_PERMANENT_FAILURE and code not in [None, '000']: + if status == NOTIFICATION_PERMANENT_FAILURE and detailed_status_code not in [None, '000']: try: - status, reason = get_message_status_and_reason_from_firetext_code(code) + status, reason = get_message_status_and_reason_from_firetext_code(detailed_status_code) current_app.logger.info(f'Updating notification id {notification.id} to status {status}, reason: {reason}') return status except KeyError: - current_app.logger.warning(f'Failure code {code} from Firetext not recognised') + current_app.logger.warning(f'Failure code {detailed_status_code} from Firetext not recognised') # fallback option: if notification.status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE: status = NOTIFICATION_TEMPORARY_FAILURE @@ -113,8 +113,10 @@ def country_records_delivery(phone_prefix): return dlr and dlr.lower() == 'yes' -def _update_notification_status(notification, status, code=None): - status = _decide_permanent_temporary_failure(status=status, notification=notification, code=code) +def _update_notification_status(notification, status, detailed_status_code=None): + status = _decide_permanent_temporary_failure( + status=status, notification=notification, detailed_status_code=detailed_status_code + ) notification.status = status dao_update_notification(notification) return notification @@ -122,7 +124,7 @@ def _update_notification_status(notification, status, code=None): @statsd(namespace="dao") @transactional -def update_notification_status_by_id(notification_id, status, sent_by=None, code=None): +def update_notification_status_by_id(notification_id, status, sent_by=None, detailed_status_code=None): notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first() if not notification: @@ -149,7 +151,7 @@ def update_notification_status_by_id(notification_id, status, sent_by=None, code return _update_notification_status( notification=notification, status=status, - code=code + detailed_status_code=detailed_status_code ) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 447acab50..7789f592a 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -22,12 +22,12 @@ def process_mmg_response(): raise InvalidRequest(errors, status_code=400) status = str(data.get('status')) - substatus = str(data.get('substatus')) + detailed_status_code = str(data.get('substatus')) provider_reference = data.get('CID') process_sms_client_response.apply_async( - [status, provider_reference, client_name, substatus], + [status, provider_reference, client_name, detailed_status_code], queue=QueueNames.SMS_CALLBACKS, ) @@ -50,7 +50,7 @@ def process_firetext_response(): raise InvalidRequest(errors, status_code=400) status = request.form.get('status') - code = request.form.get('code') + detailed_status_code = request.form.get('code') provider_reference = request.form.get('reference') safe_to_log = dict(request.form).copy() @@ -60,7 +60,7 @@ def process_firetext_response(): ) process_sms_client_response.apply_async( - [status, provider_reference, client_name, code], + [status, provider_reference, client_name, detailed_status_code], queue=QueueNames.SMS_CALLBACKS, ) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 773549f2b..3494ef83b 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -8,25 +8,25 @@ import requests_mock from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException, FiretextClientResponseException -@pytest.mark.parametrize('code, result', [ +@pytest.mark.parametrize('detailed_status_code, result', [ (None, ('delivered', None)), ('000', ('delivered', None)) ]) -def test_get_firetext_responses_should_return_correct_details_for_delivery(code, result): - assert get_firetext_responses('0', code) == result +def test_get_firetext_responses_should_return_correct_details_for_delivery(detailed_status_code, result): + assert get_firetext_responses('0', detailed_status_code) == result -@pytest.mark.parametrize('code, result', [ +@pytest.mark.parametrize('detailed_status_code, result', [ (None, ('permanent-failure', None)), ('401', ('permanent-failure', 'Message Rejected')) ]) -def test_get_firetext_responses_should_return_correct_details_for_bounced(code, result): - assert get_firetext_responses('1', code) == result +def test_get_firetext_responses_should_return_correct_details_for_bounced(detailed_status_code, result): + assert get_firetext_responses('1', detailed_status_code) == result def test_get_firetext_responses_should_return_correct_details_for_complaint(): assert get_firetext_responses('2') == ('pending', None) -def test_get_firetext_responses_should_be_none_if_unrecognised_status_code(): +def test_get_firetext_responses_raises_KeyError_if_unrecognised_status_code(): with pytest.raises(KeyError) as e: get_firetext_responses('99') assert '99' in str(e.value) diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 026325c0e..a547e2df5 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -9,31 +9,31 @@ from app.clients.sms import SmsClientResponseException from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException -@pytest.mark.parametrize('substatus, result', [ +@pytest.mark.parametrize('detailed_status_code, result', [ (None, ('delivered', None)), ('5', ('delivered', 'Delivered to handset')) ]) -def test_get_mmg_responses_should_return_correct_details_for_delivery(substatus, result): - assert get_mmg_responses('3', substatus) == result +def test_get_mmg_responses_should_return_correct_details_for_delivery(detailed_status_code, result): + assert get_mmg_responses('3', detailed_status_code) == result -@pytest.mark.parametrize('substatus, result', [ +@pytest.mark.parametrize('detailed_status_code, result', [ (None, ('temporary-failure', None)), ('15', ('temporary-failure', 'Expired')) ]) -def test_get_mmg_responses_should_return_correct_details_for_temporary_failure(substatus, result): - assert get_mmg_responses('4', substatus) == result +def test_get_mmg_responses_should_return_correct_details_for_temporary_failure(detailed_status_code, result): + assert get_mmg_responses('4', detailed_status_code) == result -@pytest.mark.parametrize('status, substatus, result', [ +@pytest.mark.parametrize('status, detailed_status_code, result', [ ('2', None, ('permanent-failure', None)), ('2', '12', ('permanent-failure', "Illegal equipment")), ('5', None, ('permanent-failure', None)), ('5', '20', ('permanent-failure', 'Rejected by anti-flooding mechanism')) ]) -def test_get_mmg_responses_should_return_correct_details_for_bounced(status, substatus, result): - assert get_mmg_responses(status, substatus) == result +def test_get_mmg_responses_should_return_correct_details_for_bounced(status, detailed_status_code, result): + assert get_mmg_responses(status, detailed_status_code) == result -def test_get_mmg_responses_should_be_raise_if_unrecognised_status_code(): +def test_get_mmg_responses_raises_KeyError_if_unrecognised_status_code(): with pytest.raises(KeyError) as e: get_mmg_responses('99') assert '99' in str(e.value) diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index b849d96c4..c60da0003 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -35,7 +35,7 @@ def test_process_sms_response_raises_client_exception_for_unknown_status( assert sample_notification.status == NOTIFICATION_TECHNICAL_FAILURE -@pytest.mark.parametrize('status, substatus, sms_provider, expected_notification_status, reason', [ +@pytest.mark.parametrize('status, detailed_status_code, sms_provider, expected_notification_status, reason', [ ('0', None, 'Firetext', 'delivered', None), ('1', '101', 'Firetext', 'permanent-failure', 'Unknown Subscriber'), ('2', '102', 'Firetext', 'pending', 'Absent Subscriber'), @@ -48,7 +48,7 @@ def test_process_sms_client_response_updates_notification_status( sample_notification, mocker, status, - substatus, + detailed_status_code, sms_provider, expected_notification_status, reason @@ -56,14 +56,14 @@ def test_process_sms_client_response_updates_notification_status( mock_logger = mocker.patch('app.celery.tasks.current_app.logger.info') sample_notification.status = 'sending' - process_sms_client_response(status, str(sample_notification.id), sms_provider, substatus) + process_sms_client_response(status, str(sample_notification.id), sms_provider, detailed_status_code) message = f"{sms_provider} callback returned status of {expected_notification_status}: {reason} for reference: {sample_notification.id}" # noqa mock_logger.assert_any_call(message) assert sample_notification.status == expected_notification_status -@pytest.mark.parametrize('code, expected_notification_status, reason', [ +@pytest.mark.parametrize('detailed_status_code, expected_notification_status, reason', [ ('101', 'permanent-failure', 'Unknown Subscriber'), ('102', 'temporary-failure', 'Absent Subscriber'), (None, 'temporary-failure', None), @@ -72,7 +72,7 @@ def test_process_sms_client_response_updates_notification_status( def test_process_sms_client_response_updates_notification_status_when_called_second_time( sample_notification, mocker, - code, + detailed_status_code, expected_notification_status, reason ): @@ -80,29 +80,29 @@ def test_process_sms_client_response_updates_notification_status_when_called_sec sample_notification.status = 'sending' process_sms_client_response('2', str(sample_notification.id), 'Firetext') - process_sms_client_response('1', str(sample_notification.id), 'Firetext', code) + process_sms_client_response('1', str(sample_notification.id), 'Firetext', detailed_status_code) - if code and code != '000': + if detailed_status_code and detailed_status_code != '000': message = f'Updating notification id {sample_notification.id} to status {expected_notification_status}, reason: {reason}' # noqa mock_logger.assert_called_with(message) assert sample_notification.status == expected_notification_status -@pytest.mark.parametrize('code', ['102', None, '000']) -def test_process_sms_client_response_updates_notification_status_to_pending_with_and_without_failire_code_present( +@pytest.mark.parametrize('detailed_status_code', ['102', None, '000']) +def test_process_sms_client_response_updates_notification_status_to_pending_with_and_without_failure_code_present( sample_notification, mocker, - code + detailed_status_code ): sample_notification.status = 'sending' - process_sms_client_response('2', str(sample_notification.id), 'Firetext', code) + process_sms_client_response('2', str(sample_notification.id), 'Firetext', detailed_status_code) assert sample_notification.status == 'pending' -def test_process_sms_client_response_updates_notification_status_when_code_unknown( +def test_process_sms_client_response_updates_notification_status_when_detailed_status_code_not_recognised( sample_notification, mocker, ):