On Wed, Sep 25, 2013 at 8:04 AM, Luke Sneeringer <l...@sneeringer.com>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 completely fail to see why this is the case. You seem to be implying that by putting the EmailUser model in a separate app, this means we need to duplicate code. Why can't you: * Put a base class in contrib.auth and put a subclass in contrib.email_auth * Put the model and admin registration in contrib.email_auth and point at contrib.auth.forms. If there truly is common base functionality, then *put it in a base class*, and use that base class. DRY doesn't mean you don't *EVER* duplicate code. It means that you only ever express an *idea* once. If you have two ideas, and they're actually different, the fact that they've got largely similar implementations doesn't mean you tie yourself in knots trying to avoid typing the same characters twice. At the end of the day, practicality beats purity. "Duplicating" a handful of lines of code at the cost of a simple, predictable implementation is a hands-down win in my book. This is especially true when you consider that we're dealing with user authentication here, which has huge implications for the security of Django as a platform. Authentication is literally the front line of framework security, because it's how you get authenticated to do stuff in the first place. Complex implementations mean edge cases accidentally slip through; simple implementations don't allow those problems. > > - 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. > > … and all of these are exactly the problems that I predicted -- that you'll end up with complex (and potentially backwards incompatible) internals, or weird logic switching between models, or problems with tests binding the User model to a global context. > 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. > I think it's safe to say I'm not sold at this point. Yours, Russ Magee %-) -- 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. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq8482T6NSf_KMgDCSKpU-%3D1-1RpryxU5N5ACPyLPqcycu0A%40mail.gmail.com. For more options, visit https://groups.google.com/groups/opt_out.
