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

Email verification support #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

aehlke
Copy link
Contributor

@aehlke aehlke commented Apr 21, 2011

Hey, I'm using Pinax's account app along with the django-emailconfirmation app to require email verification after signing up before being able to login. This doesn't work with the current lazysignup convert view, since it calls login on the new user immediately after creating it. So I added some kwargs to the view to accomodate email verification and to specify a template to render instead of redirecting if email_verification is on.

I don't expect you to pull this as it is -- I haven't written any tests for this yet, since I'm not sure the way I've done this is how you'd like it (if you even think this update is a good idea in the first place). In particular, I'm not sure how the AJAX requests should be handled in this case -- see the TODO in the commit.

Let me know your thoughts on this and I can try to clean it up some. Thanks!

allow redirect URL to be overridden
@danfairs
Copy link
Owner

Mm - that's really interesting, I like the idea. I'll try to take a more detailed look in the next couple of days. The structure looks OK, I can imagine that this would be useful on a site where membership needs to be approved by an administrator. So initial thoughts (and I'll try and review in more detail over the weekend)

  • Rename references to 'email verification' to something less email-specific
  • As far as the AJAX request goes, sending HTTP 202 (Accepted) is probably the best thing we can do here, together with some simple text in the response body so a human can figure out what's going on. At the risk of getting parameter-itis, the template for this should probably be passed in to the view, like the others are.

But in general, looks really good. If you do get time to add docs and tests, I'd be happy to roll that into a release.

(I've still got better Pinax integration in the back of my mind, but I haven't had to do a Pinax project yet, so I don't know where the pain points are!)

Thanks again for your pull request!

@aladagemre
Copy link

Hi, shall you merge this pull request? I think email verification is an essential feature. I'm using userena and it forces used to verify their emails. But they can overcome this restriction by registering lazily. So this is security-problem, leading to more spammers.

@danfairs
Copy link
Owner

I haven't had the time to look at this in more detail as I said I would :(

I still like the idea that this feature introduces, and my original comments stand.

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.

None yet

3 participants