From 441c1f441bba76eea8205d1de2b8ca4a8b1f0a7a Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 16 May 2019 17:05:55 +0100 Subject: [PATCH] Verify email address when editing reply-to address Also fix the tests --- app/main/views/service_settings.py | 37 +++++++++++------- app/navigation.py | 8 ++++ .../email-reply-to/_verify-updates.html | 2 +- .../email-reply-to/verify.html | 2 +- tests/app/main/views/test_service_settings.py | 38 ++++++++++--------- 5 files changed, 54 insertions(+), 33 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 95704a49d..d919deeea 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1,4 +1,5 @@ from collections import OrderedDict +from datetime import datetime from flask import ( abort, @@ -418,7 +419,7 @@ def service_add_email_reply_to(service_id): else: raise e return redirect(url_for( - '.verify_reply_to_address', + '.service_verify_reply_to_address', service_id=service_id, notification_id=notification_id ) + "?is_default={}".format(is_default)) @@ -432,23 +433,23 @@ def service_add_email_reply_to(service_id): @main.route("/services//service-settings/email-reply-to//verify", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') -def verify_reply_to_address(service_id, notification_id): +def service_verify_reply_to_address(service_id, notification_id): return render_template( 'views/service-settings/email-reply-to/verify.html', service_id=service_id, notification_id=notification_id, - partials=get_verify_reply_to_address_partials(service_id, notification_id) + partials=get_service_verify_reply_to_address_partials(service_id, notification_id) ) @main.route("/services//service-settings/email-reply-to//verify.json") @login_required @user_has_permissions('manage_service') -def verify_reply_to_address_updates(service_id, notification_id): - return jsonify(**get_verify_reply_to_address_partials(service_id, notification_id)) +def service_verify_reply_to_address_updates(service_id, notification_id): + return jsonify(**get_service_verify_reply_to_address_partials(service_id, notification_id)) -def get_verify_reply_to_address_partials(service_id, notification_id): +def get_service_verify_reply_to_address_partials(service_id, notification_id): notification = notification_api_client.get_notification(current_app.config["NOTIFY_SERVICE_ID"], notification_id) verification_status = "pending" is_default = request.args.get('is_default', False) @@ -497,13 +498,23 @@ def service_edit_email_reply_to(service_id, reply_to_email_id): form.email_address.data = reply_to_email_address['email_address'] form.is_default.data = reply_to_email_address['is_default'] if form.validate_on_submit(): - service_api_client.update_reply_to_email_address( - current_service.id, - reply_to_email_id=reply_to_email_id, - email_address=form.email_address.data, - is_default=True if reply_to_email_address['is_default'] else form.is_default.data - ) - return redirect(url_for('.service_email_reply_to', service_id=service_id)) + try: + notification_id = service_api_client.verify_reply_to_email_address( + service_id, form.email_address.data + )["data"]["id"] + except HTTPError as e: + error_msg = "Your service already uses '{}' as an email reply-to address.".format(form.email_address.data) + if e.status_code == 400 and error_msg == e.message: + flash(error_msg, 'error') + return redirect(url_for('.service_email_reply_to', service_id=service_id)) + else: + raise e + return redirect(url_for( + '.service_verify_reply_to_address', + service_id=service_id, + notification_id=notification_id + ) + "?is_default={}".format(True if reply_to_email_address['is_default'] else form.is_default.data)) + if (request.endpoint == "main.service_confirm_delete_email_reply_to"): flash("Are you sure you want to delete this email reply-to address?", 'delete') return render_template( diff --git a/app/navigation.py b/app/navigation.py index b5f2d82fd..6ae866fd8 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -273,6 +273,8 @@ class HeaderNavigation(Navigation): 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', + 'service_verify_reply_to_address', + 'service_verify_reply_to_address_updates', 'services_or_dashboard', 'set_free_sms_allowance', 'set_organisation_type', @@ -393,6 +395,8 @@ class MainNavigation(Navigation): 'service_set_letters', 'service_set_reply_to_email', 'service_set_sms_prefix', + 'service_verify_reply_to_address', + 'service_verify_reply_to_address_updates', 'service_settings', 'service_sms_senders', 'set_free_sms_allowance', @@ -783,6 +787,8 @@ class CaseworkNavigation(Navigation): 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', + 'service_verify_reply_to_address', + 'service_verify_reply_to_address_updates', 'services_or_dashboard', 'set_free_sms_allowance', 'service_set_letter_branding', @@ -1044,6 +1050,8 @@ class OrgNavigation(Navigation): 'service_switch_count_as_live', 'service_switch_live', 'service_set_permission', + 'service_verify_reply_to_address', + 'service_verify_reply_to_address_updates', 'services_or_dashboard', 'set_free_sms_allowance', 'service_set_letter_branding', diff --git a/app/templates/views/service-settings/email-reply-to/_verify-updates.html b/app/templates/views/service-settings/email-reply-to/_verify-updates.html index ad628f077..207742e77 100644 --- a/app/templates/views/service-settings/email-reply-to/_verify-updates.html +++ b/app/templates/views/service-settings/email-reply-to/_verify-updates.html @@ -11,7 +11,7 @@

- Refresh + Refresh

{% elif verification_status == "success" %} {{ banner("Yay success! Your new reply-to email has been added ^__^", type='default', with_tick=True) }} diff --git a/app/templates/views/service-settings/email-reply-to/verify.html b/app/templates/views/service-settings/email-reply-to/verify.html index d79b00080..3d80133aa 100644 --- a/app/templates/views/service-settings/email-reply-to/verify.html +++ b/app/templates/views/service-settings/email-reply-to/verify.html @@ -18,7 +18,7 @@ ) }} {{ ajax_block( partials, - url_for('main.verify_reply_to_address_updates', service_id=service_id, notification_id=notification_id), + url_for('main.service_verify_reply_to_address_updates', service_id=service_id, notification_id=notification_id), 'status', finished=finished ) }} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 2d7dac890..4371ed27e 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -31,7 +31,6 @@ from tests.conftest import ( get_non_default_letter_contact_block, get_non_default_reply_to_email_address, get_non_default_sms_sender, - mock_get_notification, mock_get_service_organisation, multiple_letter_contact_blocks, multiple_reply_to_email_addresses, @@ -2013,20 +2012,22 @@ def test_add_reply_to_email_address_sends_test_notification( ): fixture(mocker) data['email_address'] = "test@example.com" - mock_verify = mocker.patch('app.service_api_client.verify_reply_to_email_address', return_value={"id": "123"}) + mock_verify = mocker.patch( + 'app.service_api_client.verify_reply_to_email_address', return_value={"data": {"id": "123"}} + ) client_request.post( 'main.service_add_email_reply_to', service_id=SERVICE_ONE_ID, _data=data, _expected_status=302, _expected_redirect=url_for( - 'main.verify_reply_to_address', + 'main.service_verify_reply_to_address', service_id=SERVICE_ONE_ID, notification_id="123", _external=True, - ) + "?is_default={}".format(SERVICE_ONE_ID, api_default_args) + ) + "?is_default={}".format(api_default_args) ) - mock_verify.assert_called_once_with("test@example.com") + mock_verify.assert_called_once_with(SERVICE_ONE_ID, "test@example.com") @pytest.mark.parametrize("is_default", [True, False]) @@ -2035,7 +2036,8 @@ def test_add_reply_to_email_address_sends_test_notification( ("sending", 0, 0), ("permanent-failure", 1, 0), ]) -def test_verify_reply_to_address( +@freeze_time("2018-06-01 11:11:00.061258") +def test_service_verify_reply_to_address( mocker, client_request, fake_uuid, status, expected_failure, expected_success, is_default ): notification = { @@ -2044,12 +2046,16 @@ def test_verify_reply_to_address( "to": "email@example.gov.uk", "service_id": SERVICE_ONE_ID, "template_id": TEMPLATE_ONE_ID, - "notification_type": "email" + "notification_type": "email", + "created_at": '2018-06-01T11:10:52.499230+00:00' } mocker.patch('app.notification_api_client.get_notification', return_value=notification) mock_add_reply_to_email_address = mocker.patch('app.service_api_client.add_reply_to_email_address') + mocker.patch( + 'app.service_api_client.get_reply_to_email_addresses', return_value=[] + ) page = client_request.get( - 'main.verify_reply_to_address', + 'main.service_verify_reply_to_address', service_id=SERVICE_ONE_ID, notification_id=notification["id"], _optional_args="?is_default={}".format(is_default) @@ -2071,7 +2077,7 @@ def test_verify_reply_to_address( @freeze_time("2018-06-01 11:11:00.061258") -def test_add_reply_to_email_address_fails_if_notification_not_received(mocker, client_request, fake_uuid): +def test_add_reply_to_email_address_fails_if_notification_not_delivered_in_5_mins(mocker, client_request, fake_uuid): notification = { "id": fake_uuid, "status": "sending", @@ -2084,7 +2090,7 @@ def test_add_reply_to_email_address_fails_if_notification_not_received(mocker, c mocker.patch('app.notification_api_client.get_notification', return_value=notification) mock_add_reply_to_email_address = mocker.patch('app.service_api_client.add_reply_to_email_address') page = client_request.get( - 'main.verify_reply_to_address', + 'main.service_verify_reply_to_address', service_id=SERVICE_ONE_ID, notification_id=notification["id"], _optional_args="?is_default={}".format(False) @@ -2213,8 +2219,10 @@ def test_edit_reply_to_email_address( mocker, fake_uuid, client_request, - mock_update_reply_to_email_address ): + mock_verify = mocker.patch( + 'app.service_api_client.verify_reply_to_email_address', return_value={"data": {"id": "123"}} + ) fixture(mocker) data['email_address'] = "test@example.gov.uk" client_request.post( @@ -2223,13 +2231,7 @@ def test_edit_reply_to_email_address( reply_to_email_id=fake_uuid, _data=data ) - - mock_update_reply_to_email_address.assert_called_once_with( - SERVICE_ONE_ID, - reply_to_email_id=fake_uuid, - email_address="test@example.gov.uk", - is_default=api_default_args - ) + mock_verify.assert_called_once_with(SERVICE_ONE_ID, "test@example.gov.uk") @pytest.mark.parametrize('fixture, expected_link_text, partial_href', [