python-dirbtuves / website

Python dirbtuvės project web site.
GNU Affero General Public License v3.0
2 stars 0 forks source link

Add custom User model #11

Closed niekas closed 9 years ago

niekas commented 9 years ago

The new custom User model is based on django.contrib.auth.models.User. Custom User model was extended by adding language field.

All migration history was purged as sugested in first warning message here: https://docs.djangoproject.com/en/1.8/topics/auth/customizing/#substituting-a-custom-user-model

settings.base.AVAILABLE_LANGUAGES tuple was defined for usage in form choice fields. The titles in this tuple should be localised.

sirex commented 9 years ago

According to Django documentation:

If you’re entirely happy with Django’s User model and you just want to add some additional profile information, you could simply subclass django.contrib.auth.models.AbstractUser and add your custom profile fields, although we’d recommend a separate model as described in the “Model design considerations” note of Specifying a custom User model.

From the "Model design considerations":

Model design considerations

Think carefully before handling information not directly related to authentication in your custom User Model.

It may be better to store app-specific user information in a model that has a relation with the User model. That allows each app to specify its own user data requirements without risking conflicts with other apps. On the other hand, queries to retrieve this related information will involve a database join, which may have an effect on performance.

In other words, you should use custom User model only if you want to replace username, email or other fields if defaults does not work in your case. But if you just want to add extra fields you should do that in separate profile model.

So I would suggest to follow this recommendation and use separate model to store Profile specific information and leave User model just for authentication.

sirex commented 9 years ago

Also, after changing user model, you have to write data migration to preserve existing user, because pylab.lt is already in user and has users. But as I said previously, you don't need custom user, you need separate profile model. Separate profile model does not require custom data migration.

niekas commented 9 years ago

Well, we made assumption, that we are not satisfied with django.contrib.auth.models.User, nor with django.contrib.auth.models.AbstractUser. Because in near future we would like to change our custom user table. Migrating to custom user model requires to purge migrations, so we wanted to do it as early as possible. Even if we suggested to copy and use all AbstractUser functionality, its only a template for future modifications. I think we have different views of future developments. To start from, we wanted to make email field required. For django.contrib.auth.models.User email can be blank.

sirex commented 9 years ago

Why this pull request was closed and new one started?

niekas commented 9 years ago

I didn't know how branch pull requests work. Thought that separate commits are suggested to be pulled. We decided to reimplement functionality as you suggested, so closed the request. After reimplementation we opened new pull request.

Know I know that full branch is suggested for merging, not separate commits. Next time I won't close pull requests, which need fixing.