Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for custom user models #2622

Merged
merged 71 commits into from Feb 28, 2014
Merged

Conversation

tanderegg
Copy link
Contributor

This pull request is related to #1798 and #1916.

I've updated the documentation, tests, and relevant parts of the code to support custom user models as long as the name of the model is User. In addition to a simple customuserapp.User model which adds a single field, I also included a emailuserapp.User which uses the email field as USERNAME_FIELD.

Lastly, I updated the test runner in develop.py to support a new command line arguement, --user, which allows for specifying the user model. By default, django.contrib.auth.User is used.

Tested with Django 1.4, 1.5, and 1.6.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 467f970 on tanderegg:develop into 5fa6804 on divio:develop.

if get_cms_setting('PERMISSION'):
admin.site.register(PageUser, PageUserAdmin)
admin.site.register(PageUserGroup, PageUserGroupAdmin)
#if get_cms_setting('PERMISSION'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to never register

@digi604
Copy link
Contributor

digi604 commented Feb 12, 2014

@tanderegg wow... thank you for this GREAT pull request.

i have one missing thing though... the new --user option for running the tests. Could you add them to the travis.yml? So that at least one test run with django 1.5 and 1.6 and mysql or postgres is run with this option? And maybe document somewhere (maybe help text of develop.py) what the new built in custom user is.

Also docs are a little light... Are there any limitations when using a custom user model? (class name, primary key etc)? Please update the upgrade/3.0.txt as well and the CHANGELOG.txt...oh and please add yourself to AUTHORS.txt

@digi604 digi604 added this to the 3.0 milestone Feb 12, 2014
@stefanfoulis
Copy link
Contributor

Thanks a lot for pushing this issue forwards @tanderegg :-)

Some things we should consider:

  • docs: add warning that in most cases a custom user is not what you want to do (a custom profile model with a OneToOne relation is usually enough).
  • docs: add warning that swapping out a user model on an existing db is a lot of hassle (the model should be swapped out from the very beginning of the project).
  • docs: only works if the custom user model is also called User (because of reverse relation naming with other models, e.g Group)
  • docs: list what fields django-cms expects to be on the User model (and how to define a custom username field)
  • testsuite: we could run the testsuite multiple times on travis: http://stackoverflow.com/questions/15369987/testing-with-a-custom-user-model-as-a-foreignkey-in-django-1-5 (not for all combinations though)

@tanderegg
Copy link
Contributor Author

Oops, sorry about the comments! Also, I intended to update the docs more, but I wasn't sure where the best place was to put info about custom user models. Should that go in customization.rst?

@stefanfoulis for the testsuite, I added functionality related to that suggestion, I'll update travis.yml to use it.

@tanderegg
Copy link
Contributor Author

I've addressed all issues except the travis.yml configuration, not as familiar with Travis so I need to look into how to run the tests with multiple different command line options. I won't be able to work on this til next Tuesday.

@digi604
Copy link
Contributor

digi604 commented Feb 18, 2014

awesome

@digi604
Copy link
Contributor

digi604 commented Feb 18, 2014

please remerge with current develop

@koirikivi
Copy link

I integrated this to a real-life project that needed custom user support, and from what I can tell, it works well. I haven't extensively tested all of CMS's functionality though.

One issue I found was that certain imports in cms.compat caused circular import errors in non-runserver contexts. I created a fix and submitted it as a PR to @tanderegg (tanderegg#1) since he's managing the issue.

@chrisvans
Copy link

Thanks for taking this on, I'll integrate this into a real-life project as well and see how it goes.

@koirikivi
Copy link

@digi604 that, too is fixed in the PR tanderegg#2, specifically in 2df0bea (on my fork). I enabled Travis CI for my local fork and all tests pass (see https://travis-ci.org/koirikivi/django-cms/builds/19692711)

@digi604
Copy link
Contributor

digi604 commented Feb 27, 2014

@koirikivi could you open a PR with your own changes so we can this baby merged?

fixes django-cms#2685 (tree view has links to current page language)
@koirikivi
Copy link

@digi604 done, see #2753.

@nickdjones
Copy link
Contributor

Amazing work tanderegg! Thanks

@tanderegg
Copy link
Contributor Author

OK, I've merged @koirikivi's PR and removed all explicit dependencies on the name "User" when querying on the Group model. All tests pass with a user model named "EmailUser", and I updated travis accordingly. Assuming Travis tests pass, this should be good to merge.

@koirikivi
Copy link

Thanks @tanderegg, you da man 8-)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) when pulling e66bd00 on tanderegg:develop into 3a09d5c on divio:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) when pulling e66bd00 on tanderegg:develop into 3a09d5c on divio:develop.

@koirikivi
Copy link

Tests failed for 885ca0c, because apparently Groups don't require the user model to be named User after all. I'm in the process of fixing this, will incorporate changes to #2753.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.19%) when pulling e66bd00 on tanderegg:develop into 3a09d5c on divio:develop.

@koirikivi
Copy link

Oh the agony - in the case the custom user model is named EmailUser, Django 1.5 creates group.emailuser_set, but for Django 1.6 that is group.user_set. See https://code.djangoproject.com/ticket/20244 and contrast https://github.com/django/django/blob/1.5/django/contrib/auth/models.py#L293 and https://github.com/django/django/blob/1.6/django/contrib/auth/models.py#L286 .

I'll take a look at this, incorporate it in my PR and likely make a separate PR that just expects the user model's name to be "User" so we can get this merged in case tests break again.

@koirikivi
Copy link

Added fixes in #2753

@digi604 digi604 merged commit e66bd00 into django-cms:develop Feb 28, 2014
@digi604
Copy link
Contributor

digi604 commented Feb 28, 2014

thanx guys for the hard work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet