From 0a429e18b48ba41a73bc022b94a39c4c68d29c2d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 10 Aug 2016 15:46:12 +0100 Subject: [PATCH 1/2] Give better error when sending to non-team member Scenario we saw in research: - trying to send a message to someone outside your team - service is in trial mode Result: - error message was terrible, no-one understood it Solution: - better error message --- app/notifications/rest.py | 10 +++++++--- .../rest/test_send_notification.py | 20 +++++++++++++++---- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 5461c198e..102a0ecd1 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -270,9 +270,13 @@ def send_notification(notification_type): [user.mobile_number, user.email_address] for user in service.users ) ): - message = 'Invalid {} for restricted service'.format(first_column_heading[notification_type]) - errors = {'to': [message]} - raise InvalidRequest(errors, status_code=400) + raise InvalidRequest( + {'to': [( + 'Can’t send to this recipient when service is in trial mode ' + '– see https://www.notifications.service.gov.uk/trial-mode' + )]}, + status_code=400 + ) notification_id = create_uuid() notification.update({"template_version": template.version}) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index c201a11d0..a4a1c23dd 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -227,7 +227,10 @@ def test_should_not_send_sms_if_restricted_and_not_a_service_user(notify_api, sa app.celery.tasks.send_sms.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Invalid phone number for restricted service' in json_resp['message']['to'] + assert [( + 'Can’t send to this recipient when service is in trial mode ' + '– see https://www.notifications.service.gov.uk/trial-mode' + )] == json_resp['message']['to'] def test_should_send_sms_if_restricted_and_a_service_user(notify_api, sample_template, mocker): @@ -519,7 +522,10 @@ def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Invalid email address for restricted service' in json_resp['message']['to'] + assert [( + 'Can’t send to this recipient when service is in trial mode – see ' + 'https://www.notifications.service.gov.uk/trial-mode' + )] == json_resp['message']['to'] @freeze_time("2016-01-01 11:09:00.061258") @@ -698,7 +704,10 @@ def test_should_not_send_email_if_team_api_key_and_not_a_service_user(notify_api app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Invalid email address for restricted service' in json_resp['message']['to'] + assert [( + 'Can’t send to this recipient when service is in trial mode – see ' + 'https://www.notifications.service.gov.uk/trial-mode' + )] == json_resp['message']['to'] def test_should_not_send_sms_if_team_api_key_and_not_a_service_user(notify_api, sample_template, mocker): @@ -721,7 +730,10 @@ def test_should_not_send_sms_if_team_api_key_and_not_a_service_user(notify_api, app.celery.tasks.send_sms.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Invalid phone number for restricted service' in json_resp['message']['to'] + assert [( + 'Can’t send to this recipient when service is in trial mode – see ' + 'https://www.notifications.service.gov.uk/trial-mode' + )] == json_resp['message']['to'] def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample_email_template, mocker): From ebfaa5dac21006d4de295d73d567320a0c3ad49b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 10 Aug 2016 15:54:53 +0100 Subject: [PATCH 2/2] Show separate error messages for team key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although using a team key is functionally the same as your service being restricted, conflating the two errors is not helpful. What we typically saw in research was that someone was using a team key, got the error, used a live key and got the _same_ error. This commit adds a new error message that specifically mentions the type of API key that you’re using. --- app/notifications/rest.py | 10 +++++++--- .../notifications/rest/test_send_notification.py | 14 ++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 102a0ecd1..61bd66574 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -270,11 +270,15 @@ def send_notification(notification_type): [user.mobile_number, user.email_address] for user in service.users ) ): - raise InvalidRequest( - {'to': [( + if (api_user.key_type == KEY_TYPE_TEAM): + message = 'Can’t send to this recipient using a team-only API key' + else: + message = ( 'Can’t send to this recipient when service is in trial mode ' '– see https://www.notifications.service.gov.uk/trial-mode' - )]}, + ) + raise InvalidRequest( + {'to': [message]}, status_code=400 ) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index a4a1c23dd..47626ee85 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -704,10 +704,9 @@ def test_should_not_send_email_if_team_api_key_and_not_a_service_user(notify_api app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert [( - 'Can’t send to this recipient when service is in trial mode – see ' - 'https://www.notifications.service.gov.uk/trial-mode' - )] == json_resp['message']['to'] + assert [ + 'Can’t send to this recipient using a team-only API key' + ] == json_resp['message']['to'] def test_should_not_send_sms_if_team_api_key_and_not_a_service_user(notify_api, sample_template, mocker): @@ -730,10 +729,9 @@ def test_should_not_send_sms_if_team_api_key_and_not_a_service_user(notify_api, app.celery.tasks.send_sms.apply_async.assert_not_called() assert response.status_code == 400 - assert [( - 'Can’t send to this recipient when service is in trial mode – see ' - 'https://www.notifications.service.gov.uk/trial-mode' - )] == json_resp['message']['to'] + assert [ + 'Can’t send to this recipient using a team-only API key' + ] == json_resp['message']['to'] def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample_email_template, mocker):