Hi, I'm another one of the authtools devs.
On Tuesday, September 24, 2013 6:04:17 PM UTC-6, Luke Sneeringer wrote: > > Good evening, Russell, et. al., > I had some time this afternoon. :-) Since there are already a couple of > reference implementations for how to do this with an e-mail app, I decided > to take a crack at an implementation that would include an EmailUser within > the base auth app. I understand we've far from decided on a method, and I > still stand by my promise to do whatever Russell decides, but I figured > code to look at would be a good thing, and that it would give me a better > idea of the task. > > Here's the branch: > https://github.com/lukesneeringer/django/tree/ticket-20824-noemailapp > > I have a few observations and self-criticisms: > > > - The big advantage, as I see it, to this approach over a separate-app > approach is that it *drastically* reduces code duplication. Forms, the > admin, etc., all use the same classes they did before, just those classes > are now a bit more aware of what the USERNAME_FIELD and REQUIRED_FIELDS > are. This is really the reason I believe this to be the correct approach. > A > separate app would have to be maintained, well, separately. :-) > - I have a few criticisms of my approach, now that I've had a chance > to see it in action: > - The create_user and create_superuser methods on UserManager were > a bit of a land mine. The current (1.6) implementation takes username, > email, and password as positional arguments, and making this into an > all-kwarg approach would break back-compat. I landed on a solution, but > I'm > not sure it's the right one. > - In order to maintain the form error message but not have the form > itself be opinionated about the presence of "username", I removed the > explicit validator on the form, simply letting ModelForm outsource it > to > the Model. This provides the same error message for both invalid and > duplicate usernames / e-mail addresses (as appropriate). I think this > actually might be a *good* thing (again, code duplication is reduced), > but > I don't really know if there's some reason why it was constructed the > way > it was before. > - There's a special message for duplicate usernames ("A user with > that username already exists."), and it's not a trivial thing to just > make > an e-mail address correlate because of internationalization. I reverted > to > the previous message, which was translated ("Please correct the > duplicate > data for email, which must be unique."), because I really don't know > what > the appropriate procedure is here. (This would be an issue with a > separate-app approach too, but it's on my list of "things I didn't > know".) > - In starting to write tests (not committed), I hit an interesting > situation where overwriting AUTH_USER_MODEL using override_settings may > not > be sufficient, which is an issue I would need to figure out. I expect > this > would not be a problem in an email_auth app, since the model wouldn't > be > loaded until after the override. > > This is a problem that we ran into with authtools, what we ended up doing was just running the tests 3 times[1]. I don't think this is a realistic approach within Django core though. One of the main problems with this is that you use get_user_model at the root level of the forms module, so the Form will be declared against only one model--I don't know what you can do though. Having email_auth as a separate app would probably clear up this problem for the tests, but if that happens then we would just have to concrete specific sets of forms, instead of more generic Forms, which was the whole idea. The only situation where you should be switching settings around is in tests, so I do think it is a little unfair to compromise the generic nature of the forms just to appease the tests. > > Russell, I definitely understand that you're still not sold on this > method. I think that at this point, I've made my case, and if you're still > not sold, I'm ready to coalesce the email_auth app implementations (two > have been offered) and get that tip-top, tested, and documented. > > Thoughts? > > L > > I left some more code-specific comments on GitHub. [1]: https://github.com/fusionbox/django-authtools/blob/master/Makefile#L6-L15 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers. For more options, visit https://groups.google.com/groups/opt_out.
