-
Notifications
You must be signed in to change notification settings - Fork 0
Set up automatic patching/unpatching based on setting change #9
Set up automatic patching/unpatching based on setting change #9
Conversation
two_factor/apps.py
Outdated
|
||
@receiver(setting_changed, dispatch_uid="two_factor.handle_setting_changed") | ||
def handle_setting_changed(sender, setting: str, value, **kwargs): | ||
from .admin import patch_admin, unpatch_admin, __default_admin_site__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you put the import here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circular dependencies, apps.py
is imported before everything else, and if that pulls in admin.py
before django is ready, then everything collapses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok :)
two_factor/apps.py
Outdated
unpatch_admin() | ||
|
||
elif setting == "TWO_FACTOR_FORCE_OTP_ADMIN": | ||
if is_patched: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can probably put this "if" in the "elif statement" in order to combine them no?
elif setting == "TWO_FACTOR_FORCE_OTP_ADMIN" and is_patched:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to go with readability & symmetry with the above if-statement to make it clear we're only looking at two settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicking: I think this change could be moved into 'two_factor/signals.py` (which already exists in this lib)
This makes it possible to use @override_settings in tests to control 2FA behaviour.
c11a4b7
to
4129802
Compare
This makes it possible to use @override_settings in tests to control 2FA behaviour.