On Sep 25, 2013, at 2:58 AM, Marc Tamlyn <marc.tam...@gmail.com> wrote:
> 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. It does this in 1.5 and 1.6. AuthenticationForm is already used for custom user models; this doesn't represent an alteration on my part. > 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. Rats. I thought about that particular issue, but forgot to address it. > - 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. This is a good point, although it could be avoided using a set instead of a list. > 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. I'm not clear on how this would make that harder. It would either make it easier, or not introduce a change at all. > 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. This is an important point. Backward compatibility changes become more serious when there are more instances of custom user models in the wild that are able to use the basic UserCreationForm (and similar) classes. That actually might be the ultimate reason why this solution won't be tenable (regardless of preferability), because any maintenance on the forms would have back-compat implications for unknown custom User models. L > 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. -- 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.
