incuna / django-user-management

User management model mixins and api views.
BSD 2-Clause "Simplified" License
57 stars 24 forks source link

Split mixins between fields and methods #86

Closed kevinetienne closed 9 years ago

kevinetienne commented 9 years ago

Customising fields on BasicUserFieldsMixin is not easy. I have a case where I need to override name and email.

My use case:

class User(AvatarMixin, VerifyEmailMixin, PermissionsMixin, AbstractBaseUser):
    pass

With the current implementation the solution is to copy VerifyEmailMixin and its parent class BasicUserFieldsMixin to redefine name and email fields to avoid name clashes.

It looks like splitting the current mixins to more mixins will allow redefining fields more easily. I suggest to make BasicUserFieldsMixin and VerifyEmailMixin children classes of some fields and methods.

Example:

class BasicUserFieldsMixin(IsStaffMixin, NameMixin, NameMethodsMixin):
    pass

class VerifyEmailMixin(BasicUserFieldsMixin, IsActiveMixin, EmailVerificationMixin, VerifyEmailMethodsMixin):
   pass

Which would allow us to create own BasicUserFieldsMixin and VerifyEmailMixin if one or more fields need changing.

kevinetienne commented 9 years ago

Having such a long chain of inheritance is maybe not the best solution, dealing with python mro and Django Meta class has proven not to be easy to use.

One of the downside of what I have created is that one should reimplement the checks when customising VerifyEmailMixin. Thanks @Ian-Foote

kevinetienne commented 9 years ago

@Ian-Foote @meshy could you have a look to this PR please?

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.01%) when pulling 179bf9ea0bd5bde53a5732bdc76b9055439da5b6 on split-mixins-into-mixins into cf607ed2a285c7f4048ee013b43758930f1b9b9b on master.

meshy commented 9 years ago

Just to clarify: it's not possible to just subclass the parent, and re-define the fields?

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.01%) when pulling 6f431b955841d17bbbf155be92450110da6ce137 on split-mixins-into-mixins into cf607ed2a285c7f4048ee013b43758930f1b9b9b on master.

kevinetienne commented 9 years ago

@meshy unfortunately no :( The error message is the following: django.core.exceptions.FieldError: Local field 'name' in class 'User' clashes with field of similar name from base class 'VerifyEmailMixin' and then you try to subclass VerifyEmailMixin but same error :)

kevinetienne commented 9 years ago

it might be related to the Meta class generating something from the attributes on the class.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.01%) when pulling 6f431b955841d17bbbf155be92450110da6ce137 on split-mixins-into-mixins into cf607ed2a285c7f4048ee013b43758930f1b9b9b on master.

meshy commented 9 years ago

Aaah, I guess it is. Shame. Ok then :)

kevinetienne commented 9 years ago

Relevant piece of code: https://github.com/django/django/blob/d66bda60590daabe21f60a532a613a31a10fedbd/django/db/models/base.py#L229-L238 (might be fixed in the future)

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.01%) when pulling d1e4b3028f5c20563640e240ecc9e835c9744b52 on split-mixins-into-mixins into 8c4649a7ed60ab036a4e3ebb92b0157870557e88 on master.

kevinetienne commented 9 years ago

@Ian-Foote @mattack108 can you merge please?