Introduce Custom User model #47
Conversation
|
|
||
| class AccountsConfig(AppConfig): | ||
| default_auto_field = "django.db.models.BigAutoField" | ||
| name = "accounts" |
There was a problem hiding this comment.
issue: This should be {{ project_name }}.accounts
Because our Django application is inside a top level package, that top level package name needs to form part of the app config name. Simeon came across the same issue and reported it in #25
And because this is a template we do not know the packahe/project name ahead of time. Which is fine because startproject will do this for us it we use the {{ project_name }} template tag.
I suspect that this will fix your CI issue.
| from django.contrib.auth.models import AbstractUser | ||
|
|
||
| class CustomUser(AbstractUser): | ||
| pass |
There was a problem hiding this comment.
issue: pass is not needed here
| pass | ||
|
|
||
| def __str__(self): | ||
| return self.username |
There was a problem hiding this comment.
issue: Please ensure that your IDE it set to ensure there is a trailing line ending. The little red symbol above represents that there is not line end on the file line.
| @@ -0,0 +1,7 @@ | |||
| from django.contrib.auth.models import AbstractUser | |||
|
|
|||
| class CustomUser(AbstractUser): | |||
There was a problem hiding this comment.
suggestion: We could just call this User.
I'm not sure calling it CustomUser is helpful. It implies there is a non-custom User which is true in the context of Django as a whole, but not really in the context of just the project in which this model exists.
| def test_create_superuser(self): | ||
| User = get_user_model() | ||
| admin_user = User.objects.create_superuser(username="superuser", password="123") | ||
|
|
||
| self.assertEqual(admin_user.username, "superuser") | ||
| self.assertTrue(admin_user.is_active) | ||
| self.assertTrue(admin_user.is_staff) | ||
| self.assertTrue(admin_user.is_superuser) | ||
| self.assertEqual(admin_user.email, '') |
There was a problem hiding this comment.
praise: Thanks for covering the super user as well as the basic user.
| class UsersManagersTests(TestCase): | ||
|
|
||
| def test_create_user(self): | ||
| User = get_user_model() |
There was a problem hiding this comment.
suggestion: Assert that it return the correct class
If the settings are incorrect, then get_user_model will return the standard, default user model. Since our custom model is functionally exactly the same as the default one - this test would still pass.
It would be a good idea to create a separate test to ensure it actually returns the expected class.
| from django.test import TestCase | ||
|
|
||
|
|
||
| class UsersManagersTests(TestCase): |
There was a problem hiding this comment.
issue: You do not need to extend TestCase
We are using Pytest for our test suite - not Django's built in unit test framework. You should not extend the Django TestCase. You don't even need to wrap your tests in a class. As long as the test function begins def test_ it will be picked up.
See: https://github.com/ackama/django-template/blob/main/template/src/tests/system/main/test_views.py for an example.
86cbebb to
0e6ee6b
Compare
| def test_create_user() -> None: | ||
| User = get_user_model() | ||
| user = User.objects.create_user(username="test", password="123") # noqa: S106 | ||
|
|
||
| assert isinstance(User, UserModel) # noqa: S101 | ||
| assert user.username == "test" # noqa: S101 | ||
| assert user.is_active # noqa: S101 | ||
| assert not user.is_staff # noqa: S101 | ||
| assert not user.is_superuser # noqa: S101 | ||
| assert user.email == "" # noqa: S101 | ||
|
|
||
|
|
||
| def test_create_superuser() -> None: | ||
| User = get_user_model() | ||
| admin_user = User.objects.create_superuser( # noqa: S106 | ||
| username="superuser", password="123" | ||
| ) | ||
|
|
||
| assert isinstance(User, UserModel) # noqa: S101 | ||
| assert admin_user.username == "superuser" # noqa: S101 | ||
| assert admin_user.is_active # noqa: S101 | ||
| assert admin_user.is_staff # noqa: S101 | ||
| assert admin_user.is_superuser # noqa: S101 | ||
| assert admin_user.email == "" # noqa: S101 |
There was a problem hiding this comment.
chore: The S101 noqa markers should not be necessary
That error code is about using asserts - and only applies to production code because running code with the python optimise flag on will remove asserts. It doesn't apply to tests where we need to use them all the time. This linting error should be disabled for the whole src/tests directory. See: https://github.com/ackama/django-template/blob/main/template/setup.cfg
Are you seeing this linting errors when you run inv lint in your projects created with this template?
The S106 is fine.
7670fc8 to
e29f8ed
Compare
e29f8ed to
6a79b74
Compare
Created ADR for the custom User model introduced by #47
Created an accounts app with custom User model.
Issue: #24