From c18a38d4f172f0cdf197e70f43ca673a2afaea14 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 12 Nov 2018 09:04:39 +0000
Subject: [PATCH] Make handling of `None` values in forms clearer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If the browser posts the value of `<input value='None'>` to the server
it does so as a string.

We want to post a value of `None` (actually JSON `null`) to the API. To
do this we:
- set the value in the form class to `'None'` (ie a string)
- convert to `None` (as a type) afterwards

However seeing `x = 'None'` in code looks a bit like a mistake. So to
make sure it looks deliberate and clear what is happening this commit:
- makes a reusable constant for `'None'`
- adds a comment explaining why it’s a string
---
 app/main/forms.py                             | 22 ++++++++++++++-----
 app/main/views/service_settings.py            |  3 +--
 tests/app/main/views/test_service_settings.py | 10 +++++++--
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/app/main/forms.py b/app/main/forms.py
index b50d1aadf..c347d714e 100644
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -702,13 +702,25 @@ class ServicePostageForm(StripWhitespaceForm):
     )
 
 
-class RadioFieldWithNoneOption(RadioField):
+class FieldWithNoneOption():
+
+    # This needs to match the data the browser will post from
+    # <input value='None'>
+    NONE_OPTION_VALUE = 'None'
 
     def post_validate(self, form, validation_stopped):
-        if self.data == 'None':
+        if self.data == self.NONE_OPTION_VALUE:
             self.data = None
 
 
+class RadioFieldWithNoneOption(FieldWithNoneOption, RadioField):
+    pass
+
+
+class HiddenFieldWithNoneOption(FieldWithNoneOption, HiddenField):
+    pass
+
+
 class ServiceSetBranding(StripWhitespaceForm):
 
     branding_style = RadioFieldWithNoneOption(
@@ -718,7 +730,7 @@ class ServiceSetBranding(StripWhitespaceForm):
         ]
     )
 
-    DEFAULT = ('None', 'GOV.UK')
+    DEFAULT = (FieldWithNoneOption.NONE_OPTION_VALUE, 'GOV.UK')
 
     def __init__(self, all_email_brandings, current_email_branding):
 
@@ -736,7 +748,7 @@ class ServiceSetBranding(StripWhitespaceForm):
 
 class ServicePreviewBranding(StripWhitespaceForm):
 
-    branding_style = HiddenField('branding_style')
+    branding_style = HiddenFieldWithNoneOption('branding_style')
 
 
 class GovernmentDomainField(StringField):
@@ -1124,7 +1136,7 @@ class TemplateAndFoldersSelectionForm(Form):
 
     ALL_TEMPLATES_FOLDER = {
         'name': 'All templates',
-        'id': 'None',
+        'id': RadioFieldWithNoneOption.NONE_OPTION_VALUE,
     }
 
     def __init__(
diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py
index 1a7786faf..c3d7100f5 100644
--- a/app/main/views/service_settings.py
+++ b/app/main/views/service_settings.py
@@ -813,9 +813,8 @@ def service_preview_email_branding(service_id):
     form = ServicePreviewBranding(branding_style=branding_style)
 
     if form.validate_on_submit():
-        branding_style = None if form.branding_style.data == 'None' else form.branding_style.data
         current_service.update(
-            email_branding=branding_style
+            email_branding=form.branding_style.data
         )
         return redirect(url_for('.service_settings', service_id=service_id))
 
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index 61a1239a0..9b6896887 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -2304,17 +2304,23 @@ def test_should_preview_email_branding(
     app.service_api_client.get_service.assert_called_once_with(service_one['id'])
 
 
+@pytest.mark.parametrize('posted_value, submitted_value', (
+    ('1', '1'),
+    ('None', None),
+))
 def test_should_set_branding_and_organisations(
     logged_in_platform_admin_client,
     service_one,
     mock_update_service,
+    posted_value,
+    submitted_value,
 ):
     response = logged_in_platform_admin_client.post(
         url_for(
             'main.service_preview_email_branding', service_id=service_one['id']
         ),
         data={
-            'branding_style': '1'
+            'branding_style': posted_value
         }
     )
     assert response.status_code == 302
@@ -2323,7 +2329,7 @@ def test_should_set_branding_and_organisations(
 
     mock_update_service.assert_called_once_with(
         service_one['id'],
-        email_branding='1'
+        email_branding=submitted_value
     )