Re: #3011 - Custom User Models -- Call for final review
Hi all,
I've just pushed fixes that:
* Correct the patch for Python 3
* Merges Anssi's AbstractUser base class changes
* Moves skipIfCustomUser to django.contrib.auth.tests.utils
* Improves the error message when you have a Proxy to a swapped class
* Updates the documentation to address the feedback received to date
Based on the feedback on the patch so far, it looks like we're on
track to merge this later this week.
Yours,
Russ Magee %-)
On Sun, Sep 16, 2012 at 7:15 AM, Russell Keith-Magee
wrote:
> On Sat, Sep 15, 2012 at 11:59 PM, Anssi Kääriäinen
> wrote:
>> On 15 syys, 15:34, Russell Keith-Magee
>> wrote:
>>> Hi all,
>>>
>>> The implementation of custom User models is now ready for final
>>> review, prior to commit to trunk.
>>>
>>> The code is available in my Github repo, in the t3011 branch.
>>>
>>> https://github.com/freakboy3742/django/tree/t3011
>>>
>>> The diff is available here:
>>>
>>> https://github.com/freakboy3742/django/compare/master...t3011
>>>
>>> Documentation is also available in the auth topic guide:
>>>
>>> https://github.com/freakboy3742/django/blob/t3011/docs/topics/auth.tx...
>>>
>>> Barring the discovery of major problems, I'm aiming to commit this
>>> towards the end of next week. Any and all feedback is welcome.
>>
>> The above branch does not work on Python3, which seems like a blocker
>> issue.
>
> Dang - I knew I forgot something. I'll put this on my list to look at.
>
>> Some questions:
>> - If I proxy the auth.User model when it is swapped out I get an
>> error from model validation: "AttributeError: type object 'UserProxy'
>> has no attribute '_base_manager'". The error could be cleaner.
>
>
>> - The last_login field is in the AbstractBaseUser, but it isn't
>> documented as a required field. Is this field required for something?
>> Is it needed as part of AbstractBaseUser?
>
> Yes, last_login is required - it's needed in order to generate
> password reset tokens etc.
>
> It isn't documented because we *really* want people to be subclassing
> AbstractBaseUser, not building their own User from scratch.
>
>> - There is no way to do single-table extension of the default User
>> model. Should there be a way? To me it seems this would be beneficial
>> when dealing with "project profiles". There is little reason I would
>> like to store a required employee number in another table. Maybe it
>> would work if current User was moved to AbstractUser, and User would
>> be class User(AbstractUser): class Meta: swappable =
>> 'AUTH_USER_MODEL'? Quick testing indicates this could work... See
>> commit:
>> https://github.com/akaariai/django/commit/08bcb4aec1ed154cefc631b8510ee13e9af0c19d
>> - I did a little change to REQUIRED_FIELDS handling in conjunction
>> with create_(super)user(). See commit:
>> https://github.com/akaariai/django/commit/d9f5e5addbad5e1a01f67e7358e4f5091c3cad81
>>
>> With the above commits I can do this:
>>
>> class MyUser(AbstractUser):
>> employee_no = models.CharField(max_length=5)
>> some_other_field = models.TextField(null=True, blank=True)
>> REQUIRED_FIELDS = AbstractUser.REQUIRED_FIELDS + ['employee_no']
>>
>> syncb & create_superuser commands work. Admin doesn't work directly,
>> but it seems you can easily subclass the default user admin & change
>> form to make it work.
>>
>> Single-table inheritance seems useful to me, and the added complexity
>> to code isn't much.
>
> To be clear -- is this just targeting the "I'm completely happy with
> auth.User; I just want to put XXX on the model" use case? If so, I can
> certainly agree with this; I'll merge these changes in.
>
>> The above questions aren't meant to be commit blockers. In my
>> (somewhat limited) testing the only real issue was the py3
>> incompatibility, and that seems to be an easy fix. Overall, this looks
>> good to me.
>>
>> - Anssi
>>
>> BTW: I did also a little test to allow select_related() for user
>> profiles:
>> https://github.com/akaariai/django/commit/81f91db3234110b90572b25ca69b9012a475c460
>> - the idea is that the profile models could just to
>> get_user_model().SELECT_RELATED_MODELS.add('user_reverse_name'). I
>> wonder if we want something like this now/later...
>
> Possibly; but if we do, I'd rather attack it at the model Meta level
> -- i.e., an analog to Meta: ordering that applies a select_related
> automatically to every queryset.
>
> Yours,
> Russ Magee %-)
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
Re: #3011 - Custom User Models -- Call for final review
On 16 syys, 02:15, Russell Keith-Magee
wrote:
> > - The last_login field is in the AbstractBaseUser, but it isn't
> > documented as a required field. Is this field required for something?
> > Is it needed as part of AbstractBaseUser?
>
> Yes, last_login is required - it's needed in order to generate
> password reset tokens etc.
>
> It isn't documented because we *really* want people to be subclassing
> AbstractBaseUser, not building their own User from scratch.
+1 to this.
> > - There is no way to do single-table extension of the default User
> > model. Should there be a way? To me it seems this would be beneficial
> > when dealing with "project profiles". There is little reason I would
> > like to store a required employee number in another table. Maybe it
> > would work if current User was moved to AbstractUser, and User would
> > be class User(AbstractUser): class Meta: swappable =
> > 'AUTH_USER_MODEL'? Quick testing indicates this could work... See
> > commit:https://github.com/akaariai/django/commit/08bcb4aec1ed154cefc631b8510...
> > - I did a little change to REQUIRED_FIELDS handling in conjunction
> > with create_(super)user(). See commit:
> >https://github.com/akaariai/django/commit/d9f5e5addbad5e1a01f67e7358e...
>
> > With the above commits I can do this:
>
> > class MyUser(AbstractUser):
> > employee_no = models.CharField(max_length=5)
> > some_other_field = models.TextField(null=True, blank=True)
> > REQUIRED_FIELDS = AbstractUser.REQUIRED_FIELDS + ['employee_no']
>
> > syncb & create_superuser commands work. Admin doesn't work directly,
> > but it seems you can easily subclass the default user admin & change
> > form to make it work.
>
> > Single-table inheritance seems useful to me, and the added complexity
> > to code isn't much.
>
> To be clear -- is this just targeting the "I'm completely happy with
> auth.User; I just want to put XXX on the model" use case? If so, I can
> certainly agree with this; I'll merge these changes in.
Yes, that is the use case. The proxy use case I was worried about in
the other post is about just overriding methods or the manager
without adding any fields. The ability to actually have a _proxy_
isn't important, it is enough you can extend the default user model
easily.
Creating an admin class for no-added-fields extended user model is
somewhat straightforward, although the admin forms currently assume
the auth.User as the base user. I did a little hack to make it easier
to use these forms, see commit:
https://github.com/akaariai/django/commit/04ae5183df8613fd0d91b43834a4582fdf6b0d04
- but I am not totally happy about this approach. If you use an
AbstractBaseUser derived user model, these forms aren't actually
usable.
Creating an admin class for added fields model is also somewhat easy,
you just need to play with UserAdmin.fieldsets to have the additional
fields actually present in the edit form, and you need to make your
required fields "blank=False, null=True", so that the two-stage user
creation works correctly.
> > BTW: I did also a little test to allow select_related() for user
> > profiles:https://github.com/akaariai/django/commit/81f91db3234110b90572b25ca69...
> > - the idea is that the profile models could just to
> > get_user_model().SELECT_RELATED_MODELS.add('user_reverse_name'). I
> > wonder if we want something like this now/later...
>
> Possibly; but if we do, I'd rather attack it at the model Meta level
> -- i.e., an analog to Meta: ordering that applies a select_related
> automatically to every queryset.
Rethinking this, and I don't see the need anymore. It is now possible
to control the user model's manager, so you can just override
get_query_set() and use select_related() manually. Automatic
select_related/prefetch_related could be good for other use cases,
though.
Some additional findings:
- The get_by_natural_field() manager method has a bug: getattr(self,
'USERNAME_FIELD', ...) should be getattr(self.model,
'USERNAME_FIELD', ...)
- Do we want to future-proof some of the interface methods by adding
**kwargs to the method signatures? At least has_perm /
has_module_perms seem like possible candidates.
I wonder if we want to do something about allowing easier password
handling in (admin)forms for custom user classes. Currently making an
admin class for your user class is somewhat hard - you need to either
dig out what auth.admin does for the password field or reinvent it
yourself.
In total this is looking really good, but I can see the need to do
additional polish for some time. Maybe we could take a somewhat
relaxed approach to what constitutes a feature addition when it comes
to this feature? To me it seems there might be something we want to do
about the password forms for example. But, I am not sure we can get
that something done about this by the end of month...
- Anssi
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email
Re: #3011 - Custom User Models -- Call for final review
On Sun, Sep 16, 2012 at 8:15 PM, Anssi Kääriäinen wrote: > On 16 syys, 02:15, Russell Keith-Magee > wrote: > Creating an admin class for no-added-fields extended user model is > somewhat straightforward, although the admin forms currently assume > the auth.User as the base user. I did a little hack to make it easier > to use these forms, see commit: > https://github.com/akaariai/django/commit/04ae5183df8613fd0d91b43834a4582fdf6b0d04 > - but I am not totally happy about this approach. If you use an > AbstractBaseUser derived user model, these forms aren't actually > usable. > > Creating an admin class for added fields model is also somewhat easy, > you just need to play with UserAdmin.fieldsets to have the additional > fields actually present in the edit form, and you need to make your > required fields "blank=False, null=True", so that the two-stage user > creation works correctly. I'll take a closer look at this patch; however, see my comments below about admin integration. > Some additional findings: > - The get_by_natural_field() manager method has a bug: getattr(self, > 'USERNAME_FIELD', ...) should be getattr(self.model, > 'USERNAME_FIELD', …) Good catch - I've just pushed a fix. > - Do we want to future-proof some of the interface methods by adding > **kwargs to the method signatures? At least has_perm / > has_module_perms seem like possible candidates. I'm not convinced by this -- at least, not for the permissions methods. The API for module permissions isn't a new invention -- it's something that's been part of the auth backend API since pretty much day 1, and the only modification that has been required is to support object permissions. If there's any other API where you think there's room for expansion, I'm open to discussion. > I wonder if we want to do something about allowing easier password > handling in (admin)forms for custom user classes. Currently making an > admin class for your user class is somewhat hard - you need to either > dig out what auth.admin does for the password field or reinvent it > yourself. > > In total this is looking really good, but I can see the need to do > additional polish for some time. Maybe we could take a somewhat > relaxed approach to what constitutes a feature addition when it comes > to this feature? To me it seems there might be something we want to do > about the password forms for example. But, I am not sure we can get > that something done about this by the end of month... We could easily hold up this patch on making the admin forms completely flexible for all possible user models. However, for my money, we still have a couple of months to nail down any problem areas in the API, and we'll get more benefit from having this in the trunk repo and getting more eyeballs looking at it than we will from endlessly polishing behind the scenes aiming at some Platonic ideal. However, that said -- even if we don't make any significant changes between now and 1.5 final, it's now *possible* to write and use a custom User model that is compatible with admin. That's the big step IMHO, and the piece that needs to land for the alpha. We can continue to improve forms and documentation until the 1.5 release, and we can continue to improve post-release. I'm not denying that there is room for improvement, but the problems you're talking about are on the scale of "making difficult things easier", not "making impossible things possible". Russ %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #3011 - Custom User Models -- Call for final review
On 16 syys, 15:50, Russell Keith-Magee wrote: > We could easily hold up this patch on making the admin forms > completely flexible for all possible user models. However, for my > money, we still have a couple of months to nail down any problem areas > in the API, and we'll get more benefit from having this in the trunk > repo and getting more eyeballs looking at it than we will from > endlessly polishing behind the scenes aiming at some Platonic ideal. My intention wasn't to hold this patch out until it is perfect - I meant to say that after this is committed to 1.5, should we allow for some polishing changes to get in after feature freeze even if they are technically feature additions. Though, I can see a point in shipping with what we get into 1.5 by the end of month, collect feedback and then improve things for 1.6... I am definitely +1 to committing this before feature freeze. - Anssi -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: #3011 - Custom User Models -- Call for final review
On Sun, Sep 16, 2012 at 9:13 AM, Anssi Kääriäinen wrote: > I meant to say that after this is committed to 1.5, should we allow for > some polishing changes to get in after feature freeze even if they are > technically feature additions. > The kinds of changes you are describing can, I believe, be made after alpha (i.e. feature freeze). Major features like this one may very well need some additions/polishing after landing on master, once many more people start to experiment with them and find out what is needed/useful/etc. The intent of "feature freeze" is to stabilize the code base in general and not introduce side-effect bugs that may go unnoticed until after release if made late in a release cycle. For big brand-new features, sticking to a hard "feature freeze" line after alpha is more likely to be harmful than helpful, and I don't believe we've ever taken that sort of hard line for major features. It's even possible (though clearly not desirable) to introduce backwards-incompatible changes in the public interfaces for a new feature between alpha/beta/final. In this phase we're seeking to gather feedback from early adopters and experimenters, and not being able to incorporate that feedback into the in-progress release would be silly. Karen -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: end of "coming soon" in the tutorial? - tutorial 5 feedback needed!
https://code.djangoproject.com/ticket/16671 On Saturday, September 15, 2012 7:24:23 PM UTC-4, Tim Graham wrote: > > Katie Miller and Ben Sturmfels have written a fifth tutorial covering > making "polls" a reusable app. I've given it an initial review, but I'm > hoping we can get a few more people to give it a look and/or try it out, > particularly if you are an expert in packaging (which I'm not). Thanks > guys! > > p.s. I've built the docs with the latest patch here: > http://techytim.com/django/tutorial05/intro/tutorial05.html > -- You received this message because you are subscribed to the Google Groups "Django developers" group. To view this discussion on the web visit https://groups.google.com/d/msg/django-developers/-/eCoT-L40OXEJ. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Testing django 1.5 with firebirdsql
Here's the topic you were referring to Anssi. https://groups.google.com/forum/#!topic/django-developers/AZrSn6LCLVU/discussion Le samedi 15 septembre 2012 13:18:38 UTC-4, Anssi Kääriäinen a écrit : > > On 15 syys, 01:05, maxi wrote: > > BTW, I've another issue related to one specific test. > > In test/regressiontests/model_fields/models.py there is a model > > defined as: > > > > class BigD(models.Model): > > d = models.DecimalField(max_digits=38, decimal_places=30) > > > > Firebird doesn't accept that definitions for max_digits and > > decimal_places (the max is 18) > > It will be possible to add a DatabaseFeature attribute for avoid or > > skip related tests? > > I have a feeling I have seen the max_digits issue before. Quick trac- > search didn't reveal anything, though. But yes, a database feature is > the way to go. > > - Anssi > -- You received this message because you are subscribed to the Google Groups "Django developers" group. To view this discussion on the web visit https://groups.google.com/d/msg/django-developers/-/QWmriQbicS4J. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
