From 933d5bf68e3e6fead4377d7883cc821658a41e55 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 8 Feb 2021 16:38:50 +0000 Subject: [PATCH 1/2] Show 'From' / 'Reply To' if set for notification Previously when a service had multiple "reply to" entries setup for email or SMS, we would show the one that was selected on all screens [1][2] except the final one, where the notification is actually sent. This fixes that, with the caveat that it will also show for services with only one "reply to" entry (see notes below) - we will look at making this consistent on the previous screens in the next commit. Here's a bit more detail on how this works: - If a service has multiple "reply to" entries, the journey to send a one-off message starts with a screen to select the "sender_id", which is otherwise "None" [3]. - The "sender_id" is subsequently resolved to an actual email / phone number by calling an API [4] and plucking it out of the response JSON. - The email / phone number then get rendered as part of the preview template [5][6]. - Unfortunately the "sender_id" is removed from the session by the time we get to the "view_notification" view [7]. - However, we can get back the equivalent text from the notification JSON, which is set by the API when the notification is created [8], give or take a bit of validation code [9][10]. - But the "reply_to_text" field is also set by the API when the service only has one "reply to" entry, so it will show then as well. We could add look at the number of "reply to" entries for the service, in order to consistently only show it when there is more the one. But it seems more useful to show it on previous screens, since it provides more information than is currently show (esp. for emails). [1]: https://github.com/alphagov/notifications-admin/blob/93226ec5d6dde489fae6e2e605aa2cfc04a8eff0/app/main/views/send.py#L441-L442 [2]: https://github.com/alphagov/notifications-admin/blob/93226ec5d6dde489fae6e2e605aa2cfc04a8eff0/app/main/views/send.py#L966-L967 [3]: https://github.com/alphagov/notifications-admin/blob/93226ec5d6dde489fae6e2e605aa2cfc04a8eff0/app/main/views/send.py#L247 [4]: https://github.com/alphagov/notifications-admin/blob/93226ec5d6dde489fae6e2e605aa2cfc04a8eff0/app/main/views/send.py#L1071-L1082 [5]: https://github.com/alphagov/notifications-admin/blob/93226ec5d6dde489fae6e2e605aa2cfc04a8eff0/app/templates/views/notifications/notification.html#L80 [6]: https://github.com/alphagov/notifications-utils/blob/master/notifications_utils/jinja_templates/sms_preview_template.jinja2 [7]: https://github.com/alphagov/notifications-admin/blob/93226ec5d6dde489fae6e2e605aa2cfc04a8eff0/app/main/views/send.py#L1059 [8]: https://github.com/alphagov/notifications-api/blob/f8b4c9151cd135195c2fe416d4f54516d8d3833f/app/service/send_notification.py#L87-L93 [9]: https://github.com/alphagov/notifications-api/blob/f8b4c9151cd135195c2fe416d4f54516d8d3833f/app/models.py#L653 [10]: https://github.com/alphagov/notifications-utils/blob/master/notifications_utils/recipients.py#L482 --- app/main/views/notifications.py | 2 ++ tests/app/main/views/test_notifications.py | 19 +++++++++++++++++++ tests/conftest.py | 4 +++- 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 2a533bc30..00199291b 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -95,6 +95,8 @@ def view_notification(service_id, notification_id): page_count=page_count, show_recipient=True, redact_missing_personalisation=True, + sms_sender=notification['reply_to_text'], + email_reply_to=notification['reply_to_text'], ) template.values = personalisation if notification['job']: diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 3160c4cff..0a44ee326 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -949,3 +949,22 @@ def test_cancelling_a_letter_calls_the_api( ) assert cancel_endpoint.called + + +@pytest.mark.parametrize('notification_type', ['sms', 'email']) +def test_should_show_reply_to_from_notification( + mocker, + fake_uuid, + notification_type, + client_request, +): + notification = create_notification(reply_to_text='reply to info', template_type=notification_type) + mocker.patch('app.notification_api_client.get_notification', return_value=notification) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert 'reply to info' in page.text diff --git a/tests/conftest.py b/tests/conftest.py index e1d8eee1d..98e2a551e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4174,13 +4174,15 @@ def create_notification( key_type=None, postage=None, sent_one_off=True, + reply_to_text=None, ): noti = notification_json( service_id, rows=1, status=notification_status, template_type=template_type, - postage=postage + postage=postage, + reply_to_text=reply_to_text, )['notifications'][0] noti['id'] = notifification_id or sample_uuid() From 486393962e2dda1cb3147409414c98687c87d207 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 8 Feb 2021 17:23:47 +0000 Subject: [PATCH 2/2] Show 'From' / 'Reply to' on all sending screens This makes the preview of the email / SMS to send consistent with the final screen, which we previously changed to show the "reply to" text irrespective of whether the user had selected anything. --- app/main/views/send.py | 4 +++ tests/app/main/views/test_send.py | 46 +++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/app/main/views/send.py b/app/main/views/send.py index 20d1deddc..04356069e 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -218,6 +218,10 @@ def set_sender(service_id, template_id): return redirect_to_one_off sender_details = get_sender_details(service_id, template['template_type']) + + if len(sender_details) == 1: + session['sender_id'] = sender_details[0]['id'] + if len(sender_details) <= 1: return redirect_to_one_off diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 35ca10131..0b6f3bc0c 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -254,6 +254,52 @@ def test_set_sender_redirects_if_no_sms_senders( ) +def test_set_sender_redirects_if_one_email_sender( + client_request, + fake_uuid, + mock_get_service_email_template, + single_reply_to_email_address, +): + client_request.get( + '.set_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=302, + _expected_url=url_for( + '.send_one_off', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _external=True, + ) + ) + + with client_request.session_transaction() as session: + assert session['sender_id'] == '1234' + + +def test_set_sender_redirects_if_one_sms_sender( + client_request, + fake_uuid, + mock_get_service_template, + single_sms_sender, +): + client_request.get( + '.set_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _expected_status=302, + _expected_url=url_for( + '.send_one_off', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _external=True, + ) + ) + + with client_request.session_transaction() as session: + assert session['sender_id'] == '1234' + + def test_that_test_files_exist(): assert len(test_spreadsheet_files) == 8 assert len(test_non_spreadsheet_files) == 6