Require email address for reporting problems

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

@@ -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?',

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/<ticket_type>', 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),

app/templates/views/support/problem.html

@@ -23,8 +23,6 @@
         <form method="post">
             {{ textbox(form.feedback, width='1-1', hint='', rows=10) }}
             {% if not current_user.is_authenticated %}
-              <h3 class="heading-medium">Do you want a reply?</h3>
-              <p>Leave your details below if you'd like a response.</p>
               {{ textbox(form.name, width='1-1') }}
               {{ textbox(form.email_address, width='1-1') }}
             {% else %}

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)