Thanks for compiling a more complete solution. The biggest disadvantage as I see it is that is adds even more complexity to the implementation of those forms. In particular the `setattr` on the form class of a `clean_` method is a really nasty code smell. The code in the `create_user`/`create_superuser` commands is also a prime example of additional unnecessary complexity in a function which would be trivially avoided by separation.
The authentication form renders a field called "username" which is a CharField, not an EmailField. This seems pretty bad to me. Other things which appear to be bugs to me: - You automatically register an admin class for any custom user model, which is likely to break validation. - If I set first_name and last_name as REQUIRED_FIELDS then your admin class will break validation or render a weird form as the field names appear twice. - Docstring for AbstractCommonUser is wrong. - There are a number of style errors in the code - backslashes for line continuation, unnecessarily wrapped lines, spaces in the wrong places etc. The Admin related issues here are the biggest problem. If admin validation fails for *any custom user model*, then the server will not start if contrib.auth is installed. Your main aim here seems to be to reduce code duplication by writing clever, quasi-magical code instead. We're not just trying to make this work for a particular new User model - we need to make it not break for *every* user model. Writing custom user models is an inherently duplicatory process in some places - I've done it in several projects and it always involves lifting some components from Django and tweaking them appropriately. The more we complexify the forms, the harder this is to do. Russell's original point about providing a complete implementation of "a custom user model" as an example is still very valid. Introducing an easier way to do say, email based auth, should not make writing a custom social based auth solution harder. Now I admit there's not as much magic so far as I expected (though I expect it may get worse in the test suite). However I think the points regarding complexity still stand. From a maintenance perspective, changing this code in two places is bad, but always having to consider at least two (in your case actually many) distinct cases whenever changing the code is worse. On 25 September 2013 01:04, 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 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. > > > 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 > > -- > 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. > -- 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.
