-
Notifications
You must be signed in to change notification settings - Fork 346
Fix admin client with custom user models #124
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
Fix admin client with custom user models #124
Conversation
@pelme Hi, what is with my PR? :-) |
Hi @bh, Thanks a lot for this patch and sorry for the delay in the response. It looks good to merge to me, but before merging I would like to have a test that uses a custom user model with a custom username to make sure this works properly. There are already tests for the admin client without custom user models, but none for the custom user model. I guess it is not possible to just change the custom user model during run time to add a test, so this likely needs to be tested by invoking another pytest run similar to the db_reuse tests. It makes writing the test a bit more complicated, but it makes sure that this works all the way. |
Ok, I'll add a test for this :-) |
Since Django >= 1.5 the username field is variable, so get 'username field' by using UserModel.USERNAME_FIELD
This also avoids hacks to mute pyflakes.
Did it! ;) |
Thank you, Benjamin. I think this test would be better tested by adding a test which uses the testdir fixture and invokes pytest from within a test itself. The number of tox testenv's is already a bit problematic, and I'd like to avoid adding more of those unless new databases are introduced. An example of such a test (using testdir) can be found in https://github.com/pelme/pytest_django/blob/master/tests/test_django_settings_module.py#L18. It adds some extra indirection since py.test is actually started in a subprocess ( That test could then be skipped for certain Django versions (using |
@bh Thanks for your work on this, @Stranger6667 just fixed the test and this is now merged into master. Please open a new issue if there are still problems with it. |
Cool! 👍 |
No description provided.