Skip to content

Commit 698eb06

Browse files
brunobordbrunobord
authored
Merge pull request #301 from peopledoc/multiple-conditions-for-one-field-bb
Add support for multiple conditions to target a common field
2 parents 32e0eb5 + 958c506 commit 698eb06

File tree

7 files changed

+209
-35
lines changed

7 files changed

+209
-35
lines changed

‎CHANGELOG.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ master (unreleased)
88
- Added tests against the ``formidable.yml`` schema definition of Forms (#295).
99
- Fixed various items in the schema definition (#297).
1010
- Validation endpoint for **user data** doesn't allow GET method anymore (#300).
11+
- Add support for multiple conditions to target a common field.
1112

1213
Release 1.3.0 (2018-02-14)
1314
==========================

‎demo/tests/test_conditions.py

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,123 @@ def get_form_class(self, formidable, role):
251251
return get_dynamic_form_class_from_schema(schema)
252252

253253

254+
class MultipleConditionsTestCase(TestCase):
255+
256+
def get_form_class(self, formidable, role):
257+
return get_dynamic_form_class(formidable, role)
258+
259+
def setUp(self):
260+
conditions_schema = [
261+
{
262+
'name': 'display A',
263+
'action': 'display_iff',
264+
'fields_ids': ['a'],
265+
'tests': [
266+
{
267+
'field_id': 'checkbox_a',
268+
'operator': 'eq',
269+
'values': [True],
270+
}
271+
]
272+
},
273+
{
274+
'name': 'display AB',
275+
'action': 'display_iff',
276+
'fields_ids': ['a', 'b'],
277+
'tests': [
278+
{
279+
'field_id': 'checkbox_ab',
280+
'operator': 'eq',
281+
'values': [True],
282+
}
283+
]
284+
}
285+
286+
]
287+
288+
class TestForm(FormidableForm):
289+
checkbox_a = fields.BooleanField(
290+
label='My checkbox',
291+
accesses={'padawan': constants.EDITABLE,
292+
'jedi': constants.EDITABLE,
293+
'human': constants.EDITABLE,
294+
'robot': constants.REQUIRED}
295+
)
296+
checkbox_ab = fields.BooleanField(
297+
label='My checkbox 2',
298+
accesses={'padawan': constants.EDITABLE,
299+
'jedi': constants.EDITABLE,
300+
'human': constants.EDITABLE,
301+
'robot': constants.REQUIRED}
302+
)
303+
a = fields.CharField(
304+
label='a',
305+
accesses={'padawan': constants.EDITABLE,
306+
'jedi': constants.EDITABLE,
307+
'human': constants.REQUIRED,
308+
'robot': constants.REQUIRED}
309+
)
310+
b = fields.CharField(
311+
label='b',
312+
accesses={'padawan': constants.EDITABLE,
313+
'jedi': constants.EDITABLE,
314+
'human': constants.READONLY,
315+
'robot': constants.REQUIRED}
316+
)
317+
318+
self.formidable = TestForm.to_formidable(label='title')
319+
self.formidable.conditions = conditions_schema
320+
321+
def test_a_and_ab_checked(self):
322+
form_class = self.get_form_class(self.formidable, 'jedi')
323+
data = {
324+
'a': 'A',
325+
'b': 'B',
326+
'checkbox_a': True,
327+
'checkbox_ab': True
328+
}
329+
330+
form = form_class(data)
331+
self.assertTrue(form.is_valid())
332+
self.assertTrue('a' in form.cleaned_data)
333+
self.assertTrue('b' in form.cleaned_data)
334+
335+
def test_a_only_checked(self):
336+
form_class = self.get_form_class(self.formidable, 'jedi')
337+
data = {
338+
'a': 'A',
339+
'checkbox_a': True
340+
}
341+
form = form_class(data)
342+
self.assertTrue(form.is_valid())
343+
self.assertTrue('a' in form.cleaned_data)
344+
self.assertTrue('b' not in form.cleaned_data)
345+
346+
def test_ab_only_checked(self):
347+
form_class = self.get_form_class(self.formidable, 'jedi')
348+
data = {
349+
'a': 'A',
350+
'b': 'B',
351+
'checkbox_ab': True
352+
}
353+
354+
form = form_class(data)
355+
self.assertTrue(form.is_valid())
356+
self.assertTrue('a' in form.cleaned_data)
357+
self.assertTrue('b' in form.cleaned_data)
358+
359+
def test_none_checked(self):
360+
form_class = self.get_form_class(self.formidable, 'jedi')
361+
data = {
362+
'checkbox_ab': False
363+
}
364+
365+
form = form_class(data)
366+
self.assertTrue(form.is_valid())
367+
self.assertTrue('a' not in form.cleaned_data)
368+
self.assertTrue('b' not in form.cleaned_data)
369+
370+
254371
class ConditionSerializerTestCase(TestCase):
255372

256373
payload = {

‎demo/tests/test_end_point.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -915,11 +915,9 @@ def test_create_form_conditions_invalid_dup(self):
915915
data['fields'] = copy.deepcopy(self.fields_with_validation)
916916
data['conditions'] = copy.deepcopy(self.valid_conditions_invalid_dup)
917917
serializer = FormidableSerializer(data=data)
918-
self.assertFalse(serializer.is_valid())
919-
self.assertEqual(
920-
serializer.errors['non_field_errors'][0],
921-
'Action display_iff in condition (my condition) is used many times for the same fields (input-date)' # noqa
922-
)
918+
# TODO decide if the validation should fail
919+
# now that we allow multiple conditions for one field
920+
self.assertTrue(serializer.is_valid())
923921

924922
def test_create_field(self):
925923
data = copy.deepcopy(self.data)

‎docs/source/forms.rst

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,17 @@ Conditions
189189

190190
.. automodule:: formidable.forms.conditions
191191

192+
.. important::
193+
194+
As of ``1.4.0``, it is allowed to have several conditional display rules that target a common field. In case of "conflict" between these rules, priority goes to the *display*, rather than the *hide* action.
195+
196+
e.g.:
197+
198+
* Rule 1 says: "if checkbox-1 is checked, then display field X"
199+
* Rule 2 says: "if checkbox-2 is checked, then display field X and Y"
200+
201+
if only checkbox-1 is checked, field X will be displayed, even if checkbox-2 is unchecked, and vice-versa. If both are checked, fields X and Y will be displayed. If none is checked, fields X and Y will be hidden.
202+
192203
Python builder
193204
==============
194205

‎formidable/forms/__init__.py

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,44 @@ def __init__(self, *args, **kwargs):
4848
super(BaseDynamicForm, self).__init__(*args, **kwargs)
4949
self._bound_fields_cache = FormidableBoundFieldCache()
5050

51+
def get_removed_fields(self, cleaned_data):
52+
"""
53+
Build the list of fields to be removed due to conditional displays
54+
"""
55+
# build a catalog of fields **targeted** by the conditions
56+
condition_targets = {}
57+
58+
# For each condition, extract its status (should I display or not)
59+
for condition in self._conditions:
60+
# should we keep these fields?
61+
keep_fields = condition.keep_fields(cleaned_data)
62+
for field_id in condition.fields_ids:
63+
# Fill the catalog
64+
if field_id not in condition_targets:
65+
condition_targets[field_id] = []
66+
condition_targets[field_id].append(keep_fields)
67+
# Here, the catalog contains fields targeted by 1 or many conditions.
68+
69+
# If only one condition says "please display X", we'll keep X
70+
# That's why we gather the conditions using "any"
71+
condition_targets = {k: any(v) for k, v in condition_targets.items()}
72+
# We'll only remove fields that are targeted by conditions **and**
73+
# those conditions are false
74+
return (k for k, v in condition_targets.items() if not v)
75+
5176
def clean(self):
5277
cleaned_data = super(BaseDynamicForm, self).clean()
53-
for condition in self._conditions:
54-
cleaned_data = condition(self, cleaned_data)
78+
79+
removed_fields = self.get_removed_fields(cleaned_data)
80+
for field_id in removed_fields:
81+
# Remove field from cleaned_data
82+
cleaned_data.pop(field_id, None)
83+
# Remove from eventual existing errors
84+
self.errors.pop(field_id, None)
85+
# The field might have been removed if it was a file field.
86+
if field_id in self.fields:
87+
del self.fields[field_id]
88+
5589
return cleaned_data
5690

5791

‎formidable/forms/conditions.py

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,13 @@ def __call__(self, cleaned_data):
110110
else:
111111
return meth(ref_value, self.values)
112112

113+
def __repr__(self):
114+
return '<ConditionTest: {field} {operator} {values}>'.format(
115+
field=self.field_id,
116+
operator=self.operator,
117+
values=self.values
118+
)
119+
113120

114121
class Condition(six.with_metaclass(ConditionsMetaClass)):
115122
"""
@@ -129,24 +136,30 @@ def __repr__(self):
129136
action=self.action,
130137
name=self.name)
131138

139+
def keep_fields(self, cleaned_data):
140+
"""
141+
Return ``True`` if the conditions require the fields to be displayed.
142+
"""
143+
raise NotImplementedError(
144+
"Your conditions should have a `keep_fields` method")
145+
132146

133147
class DisplayIffCondition(Condition):
134148
"""
135149
Display field(s) if and only if the conditions in `tests` are all True
136150
"""
137151
action = 'display_iff'
138152

139-
def __call__(self, form, cleaned_data):
140-
# Check if the conditions are True
153+
def keep_fields(self, cleaned_data):
154+
"""
155+
Return ``True`` if the conditions require the fields to be displayed.
156+
"""
141157
is_displayed = all(test(cleaned_data) for test in self.tests)
158+
return is_displayed
142159

143-
# if not, we need to remove the fields from `cleaned_data` and
144-
# `form.errors`
145-
if not is_displayed:
146-
for field_id in self.fields_ids:
147-
cleaned_data.pop(field_id, None)
148-
form.errors.pop(field_id, None)
149-
# The field might have been removed if it was a file field.
150-
if field_id in form.fields:
151-
del form.fields[field_id]
152-
return cleaned_data
160+
def __repr__(self):
161+
return "<Condition {name}: Display {fields} if {tests}>".format(
162+
fields=self.fields_ids,
163+
tests=self.tests,
164+
action=self.action,
165+
name=self.name)

‎formidable/serializers/forms.py

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from __future__ import unicode_literals
44

55
import copy
6-
from collections import Counter, defaultdict
6+
from collections import defaultdict
77

88
from django.db import transaction
99

@@ -109,21 +109,21 @@ def check_conditions_cohesion(self, data):
109109
)
110110
)
111111

112-
# 2/ check there is no more than one rule on a field per action
113-
for action, fields_ids in targets_action.items():
114-
counter = Counter(fields_ids)
115-
duplicates = [
116-
field for field, count in counter.items()
117-
if count > 1
118-
]
119-
if duplicates:
120-
raise ValidationError(
121-
'Action {action} in condition ({name}) is used many times for the same fields ({ids})'.format( # noqa
122-
name=condition['name'],
123-
action=action,
124-
ids=', '.join(duplicates)
125-
)
126-
)
112+
# # 2/ check there is no more than one rule on a field per action
113+
# for action, fields_ids in targets_action.items():
114+
# counter = Counter(fields_ids)
115+
# duplicates = [
116+
# field for field, count in counter.items()
117+
# if count > 1
118+
# ]
119+
# if duplicates:
120+
# raise ValidationError(
121+
# 'Action {action} in condition ({name}) is used many times for the same fields ({ids})'.format( # noqa
122+
# name=condition['name'],
123+
# action=action,
124+
# ids=', '.join(duplicates)
125+
# )
126+
# )
127127

128128
return data
129129

0 commit comments

Comments
 (0)