Skip to content

Conversation

mgu
Copy link
Contributor

@mgu mgu commented Feb 28, 2017

As a field is not mandatory
If empty, it should be accepted

@mgu mgu self-assigned this Feb 28, 2017
@mgu mgu requested review from brunobord and wo0dyn February 28, 2017 11:04
super(BaseDynamicForm, self).__init__(*args, **kwargs)
self._bound_fields_cache = FormidableBoundFieldCache()

def rule_has_empty_fields(self, rule, data):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put this kind of check in the rule itself ? Maybe for specific needed the rules can override its method in order to configure a specific check.

Copy link
Contributor Author

@mgu mgu Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes you don't want the rule to be aware of the form/fields and sometimes you want :)
I can move that code in the Presets class but it will change the run method signature

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Sorry.
(I had a previous implementation in mind...)

@moumoutte moumoutte dismissed a stale review February 28, 2017 12:52

rule_has_empty belongs to rule class.

return any((
is_empty_value(data.get(name, None))
for name in used_fields
))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: you don't need double-parenthesis here 😉

return any(
    is_empty_value(data.get(name, None))
    for name in used_fields
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks 👍

@mgu mgu requested a review from moumoutte February 28, 2017 13:29
formidable = TestPresets.to_formidable(label='presets')
form_class = formidable.get_django_form_class(role='jedi')
form = form_class(data={'left': '', 'right': ''})
self.assertFalse(form.is_valid(), form.errors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the test ... The field are required so the form validation will be false.

I would like to see a test like that

=> Define a Validation Between two fields required for the Jedi
=> Get the dynamic_form_class for a padawan
=> Fill the form with empty value
=> validate the form (it has to be okay).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some assertions in this test and a new test that matches your description

Copy link
Contributor

@moumoutte moumoutte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! LGTM

@mgu mgu force-pushed the skip-presets-on-empty-fields branch from ad3988e to 82080b1 Compare March 1, 2017 08:11
@mgu mgu merged commit 23031be into master Mar 1, 2017
@mgu mgu deleted the skip-presets-on-empty-fields branch March 1, 2017 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants