From d2680fe885e99c3b211fc6494ced459855664380 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 21 Dec 2016 14:55:49 +0000 Subject: [PATCH] Require email address for reporting problems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you report a problem we want to be able to get back to you to find out more information, or to update you on the status of a fix. So it shouldn’t be possible to report a problem without providing an email address. This commit makes `email_address` a required field when `ticket_type` is problem. This requires a bit of fiddling with the tests which weren’t expecting to have to provide an email address. So the tests now either: - pass an email address - check for an error when they don’t pass an email address --- app/main/forms.py | 6 ++- app/main/views/feedback.py | 22 +++++---- app/templates/views/support/problem.html | 2 - tests/app/main/views/test_feedback.py | 60 ++++++++++++++++-------- 4 files changed, 58 insertions(+), 32 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index daf6f53b1..614dc1e5c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -403,12 +403,16 @@ class SupportType(Form): ) -class Support(Form): +class Feedback(Form): name = StringField('Name') email_address = StringField('Email address') feedback = TextAreaField('Your message', validators=[DataRequired(message="Can’t be empty")]) +class Problem(Feedback): + email_address = email_address(label='Email address', gov_user=False) + + class Triage(Form): severe = RadioField( 'Is it an emergency?', diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index d7a5ce94d..02290eb0f 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -4,7 +4,7 @@ from flask import render_template, url_for, redirect, flash, current_app, abort, from flask_login import current_user from app import convert_to_boolean, current_service, service_api_client from app.main import main -from app.main.forms import SupportType, Support, Triage +from app.main.forms import SupportType, Feedback, Problem, Triage from datetime import datetime @@ -37,10 +37,14 @@ def triage(): @main.route('/support/submit/', methods=['GET', 'POST']) def feedback(ticket_type): - if ticket_type not in ['question', 'problem']: + try: + form = { + 'question': Feedback, + 'problem': Problem, + }[ticket_type]() + except KeyError: abort(404) - form = Support() if not form.feedback.data: form.feedback.data = session.pop('feedback_message', '') @@ -63,13 +67,13 @@ def feedback(ticket_type): if needs_escalation(ticket_type, severe): return redirect(url_for('.bat_phone')) + if current_user.is_authenticated: + form.email_address.data = current_user.email_address + form.name.data = current_user.name + if form.validate_on_submit(): - if current_user.is_authenticated: - user_email = current_user.email_address - user_name = current_user.name - else: - user_email = form.email_address.data - user_name = form.name.data or None + user_email = form.email_address.data + user_name = form.name.data or None feedback_msg = 'Environment: {}\n{}\n{}'.format( url_for('main.index', _external=True), '' if user_email else '{} (no email address supplied)'.format(form.name.data), diff --git a/app/templates/views/support/problem.html b/app/templates/views/support/problem.html index e50c88ae7..e171d13cb 100644 --- a/app/templates/views/support/problem.html +++ b/app/templates/views/support/problem.html @@ -23,8 +23,6 @@
{{ textbox(form.feedback, width='1-1', hint='', rows=10) }} {% if not current_user.is_authenticated %} -

Do you want a reply?

-

Leave your details below if you'd like a response.

{{ textbox(form.name, width='1-1') }} {{ textbox(form.email_address, width='1-1') }} {% else %} diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index 49abd9d9f..40006ea75 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -30,7 +30,7 @@ def test_get_support_index_page(client): @pytest.mark.parametrize('logged_in, expected_form_field, expected_contact_details', [ (True, type(None), 'We’ll reply to test@user.gov.uk'), (True, type(None), 'We’ll reply to test@user.gov.uk'), - (False, element.Tag, 'Leave your details below if you\'d like a response.'), + (False, element.Tag, None), ]) def test_choose_support_type( client, @@ -54,7 +54,8 @@ def test_choose_support_type( assert page.h1.string.strip() == expected_h1 assert isinstance(page.find('input', {'name': 'name'}), expected_form_field) assert isinstance(page.find('input', {'name': 'email_address'}), expected_form_field) - assert page.find('form').find('p').text.strip() == expected_contact_details + if expected_contact_details: + assert page.find('form').find('p').text.strip() == expected_contact_details @freeze_time('2016-12-12 12:00:00.000000') @@ -70,22 +71,6 @@ def test_get_feedback_page(client, ticket_type, expected_status_code): @freeze_time("2016-12-12 12:00:00.000000") @pytest.mark.parametrize('data, expected_message, expected_person_name, expected_email, logged_in, is_anonymous', [ - ( - {'feedback': "blah", 'name': 'Fred'}, - 'Environment: http://localhost/\nFred (no email address supplied)\nblah', - 'Fred', - 'donotreply@notifications.service.gov.uk', - False, - True, - ), - ( - {'feedback': "blah"}, - 'Environment: http://localhost/\n (no email address supplied)\nblah', - None, - 'donotreply@notifications.service.gov.uk', - False, - True, - ), ( {'feedback': "blah", 'name': "Steve Irwin", 'email_address': 'rip@gmail.com'}, 'Environment: http://localhost/\n\nblah', @@ -104,7 +89,7 @@ def test_get_feedback_page(client, ticket_type, expected_status_code): ), ]) @pytest.mark.parametrize('ticket_type', ['problem', 'question']) -def test_post_problem_or_question( +def test_passes_user_details_through_flow( client, api_user_active, mock_get_user, @@ -146,6 +131,41 @@ def test_post_problem_or_question( ) +@freeze_time('2016-12-12 12:00:00.000000') +@pytest.mark.parametrize('data', [ + {'feedback': 'blah', 'name': 'Fred'}, + {'feedback': 'blah'}, +]) +@pytest.mark.parametrize('ticket_type, expected_response, expected_redirect, expected_error', [ + ('problem', 200, no_redirect(), element.Tag), + ('question', 302, partial(url_for, 'main.thanks', urgent=True, anonymous=True), type(None)), +]) +def test_email_address_required_for_problems( + client, + api_user_active, + mock_get_user, + mock_get_services, + mocker, + data, + ticket_type, + expected_response, + expected_redirect, + expected_error +): + mock_post = mocker.patch( + 'app.main.views.feedback.requests.post', + return_value=Mock(status_code=201) + ) + response = client.post( + url_for('main.feedback', ticket_type=ticket_type), + data=data, + ) + assert response.status_code == expected_response + assert response.location == expected_redirect(_external=True) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert isinstance(page.find('span', {'class': 'error-message'}), expected_error) + + @pytest.mark.parametrize('ticket_type, severe, is_in_business_hours, numeric_urgency, is_urgent', [ # business hours, always urgent @@ -179,7 +199,7 @@ def test_urgency( mock_post = mocker.patch('app.main.views.feedback.requests.post', return_value=Mock(status_code=201)) response = logged_in_client.post( url_for('main.feedback', ticket_type=ticket_type, severe=severe), - data={'feedback': "blah"}, + data={'feedback': 'blah', 'email_address': 'test@example.com'}, ) assert response.status_code == 302 assert response.location == url_for('main.thanks', urgent=is_urgent, anonymous=False, _external=True)