jazzband / django-waffle

A feature flipper for Django
https://waffle.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.12k stars 258 forks source link

AbstractUserFlag.is_active returns False instead of None as fallback, makes it hard to add custom Flag model checks #495

Open tkoft opened 9 months ago

tkoft commented 9 months ago

https://github.com/django-waffle/django-waffle/blob/master/waffle/models.py#L333

I'm working on a custom Flag model so that we can flag on objects not related to our request Users. I'd like to reuse a lot of the logic from AbstractUserFlag.is_active so that we can still use testing mode, WAFFLE_OVERRIDE setting, missing flag defaults, etc., with the custom Flag. Only if those checks fail do we then want to do our custom model checks.

However, AbstractUserFlag.is_active seems to return False as a default when none of those checks apply. When overriding is_active in our custom Flag model, this makes it impossible to determine whether it's False because those checks explicitly determined the flag should be inactive (in which case we return that value too), or because none of those checks were relevant to the request (in which case we proceed to do our custom checks).

I see there are other conditional paths in AbstractUserFlag.is_active that already return None (e.g. when checking percentage in read_only mode). There are also some other inconsistencies with what "None" means, like in AbstractBaseFlag._is_active_for_user where False is returned when the User is None.

It seems like AbstractUserFlag.is_active should return None as the fall through case. AbstractBaseFlag._is_active_for_user should probably do the same too.