-
Notifications
You must be signed in to change notification settings - Fork 30
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
Replace KV registration form with custom registration API #352
Conversation
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.
We should document this feature better, but the changes look good.
@@ -22,6 +23,7 @@ | |||
# Specification taken from https://support.twitter.com/articles/101299 | |||
TwitterValidator = RegexValidator('^[A-Za-z0-9_]{1,15}$', | |||
'Incorrectly formatted twitter handle') | |||
is_registered = import_string(settings.WAFER_USER_IS_REGISTERED) |
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.
Is it worth catching ImportError and returning a message that explicitly points to the WAFER_USER_IS_REGISTERED setting, rather than just the raw error?
# you implement your own registration system. | ||
WAFER_REGISTRATION_MODE = 'ticket' | ||
# WAFER_USER_IS_REGISTERED should return a boolean, when passed a Django user. | ||
WAFER_USER_IS_REGISTERED = 'wafer.tickets.models.user_is_registered' |
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.
We should add this to the documentation, rather than just having comments in the settings file
@stefanor Should we try land this? |
While it made implementing a registration form really simple, it made dealing with the registration data rather complex.
6358501
to
228a876
Compare
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.
👍 Happy to see this land now
I wasn't very satisfied with the KV registration approach we used for DebConf16. While this framework made implementing the form really nice and easy, the data was completely unusable in Django admin, which made dealing with the data a lot harder.
I think if you find yourself needing some sort of registration form, you're really better off throwing a django model + form together, yourself. So, here's a simple API to let wafer know if a user is registered, when you have done that.
We could leave the KV registration framework in place, but I don't know if I want to inflict it on anyone else :)