From 184a9fa605b635b4b4ec2d33723d76089afedd6a Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 7 Dec 2018 12:23:12 +0000
Subject: [PATCH] Use email fields for feedback form
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Otherwise we can end up collecting invalid email addresses…

This required some refactoring to allow our email fields to be optional
(but not by default).
---
 app/main/forms.py                     | 13 ++++++++-----
 app/main/validators.py                |  9 +++++++++
 tests/app/main/views/test_feedback.py | 25 +++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/app/main/forms.py b/app/main/forms.py
index 08498f1cf..ccb11dae5 100644
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -110,15 +110,18 @@ class MultiCheckboxField(SelectMultipleField):
     option_widget = CheckboxInput()
 
 
-def email_address(label='Email address', gov_user=True):
+def email_address(label='Email address', gov_user=True, required=True):
+
     validators = [
-        Length(min=5, max=255),
-        DataRequired(message='Can’t be empty'),
-        ValidEmail()
+        ValidEmail(),
     ]
 
     if gov_user:
         validators.append(ValidGovEmail())
+
+    if required:
+        validators.append(DataRequired(message='Can’t be empty'))
+
     return EmailField(label, validators)
 
 
@@ -561,7 +564,7 @@ class SupportType(StripWhitespaceForm):
 
 class Feedback(StripWhitespaceForm):
     name = StringField('Name')
-    email_address = StringField('Email address')
+    email_address = email_address(label='Email address', gov_user=False, required=False)
     feedback = TextAreaField('Your message', validators=[DataRequired(message="Can’t be empty")])
 
 
diff --git a/app/main/validators.py b/app/main/validators.py
index ea24bb95f..9b27e09e9 100644
--- a/app/main/validators.py
+++ b/app/main/validators.py
@@ -38,6 +38,10 @@ class CsvFileValidator:
 class ValidGovEmail:
 
     def __call__(self, form, field):
+
+        if field.data == '':
+            return
+
         from flask import url_for
         message = (
             'Enter a government email address.'
@@ -53,10 +57,15 @@ class ValidEmail(Email):
         super().__init__('Enter a valid email address')
 
     def __call__(self, form, field):
+
+        if field.data == '':
+            return
+
         try:
             validate_email_address(field.data)
         except InvalidEmailError:
             raise ValidationError(self.message)
+
         return super().__call__(form, field)
 
 
diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py
index 622c488ba..e6595d19e 100644
--- a/tests/app/main/views/test_feedback.py
+++ b/tests/app/main/views/test_feedback.py
@@ -17,6 +17,7 @@ from tests.conftest import (
     mock_get_services,
     mock_get_services_with_no_services,
     mock_get_services_with_one_service,
+    normalize_spaces,
 )
 
 
@@ -224,6 +225,30 @@ def test_email_address_required_for_problems(
     assert isinstance(page.find('span', {'class': 'error-message'}), expected_error)
 
 
+@freeze_time('2016-12-12 12:00:00.000000')
+@pytest.mark.parametrize('ticket_type', (
+    PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE
+))
+def test_email_address_must_be_valid_if_provided_to_support_form(
+    client,
+    mocker,
+    ticket_type,
+):
+    response = client.post(
+        url_for('main.feedback', ticket_type=ticket_type),
+        data={
+            'feedback': 'blah',
+            'email_address': 'not valid',
+        },
+    )
+    assert response.status_code == 200
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+
+    assert normalize_spaces(page.select_one('span.error-message').text) == (
+        'Enter a valid email address'
+    )
+
+
 @pytest.mark.parametrize('ticket_type, severe, is_in_business_hours, is_urgent, is_p1', [
 
     # business hours, always urgent, never p1