mirror of
https://github.com/GSA/notifications-admin.git
synced 2026-09-11 10:28:41 -04:00
Use unambiguously magic value for None choices
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.
This commit is contained in:
+19
-13
@@ -707,24 +707,26 @@ class ServicePostageForm(StripWhitespaceForm):
|
||||
|
||||
class FieldWithNoneOption():
|
||||
|
||||
# This needs to match the data the browser will post from
|
||||
# <input value='None'>
|
||||
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')
|
||||
])
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user