From ffa093d8c7cbe5fb088e07bf97945ac71c67677b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 21 Mar 2018 18:11:10 +0000 Subject: [PATCH] Set sent_by if not set during sms provider callback - refactored argument reference to provider_reference to make it clearer --- .../notifications_sms_callback.py | 4 +- app/notifications/process_client_response.py | 40 ++++++++++++++----- .../test_process_client_response.py | 32 ++++++++++++--- 3 files changed, 57 insertions(+), 19 deletions(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 950e68b7b..81e7b80b8 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -21,7 +21,7 @@ def process_mmg_response(): raise InvalidRequest(errors, status_code=400) success, errors = process_sms_client_response(status=str(data.get('status')), - reference=data.get('CID'), + provider_reference=data.get('CID'), client_name=client_name) safe_to_log = data.copy() @@ -49,7 +49,7 @@ def process_firetext_response(): "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), safe_to_log)) success, errors = process_sms_client_response(status=request.form.get('status'), - reference=request.form.get('reference'), + provider_reference=request.form.get('reference'), client_name=client_name) if errors: raise InvalidRequest(errors, status_code=400) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 4a5cc0434..afc718ba6 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -13,6 +13,7 @@ from app.celery.service_callback_tasks import ( create_encrypted_callback_data, ) from app.config import QueueNames +from app.dao.notifications_dao import dao_update_notification from app.dao.service_callback_api_dao import get_service_callback_api_for_service sms_response_mapper = { @@ -30,18 +31,18 @@ def validate_callback_data(data, fields, client_name): return errors if len(errors) > 0 else None -def process_sms_client_response(status, reference, client_name): +def process_sms_client_response(status, provider_reference, client_name): success = None errors = None # validate reference - if reference == 'send-sms-code': + if provider_reference == 'send-sms-code': success = "{} callback succeeded: send-sms-code".format(client_name) return success, errors try: - uuid.UUID(reference, version=4) + uuid.UUID(provider_reference, version=4) except ValueError: - errors = "{} callback with invalid reference {}".format(client_name, reference) + errors = "{} callback with invalid reference {}".format(client_name, provider_reference) return success, errors try: @@ -53,27 +54,39 @@ def process_sms_client_response(status, reference, client_name): try: notification_status = response_parser(status) current_app.logger.info('{} callback return status of {} for reference: {}'.format( - client_name, status, reference) + client_name, status, provider_reference) ) except KeyError: - _process_for_status(notification_status='technical-failure', client_name=client_name, reference=reference) + _process_for_status( + notification_status='technical-failure', + client_name=client_name, + provider_reference=provider_reference + ) raise ClientException("{} callback failed: status {} not found.".format(client_name, status)) - success = _process_for_status(notification_status=notification_status, client_name=client_name, reference=reference) + success = _process_for_status( + notification_status=notification_status, + client_name=client_name, + provider_reference=provider_reference + ) return success, errors -def _process_for_status(notification_status, client_name, reference): +def _process_for_status(notification_status, client_name, provider_reference): # record stats - notification = notifications_dao.update_notification_status_by_id(reference, notification_status) + notification = notifications_dao.update_notification_status_by_id(provider_reference, notification_status) if not notification: current_app.logger.warning("{} callback failed: notification {} either not found or already updated " "from sending. Status {}".format(client_name, - reference, + provider_reference, notification_status)) return statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status)) + + if not notification.sent_by: + set_notification_sent_by(notification, client_name) + if notification.sent_at: statsd_client.timing_with_dates( 'callback.{}.elapsed-time'.format(client_name.lower()), @@ -89,5 +102,10 @@ def _process_for_status(notification_status, client_name, reference): send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], queue=QueueNames.CALLBACKS) - success = "{} callback succeeded. reference {} updated".format(client_name, reference) + success = "{} callback succeeded. reference {} updated".format(client_name, provider_reference) return success + + +def set_notification_sent_by(notification, client_name): + notification.sent_by = client_name + dao_update_notification(notification) diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index eb2e82cd0..4a5299555 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -61,7 +61,7 @@ def test_outcome_statistics_called_for_successful_callback(sample_notification, callback_api = create_service_callback_api(service=sample_notification.service, url="https://original_url.com") reference = str(uuid.uuid4()) - success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') + success, error = process_sms_client_response(status='3', provider_reference=reference, client_name='MMG') assert success == "MMG callback succeeded. reference {} updated".format(str(reference)) assert error is None encrypted_data = create_encrypted_callback_data(sample_notification, callback_api) @@ -78,24 +78,44 @@ def test_sms_resonse_does_not_call_send_callback_if_no_db_entry(sample_notificat 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) reference = str(uuid.uuid4()) - process_sms_client_response(status='3', reference=reference, client_name='MMG') + process_sms_client_response(status='3', provider_reference=reference, client_name='MMG') send_mock.assert_not_called() def test_process_sms_response_return_success_for_send_sms_code_reference(mocker): - success, error = process_sms_client_response(status='000', reference='send-sms-code', client_name='sms-client') + success, error = process_sms_client_response( + status='000', provider_reference='send-sms-code', client_name='sms-client') assert success == "{} callback succeeded: send-sms-code".format('sms-client') assert error is None +def test_process_sms_updates_sent_by_with_client_name_if_not_in_noti(notify_db, sample_notification): + sample_notification.sent_by = None + success, error = process_sms_client_response( + status='3', provider_reference=str(sample_notification.id), client_name='MMG') + assert error is None + assert success == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id) + assert sample_notification.sent_by == 'MMG' + + +def test_process_sms_does_not_update_sent_by_if_already_set(mocker, notify_db, sample_notification): + mock_update = mocker.patch('app.notifications.process_client_response.set_notification_sent_by') + sample_notification.sent_by = 'MMG' + process_sms_client_response( + status='3', provider_reference=str(sample_notification.id), client_name='MMG') + assert not mock_update.called + + def test_process_sms_response_returns_error_bad_reference(mocker): - success, error = process_sms_client_response(status='000', reference='something-bad', client_name='sms-client') + success, error = process_sms_client_response( + status='000', provider_reference='something-bad', client_name='sms-client') assert success is None assert error == "{} callback with invalid reference {}".format('sms-client', 'something-bad') def test_process_sms_response_raises_client_exception_for_unknown_sms_client(mocker): - success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='sms-client') + success, error = process_sms_client_response( + status='000', provider_reference=str(uuid.uuid4()), client_name='sms-client') assert success is None assert error == 'unknown sms client: {}'.format('sms-client') @@ -103,6 +123,6 @@ def test_process_sms_response_raises_client_exception_for_unknown_sms_client(moc def test_process_sms_response_raises_client_exception_for_unknown_status(mocker): with pytest.raises(ClientException) as e: - process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='Firetext') + process_sms_client_response(status='000', provider_reference=str(uuid.uuid4()), client_name='Firetext') assert "{} callback failed: status {} not found.".format('Firetext', '000') in str(e.value)