Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validator support to condition registration and FlagStateForm #61

Merged
merged 6 commits into from Jun 10, 2020

Conversation

willbarton
Copy link
Member

@willbarton willbarton commented Apr 17, 2020

Closes #59. Flag state values were not previously validated in the FlagStateForm, resulting in potential exceptions being raised on flag state checks.

Implementation is loosely based on the code suggested by @Weltraum.

This change supports adding a validator callable when registering a condition. This validator is used by the FlagStateForm to ensure that values entered for conditions are valid for that condition, and to provide feedback if not.

To support that functionality, this change also refactors the conditions code, breaking the registry and the built-in conditions into their own modules, and adding a new module for validators. The API remains the same.

@willbarton willbarton requested a review from a team April 17, 2020 16:00
@chosak
Copy link
Member

chosak commented Apr 17, 2020

Unfortunately, this implementation causes an error any time the Django admin is used to edit any condition with a required parameter like request (for example). Unfortunately you can't just ignore RequiredForCondition errors, too, because then there's no guarantee that you'll actually execute the logic you want to test.

I'm not sure there's a simple way to validate values in a generic way, as opposed to on a condition-by-condition case.

As a side note, I noticed when testing this using Wagtail that wagtail-flags seem to have its own copy of FlagStateForm (only slightly different), which means this PR doesn't change how things work in Wagtail. Couldn't / shouldn't wagtail-flags use the form from django-flags?

@willbarton willbarton marked this pull request as draft April 17, 2020 17:18
@willbarton
Copy link
Member Author

@chosak Yeah, I see that now.

I'll use this opportunity to add general validation for conditions, I think, something we should have anyway.

@willbarton willbarton marked this pull request as ready for review April 20, 2020 12:36
@willbarton willbarton changed the title Ensure valid flag state values in FlagStateForm Add validator support to condition registration and FlagStateForm Apr 20, 2020
docs/debugging.md Outdated Show resolved Hide resolved
@chosak
Copy link
Member

chosak commented May 4, 2020

Just throwing out an alternate design:

class  MyCondition:
    def __call__(value, **kwargs):
        # check condition

    def validate(value):
        pass

Allow conditions to be optionally defined as callable objects with an optional validate method. This way, the conditions encapsulate their own validation, and you don't need to maintain a separate list of validators.

@willbarton
Copy link
Member Author

@chosak I thought about that, but I think I'd prefer to be able to reuse Django's validators as much as possible — and when not the actual validators, the same approach to validators. That, to me, requires them to be decoupled from the conditions.

It's possible we could support both means through introspection when a condition is registered... but that seems to more complexity without a lot of gain.

@willbarton
Copy link
Member Author

willbarton commented May 21, 2020

@chosak It turns out, it's pretty easy to support that behavior, as well as the separate condition/validators by storing separate validators as validate attributes on condition callables. Then we can assume that condition_fn.validate always exists if we get the callable out of the condition registry. I've done that in b706418.

@willbarton willbarton requested a review from higs4281 May 21, 2020 14:08
@cwdavies cwdavies requested a review from chosak May 22, 2020 14:02
@willbarton willbarton added this to 5.0 in Roadmap May 28, 2020
@cwdavies cwdavies requested a review from a team May 28, 2020 14:40
Copy link
Contributor

@cwdavies cwdavies left a comment

Choose a reason for hiding this comment

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

I have a minor suggestion that could be done in this PR, but it is not something that has to be done.
When I ran git log and tox I was pleasantly surprised to see that Django 3.0 is supported.
My change would be to remove support for Django 1.11 as cfgov-refresh is now on Django 2.2.
The file tox.ini could be updated to remove py36-dj111 and py38-dj30.

@willbarton
Copy link
Member Author

@cwdavies yes, that was in #58. I'd like to keep Django 1.11 around a little while longer.

Closes #59. Flag state values were not previously validated in the `FlagStateForm`, resulting in potential exceptions being raised on flag state checks.

Implementation based on suggestion by @Weltraum.
This change supports adding a `validator` callable when registering a condition. This validator is used by the `FlagStateForm` to ensure that values entered for conditions are valid for that condition, and to provide feedback if not.

To support that functionality, this change also refactors the conditions code, breaking the registry and the built-in conditions into their own modules, and adding a new module for validators. The API remains the same.
This change adds support for custom user models to the user condition and its validator.
This change replaces the separate global `_validators` registry by using the condition callable to store the validator as a `validate` attribute on the callable.

This allows for specifying conditions and validators as either separate callables or as a single callable object that encapsulates both.
@willbarton willbarton merged commit 9d7f50d into master Jun 10, 2020
@willbarton willbarton deleted the clean-flag-form-data branch June 10, 2020 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Add clean_value in FlagStateForm
4 participants