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

Added ALLOW_SIGNUP setting in accounts to enable/disable signups in t… #3579

Closed
wants to merge 1 commit into from
Closed

Conversation

musab-tatek
Copy link

@musab-tatek musab-tatek commented Dec 25, 2023

Added ALLOW_SIGNUP setting in accounts to enable/disable signups in the Accounts Adapte

@@ -243,7 +243,7 @@ def is_open_for_signup(self, request):
Next to simply returning True/False you can also intervene the
regular flow by raising an ImmediateHttpResponse
"""
return True
return app_settings.ALLOW_SIGNUP
Copy link
Owner

Choose a reason for hiding this comment

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

Naming wise this is inconsistent with the adapter method, would have accepted IS_OPEN_FOR_SIGNUP?

@property
def ALLOW_SIGNUP(self):
"""
Allow new registrations.
Copy link
Owner

Choose a reason for hiding this comment

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

This is a new setting -- it requires documentation.

@pennersr
Copy link
Owner

Wondering, do we really need a new setting for this -- why not just override the adapter method?

@coveralls
Copy link

Coverage Status

coverage: 95.82% (+0.001%) from 95.819%
when pulling 220e7e7 on musab-tatek:main
into 4982485 on pennersr:main.

@pennersr pennersr closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants