diff --git a/app/main/views/send.py b/app/main/views/send.py index 39fdba999..d0f311b88 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -601,12 +601,6 @@ def get_back_link(service_id, template_id, step_index): @login_required @user_has_permissions('manage_templates') def check_notification(service_id, template_id): - # go back to start of process - back_link = get_back_link(service_id, template_id, 0) - - if {'recipient', 'placeholders'} - set(session.keys()) and back_link: - return redirect(back_link) - db_template = service_api_client.get_service_template(service_id, template_id)['data'] template = get_template( @@ -614,6 +608,19 @@ def check_notification(service_id, template_id): current_service, show_recipient=True ) + + # go back to start of process + back_link = get_back_link(service_id, template_id, 0) + + if ( + ( + not session.get('recipient') or + not all_placeholders_in_session(template.placeholders) + ) + and back_link + ): + return redirect(back_link) + template.values = get_receipient_and_placeholders_from_session(template.template_type) return render_template( diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 7cb4bde86..ed476a930 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -55,7 +55,7 @@ class NotificationApiClient(NotifyAdminAPIClient): params=params ) - def send_notification(self, service_id, *, template_id, recipient, personalisation=None): + def send_notification(self, service_id, *, template_id, recipient, personalisation): data = { 'template_id': template_id, 'to': recipient, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index aa8fc380b..3df6316f2 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -561,7 +561,7 @@ def _redirects_with_help_argument( ) -def test_send_test_email_message_without_placeholders( +def test_send_test_email_message_without_placeholders_redirects_to_check_page( logged_in_client, mocker, service_one, @@ -571,19 +571,16 @@ def test_send_test_email_message_without_placeholders( mock_get_detailed_service_for_today, fake_uuid, ): - - mocker.patch('app.main.views.send.s3download', return_value='email address\r\ntest@user.gov.uk') + with logged_in_client.session_transaction() as session: + session['recipient'] = 'foo@bar.com' response = logged_in_client.get( - url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid), + url_for('main.send_test', step_index=0, service_id=service_one['id'], template_id=fake_uuid), follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with( - service_one['id'], - {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Report'}, - 'eu-west-1' - ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.select('h1')[0].text.strip() == 'Preview of Two week reminder' def test_send_test_sms_message_with_placeholders_shows_first_field( @@ -778,6 +775,7 @@ def test_send_test_sms_message_puts_submitted_data_in_session( fake_uuid, ): with logged_in_client.session_transaction() as session: + session['recipient'] = '07700 900762' session['placeholders'] = {} response = logged_in_client.post( @@ -802,7 +800,6 @@ def test_send_test_sms_message_puts_submitted_data_in_session( assert session['placeholders']['name'] == 'Jo' - @pytest.mark.parametrize('filetype', ['pdf', 'png']) def test_send_test_works_as_letter_preview( filetype, @@ -866,7 +863,7 @@ def test_send_test_clears_session( assert response.status_code == 302 with logged_in_client.session_transaction() as session: - assert session['recipient'] == None + assert session['recipient'] is None assert session['placeholders'] == {} @@ -1589,3 +1586,143 @@ def test_check_messages_back_from_help_handles_unexpected_templates(client, mock 'main.choose_template', service_id='1234', ) + + +@pytest.mark.parametrize('existing_session_items', [ + {}, + {'recipient': '07700900001'}, + {'name': 'Jo'} +]) +def test_check_notification_redirects_if_session_not_populated( + logged_in_client, + service_one, + fake_uuid, + existing_session_items, + mock_get_service_template_with_placeholders +): + with logged_in_client.session_transaction() as session: + session.update(existing_session_items) + + resp = logged_in_client.get(url_for( + 'main.check_notification', + service_id=service_one['id'], + template_id=fake_uuid + )) + + assert resp.location == url_for( + 'main.view_template', + service_id=service_one['id'], + template_id=fake_uuid, + _external=True + ) + + +def test_check_notification_shows_preview( + client_request, + service_one, + fake_uuid, + mock_get_service_template +): + with client_request.session_transaction() as session: + session['recipient'] = '07700900001' + session['placeholders'] = {} + + page = client_request.get( + 'main.check_notification', + service_id=service_one['id'], + template_id=fake_uuid + ) + + assert page.h1.text.strip() == 'Preview of Two week reminder' + assert ( + page.findAll('a', {'class': 'page-footer-back-link'})[0]['href'] + ) == url_for('main.view_template', service_id=service_one['id'], template_id=fake_uuid) + + +def test_send_notification_submits_data( + client_request, + service_one, + fake_uuid, + mock_send_notification, +): + with client_request.session_transaction() as session: + session['recipient'] = '07700900001' + session['placeholders'] = {'a': 'b'} + + client_request.post( + 'main.send_notification', + service_id=service_one['id'], + template_id=fake_uuid + ) + + mock_send_notification.assert_called_once_with( + service_one['id'], + template_id=fake_uuid, + recipient='07700900001', + personalisation={'a': 'b'} + ) + + +def test_send_notification_clears_session( + client_request, + service_one, + fake_uuid, + mock_send_notification, +): + with client_request.session_transaction() as session: + session['recipient'] = '07700900001' + session['placeholders'] = {'a': 'b'} + + client_request.post( + 'main.send_notification', + service_id=service_one['id'], + template_id=fake_uuid + ) + + with client_request.session_transaction() as session: + assert 'recipient' not in session + assert 'placeholders' not in session + + +def test_send_notification_redirects_if_missing_data( + logged_in_client, + service_one, + fake_uuid, +): + with logged_in_client.session_transaction() as session: + session['placeholders'] = {'a': 'b'} + + resp = logged_in_client.post( + url_for('main.send_notification', service_id=service_one['id'], template_id=fake_uuid) + ) + + assert resp.status_code == 302 + assert resp.location == url_for( + '.send_one_off', + service_id=service_one['id'], + template_id=fake_uuid, + _external=True + ) + + +def test_send_notification_redirects_to_view_page( + logged_in_client, + service_one, + fake_uuid, + mock_send_notification, +): + with logged_in_client.session_transaction() as session: + session['recipient'] = '07700900001' + session['placeholders'] = {'a': 'b'} + + resp = logged_in_client.post( + url_for('main.send_notification', service_id=service_one['id'], template_id=fake_uuid) + ) + + assert resp.status_code == 302 + assert resp.location == url_for( + '.view_notification', + service_id=service_one['id'], + notification_id=fake_uuid, + _external=True + ) diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index fcea96dd5..4f9750a05 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -37,7 +37,7 @@ def test_client_gets_notifications_for_service_and_job_by_page(mocker, arguments def test_send_notification(mocker, logged_in_client, active_user_with_permissions): mock_post = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.post') - NotificationApiClient().send_notification('foo', template_id='bar', recipient='07700900001') + NotificationApiClient().send_notification('foo', template_id='bar', recipient='07700900001', personalisation=None) mock_post.assert_called_once_with( url='/service/foo/send-notification', data={ diff --git a/tests/conftest.py b/tests/conftest.py index 410ccb5a5..342134e26 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,4 @@ - +from contextlib import contextmanager import os from datetime import date, datetime, timedelta from unittest.mock import Mock @@ -61,11 +61,6 @@ def service_with_reply_to_addresses(api_user_active): ) -@pytest.fixture(scope='function') -def mock_send_sms(request, mocker): - return mocker.patch("app.service_api_client.send_sms") - - @pytest.fixture(scope='function') def fake_uuid(): return sample_uuid() @@ -1665,6 +1660,19 @@ def mock_get_notification(mocker, fake_uuid, notification_status='delivered'): ) +@pytest.fixture +def mock_send_notification(mocker, fake_uuid): + def _send_notification( + service_id, *, template_id, recipient, personalisation + ): + return {'id': fake_uuid} + + return mocker.patch( + 'app.notification_api_client.send_notification', + side_effect=_send_notification + ) + + @pytest.fixture(scope='function') def client(app_): with app_.test_request_context(), app_.test_client() as client: @@ -1712,6 +1720,12 @@ def os_environ(): def client_request(logged_in_client): class ClientRequest: + @staticmethod + @contextmanager + def session_transaction(): + with logged_in_client.session_transaction() as session: + yield session + @staticmethod def get(endpoint, _expected_status=200, _follow_redirects=False, **endpoint_kwargs): resp = logged_in_client.get(