cfpb / django-flags

Feature flags for Django projects
https://cfpb.github.io/django-flags/
Creative Commons Zero v1.0 Universal
260 stars 32 forks source link

Fix boolean and parameter conditions #31

Closed willbarton closed 5 years ago

willbarton commented 5 years ago

This PR makes small changes to the boolean and parameter conditions:

Boolean conditions can now match strings with superfluous whitespace around their values, so if, for example, ' true ' is entered as the value of the boolean condition, it will evaluate to True. Previously it did not.

Parameter conditions previously checked for True explicitly as the value of the parameter, i.e. ?my_flag=True. This means other possible variations, ?my_flag=true, ?my_flag=yesplease, and just ?my_flag would not work. The documentation is ambiguous on the expected behavior of parameter conditions, but the original intent was probably that any variation above would work. This PR clarifies the documentation and changes the implementation of the condition such that as long as the parameter exists in request.GET it will evaluate to True. This means `?my_flag=True, ?my_flag=true,?my_flag=yesplease, and just?my_flagwill all evaluate toTrue`.

This PR closes #27 and closes #30.

willbarton commented 5 years ago

Should this be treated as a breaking change? If someone already has behavior set up using parameter with ?something=False as an explicit way to disable a flag, this will break that.

This is what I struggle with — I almost wonder if this should be added as an alternative parameter condition (and maybe a deprecation warning added for the existing condition).

contolini commented 5 years ago

This is what I struggle with — I almost wonder if this should be added as an alternative parameter condition (and maybe a deprecation warning added for the existing condition).

Another option to avoid a breaking change might be to allow users to provide their own param value. Something like:

FLAGS = {'MY_FLAG': [{'condition': 'parameter', 'value': 'activate=yes'}]}

And if =XXXXX isn't found in the value config option, default to True.

willbarton commented 5 years ago

@contolini I actually like that a lot — explicit behavior is always better. I think that's the answer I'm looking for.

contolini commented 5 years ago

@contolini I actually like that a lot — explicit behavior is always better. I think that's the answer I'm looking for.

The only downside with that approach is that you couldn't have a valueless param enable a flag, e.g. /foo?activate. But I guess that's fine, just update the docs to make it clear.

willbarton commented 5 years ago

@contolini alrighty, that's implemented! And you can use ?foo by specifying the value as foo=!

@chosak I also updated the description to be a little clearer (by copying/pasting what you wrote that's still relevant).