From 2fcf278a5b701bdf6296a435eccae7c2ae408ff8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Dec 2018 17:29:23 +0000 Subject: [PATCH] Use unambiguously magic value for `None` choices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WTForms coerces `None` as a choice to `'None'` as a string when rendering form fields (form fields will only ever have string data because that what the browser posts back). But internally WTForms coerces `None` to mean an unset value, ie where the user hasn’t selected a radio button: https://github.com/wtforms/wtforms/blob/283b2803206825158834f1828bbf749c129b7c47/src/wtforms/utils.py#L1-L20 We shouldn’t use `None` to mean two different things. And in fact we can’t, because it in effect means that we’re always getting a value for the `move_to` field, even if the user hasn’t chosen to move any templates. Which results in some very expected behaviour. --- app/main/forms.py | 32 +++++++++++-------- tests/app/main/views/test_service_settings.py | 7 ++-- tests/app/main/views/test_template_folders.py | 17 +++++++++- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 80ded47e9..d46815e8d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -707,24 +707,26 @@ class ServicePostageForm(StripWhitespaceForm): class FieldWithNoneOption(): - # This needs to match the data the browser will post from - # - NONE_OPTION_VALUE = 'None' + # This is a special value that is specific to our forms. This is + # more expicit than casting `None` to a string `'None'` which can + # have unexpected edge cases + NONE_OPTION_VALUE = '__NONE__' + # When receiving Python data, eg when instantiating the form object + # we want to convert that data to our special value, so that it gets + # recognised as being one of the valid choices + def process_data(self, value): + self.data = self.NONE_OPTION_VALUE if value is None else value + + # After validation we want to convert it back to a Python `None` for + # use elsewhere, eg posting to the API def post_validate(self, form, validation_stopped): - if self.data == self.NONE_OPTION_VALUE: + if self.data == self.NONE_OPTION_VALUE and not validation_stopped: self.data = None class RadioFieldWithNoneOption(FieldWithNoneOption, RadioField): - - def validate(self, *args, **kwargs): - - if self.data == self.NONE_OPTION_VALUE: - self.data = None - return True - - return super().validate(*args, **kwargs) + pass class HiddenFieldWithNoneOption(FieldWithNoneOption, HiddenField): @@ -1194,10 +1196,13 @@ class TemplateAndFoldersSelectionForm(Form): self.op = None self.is_move_op = self.is_add_folder_op = self.is_add_template_op = False + if current_folder_id is None: + current_folder_id = RadioFieldWithNoneOption.NONE_OPTION_VALUE + self.move_to.choices = [ (item['id'], item['name']) for item in ([self.ALL_TEMPLATES_FOLDER] + all_template_folders) - if item['id'] != str(current_folder_id) + if item['id'] != current_folder_id ] self.add_template_by_template_type.choices = list(filter(None, [ @@ -1237,5 +1242,6 @@ class TemplateAndFoldersSelectionForm(Form): move_to_new_folder_name = StringField('Folder name', validators=[required_for_ops('move_to_new_folder')]) add_template_by_template_type = RadioFieldWithNoneOption('Add new', validators=[ + Optional(), required_for_ops('add_template') ]) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 391051cdb..53e62e402 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2188,12 +2188,12 @@ def test_set_postage_saves( @pytest.mark.parametrize('current_branding, expected_values, expected_labels', [ (None, [ - 'None', '1', '2', '3', '4', '5', + '__NONE__', '1', '2', '3', '4', '5', ], [ 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5' ]), ('5', [ - '5', 'None', '1', '2', '3', '4', + '5', '__NONE__', '1', '2', '3', '4', ], [ 'org 5', 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', ]), @@ -2314,7 +2314,8 @@ def test_should_preview_email_branding( @pytest.mark.parametrize('posted_value, submitted_value', ( ('1', '1'), - ('None', None), + ('__NONE__', None), + pytest.param('None', None, marks=pytest.mark.xfail(raises=AssertionError)), )) def test_should_set_branding_and_organisations( logged_in_platform_admin_client, diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index b4c5e97b4..6f0ff26d2 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -901,7 +901,7 @@ def test_should_be_able_to_move_a_sub_item( template_folder_id=PARENT_FOLDER_ID, _data={ 'operation': 'move_to_existing_folder', - 'move_to': 'None', + 'move_to': '__NONE__', 'templates_and_folders': [GRANDCHILD_FOLDER_ID], }, _expected_status=302, @@ -929,6 +929,13 @@ def test_should_be_able_to_move_a_sub_item( 'move_to_new_folder_name': 'foo', 'move_to': PARENT_FOLDER_ID }, + # move to existing, but no templates to move + { + 'operation': 'move_to_existing_folder', + 'templates_and_folders': [], + 'move_to_new_folder_name': '', + 'move_to': PARENT_FOLDER_ID + }, # move to new, but nothing selected to move { 'operation': 'move_to_new_folder', @@ -944,6 +951,14 @@ def test_should_be_able_to_move_a_sub_item( 'move_to': PARENT_FOLDER_ID, 'add_template_by_template_type': 'email', }, + # add a new template, but also move to root folder + { + 'operation': 'add_template', + 'templates_and_folders': [], + 'move_to_new_folder_name': '', + 'move_to': '__NONE__', + 'add_template_by_template_type': 'email', + }, ]) def test_no_action_if_user_fills_in_ambiguous_fields( client_request,