From 190dff88b6b1615cf5ebf58001053618387b278a Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 11 Dec 2023 14:46:30 +0000 Subject: [PATCH] feedback --- codeforlife/user/migrations/0001_initial.py | 14 ++-- codeforlife/user/models/klass.py | 2 + codeforlife/user/models/otp_bypass_token.py | 5 +- codeforlife/user/models/student.py | 3 +- codeforlife/user/models/teacher.py | 2 +- codeforlife/user/models/user.py | 24 +++++-- .../tests/models/test_otp_bypass_token.py | 1 + codeforlife/user/tests/models/test_teacher.py | 2 + codeforlife/user/tests/models/test_user.py | 65 +++++++++++++++++-- .../{test_abstract.py => test_warehouse.py} | 2 +- codeforlife/user/tests/views/test_klass.py | 1 + 11 files changed, 102 insertions(+), 19 deletions(-) rename codeforlife/user/tests/models/{test_abstract.py => test_warehouse.py} (98%) diff --git a/codeforlife/user/migrations/0001_initial.py b/codeforlife/user/migrations/0001_initial.py index 419e3eed..6eda2a0b 100644 --- a/codeforlife/user/migrations/0001_initial.py +++ b/codeforlife/user/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.20 on 2023-12-11 09:53 +# Generated by Django 3.2.20 on 2023-12-11 14:36 from django.conf import settings import django.core.validators @@ -71,8 +71,6 @@ class Migration(migrations.Migration): name='OtpBypassToken', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('last_saved_at', models.DateTimeField(auto_now=True, help_text='Record the last time the model was saved. This is used by our data warehouse to know what data was modified since the last scheduled data transfer from the database to the data warehouse.', verbose_name='last saved at')), - ('delete_after', models.DateTimeField(blank=True, help_text="When this data is scheduled for deletion. Set to null if not scheduled for deletion. This is used by our data warehouse to transfer data that's been scheduled for deletion before it's actually deleted. Data will actually be deleted in a CRON job after this point in time.", null=True, verbose_name='delete after')), ('token', models.CharField(max_length=8, validators=[django.core.validators.MinLengthValidator(8)])), ], options={ @@ -116,7 +114,7 @@ class Migration(migrations.Migration): ('last_saved_at', models.DateTimeField(auto_now=True, help_text='Record the last time the model was saved. This is used by our data warehouse to know what data was modified since the last scheduled data transfer from the database to the data warehouse.', verbose_name='last saved at')), ('delete_after', models.DateTimeField(blank=True, help_text="When this data is scheduled for deletion. Set to null if not scheduled for deletion. This is used by our data warehouse to transfer data that's been scheduled for deletion before it's actually deleted. Data will actually be deleted in a CRON job after this point in time.", null=True, verbose_name='delete after')), ('is_admin', models.BooleanField(default=False, help_text='Designates if the teacher has admin privileges.', verbose_name='is administrator')), - ('school', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='teachers', to='user.school')), + ('school', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='teachers', to='user.school')), ], options={ 'verbose_name': 'teacher', @@ -202,6 +200,10 @@ class Migration(migrations.Migration): name='otpbypasstoken', unique_together={('user', 'token')}, ), + migrations.AlterUniqueTogether( + name='class', + unique_together={('name', 'school')}, + ), migrations.AlterUniqueTogether( name='authfactor', unique_together={('user', 'type')}, @@ -214,4 +216,8 @@ class Migration(migrations.Migration): model_name='user', constraint=models.CheckConstraint(check=models.Q(models.Q(('email__isnull', False), ('teacher__isnull', False)), models.Q(('email__isnull', True), ('student__isnull', False)), models.Q(('email__isnull', False), ('student__isnull', True), ('teacher__isnull', True)), _connector='OR'), name='user__email'), ), + migrations.AddConstraint( + model_name='user', + constraint=models.CheckConstraint(check=models.Q(models.Q(('last_name__isnull', False), ('teacher__isnull', False)), models.Q(('last_name__isnull', True), ('student__isnull', False)), models.Q(('last_name__isnull', False), ('student__isnull', True), ('teacher__isnull', True)), _connector='OR'), name='user__last_name'), + ), ] diff --git a/codeforlife/user/models/klass.py b/codeforlife/user/models/klass.py index 1b8206ac..75ed94d7 100644 --- a/codeforlife/user/models/klass.py +++ b/codeforlife/user/models/klass.py @@ -64,6 +64,7 @@ class Manager(WarehouseModel.Manager["Class"]): max_length=200, ) + # TODO: phase out and use django's permission system. read_classmates_data = models.BooleanField( _("read classmates data"), default=False, @@ -87,3 +88,4 @@ class Manager(WarehouseModel.Manager["Class"]): class Meta(TypedModelMeta): verbose_name = _("class") verbose_name_plural = _("classes") + unique_together = ["name", "school"] diff --git a/codeforlife/user/models/otp_bypass_token.py b/codeforlife/user/models/otp_bypass_token.py index 30a08ca9..164f59b0 100644 --- a/codeforlife/user/models/otp_bypass_token.py +++ b/codeforlife/user/models/otp_bypass_token.py @@ -15,11 +15,10 @@ from django.utils.translation import gettext_lazy as _ from django_stubs_ext.db.models import TypedModelMeta -from ...models import WarehouseModel from . import user as _user -class OtpBypassToken(WarehouseModel): +class OtpBypassToken(models.Model): """ A one-time-use token that a user can use to bypass their OTP auth factor. Each user has a limited number of OTP-bypass tokens. @@ -31,7 +30,7 @@ class OtpBypassToken(WarehouseModel): ) # pylint: disable-next=missing-class-docstring - class Manager(WarehouseModel.Manager["OtpBypassToken"]): + class Manager(models.Manager["OtpBypassToken"]): def create(self, token: str, **kwargs): # type: ignore[override] """Create an OTP-bypass token. diff --git a/codeforlife/user/models/student.py b/codeforlife/user/models/student.py index d791a7f1..2ab241c3 100644 --- a/codeforlife/user/models/student.py +++ b/codeforlife/user/models/student.py @@ -147,5 +147,4 @@ def teacher(self): The student's class-teacher. """ - if self.klass: - return self.klass.teacher + return self.klass.teacher diff --git a/codeforlife/user/models/teacher.py b/codeforlife/user/models/teacher.py index 10b4f587..51b0bc15 100644 --- a/codeforlife/user/models/teacher.py +++ b/codeforlife/user/models/teacher.py @@ -50,10 +50,10 @@ def create_user(self, teacher: t.Dict[str, t.Any], **fields): "user.School", related_name="teachers", null=True, - editable=False, on_delete=models.SET_NULL, ) + # TODO: phase out and use django's permission system. is_admin = models.BooleanField( _("is administrator"), default=False, diff --git a/codeforlife/user/models/user.py b/codeforlife/user/models/user.py index e30b6661..7e12f60d 100644 --- a/codeforlife/user/models/user.py +++ b/codeforlife/user/models/user.py @@ -119,8 +119,6 @@ def create_superuser(self, password: str, first_name: str, **fields): blank=True, ) - # TODO: is last name required for teachers? - # TODO: are students allowed to have a last name? last_name = models.CharField( _("last name"), max_length=150, @@ -198,9 +196,9 @@ class Meta(TypedModelMeta): verbose_name = _("user") verbose_name_plural = _("users") constraints = [ + # pylint: disable=unsupported-binary-operation models.CheckConstraint( check=( - # pylint: disable-next=unsupported-binary-operation Q( teacher__isnull=True, student__isnull=False, @@ -218,7 +216,6 @@ class Meta(TypedModelMeta): ), models.CheckConstraint( check=( - # pylint: disable-next=unsupported-binary-operation Q( teacher__isnull=False, email__isnull=False, @@ -235,6 +232,25 @@ class Meta(TypedModelMeta): ), name="user__email", ), + models.CheckConstraint( + check=( + Q( + teacher__isnull=False, + last_name__isnull=False, + ) + | Q( + student__isnull=False, + last_name__isnull=True, + ) + | Q( + teacher__isnull=True, + student__isnull=True, + last_name__isnull=False, + ) + ), + name="user__last_name", + ), + # pylint: enable=unsupported-binary-operation ] @property diff --git a/codeforlife/user/tests/models/test_otp_bypass_token.py b/codeforlife/user/tests/models/test_otp_bypass_token.py index 6b1edea3..2854b158 100644 --- a/codeforlife/user/tests/models/test_otp_bypass_token.py +++ b/codeforlife/user/tests/models/test_otp_bypass_token.py @@ -11,6 +11,7 @@ def setUp(self): self.user = User.objects.get(id=2) # TODO: test docstrings. + # TODO: fix unit tests. def test_bulk_create(self): token = get_random_string(8) diff --git a/codeforlife/user/tests/models/test_teacher.py b/codeforlife/user/tests/models/test_teacher.py index c77527bd..6695cd99 100644 --- a/codeforlife/user/tests/models/test_teacher.py +++ b/codeforlife/user/tests/models/test_teacher.py @@ -32,6 +32,7 @@ def test_objects__create_user(self): teacher_fields = {"is_admin": True} user_fields = { "first_name": "first_name", + "last_name": "last_name", "email": "example@codeforlife.com", "password": "password", } @@ -42,6 +43,7 @@ def test_objects__create_user(self): ) assert user.first_name == user_fields["first_name"] + assert user.last_name == user_fields["last_name"] assert user.email == user_fields["email"] assert user.password != user_fields["password"] assert user.check_password(user_fields["password"]) diff --git a/codeforlife/user/tests/models/test_user.py b/codeforlife/user/tests/models/test_user.py index 7d00a676..402c25e7 100644 --- a/codeforlife/user/tests/models/test_user.py +++ b/codeforlife/user/tests/models/test_user.py @@ -40,6 +40,7 @@ def test_constraints__profile(self): User.objects.create_user( password="password", first_name="student_and_teacher", + last_name="last_name", student=student, teacher=teacher, ) @@ -55,6 +56,7 @@ def test_constraints__email__teacher(self): User.objects.create_user( password="password", first_name="teacher", + last_name="last_name", teacher=teacher, ) @@ -86,6 +88,53 @@ def test_constraints__email__indy(self): User.objects.create_user( password="password", first_name="first_name", + last_name="last_name", + ) + + def test_constraints__last_name__teacher(self): + """ + Teachers must have a last name. + """ + + teacher = Teacher.objects.create() + + with self.assert_raises_integrity_error(): + User.objects.create_user( + password="password", + first_name="teacher", + email="teacher@codeforlife.com", + teacher=teacher, + ) + + def test_constraints__last_name__students(self): + """ + Students can't have a last name. + """ + + student = Student.objects.create( + auto_gen_password="password", + klass=self.klass__AB123, + school=self.school__1, + ) + + with self.assert_raises_integrity_error(): + User.objects.create_user( + password="password", + first_name="student", + last_name="last_name", + student=student, + ) + + def test_constraints__last_name__indy(self): + """ + Independents must have a last name. + """ + + with self.assert_raises_integrity_error(): + User.objects.create_user( + password="password", + first_name="teacher", + email="independent@codeforlife.com", ) def test_objects__create(self): @@ -103,6 +152,7 @@ def test_objects__create_user__teacher(self): user_fields = { "first_name": "first_name", + "last_name": "last_name", "email": "example@codeforlife.com", "password": "password", "teacher": Teacher.objects.create(), @@ -110,6 +160,7 @@ def test_objects__create_user__teacher(self): user = User.objects.create_user(**user_fields) # type: ignore[arg-type] assert user.first_name == user_fields["first_name"] + assert user.last_name == user_fields["last_name"] assert user.email == user_fields["email"] assert user.password != user_fields["password"] assert user.check_password(user_fields["password"]) @@ -143,12 +194,14 @@ def test_objects__create_user__indy(self): user_fields = { "first_name": "first_name", + "last_name": "last_name", "email": "example@codeforlife.com", "password": "password", } user = User.objects.create_user(**user_fields) assert user.first_name == user_fields["first_name"] + assert user.last_name == user_fields["last_name"] assert user.email == user_fields["email"] assert user.password != user_fields["password"] assert user.check_password(user_fields["password"]) @@ -160,15 +213,17 @@ def test_objects__create_superuser__teacher(self): user_fields = { "first_name": "first_name", + "last_name": "last_name", "email": "example@codeforlife.com", "password": "password", "teacher": Teacher.objects.create(), } user = User.objects.create_superuser( - **user_fields - ) # type: ignore[arg-type] + **user_fields # type: ignore[arg-type] + ) assert user.first_name == user_fields["first_name"] + assert user.last_name == user_fields["last_name"] assert user.email == user_fields["email"] assert user.password != user_fields["password"] assert user.check_password(user_fields["password"]) @@ -192,8 +247,8 @@ def test_objects__create_superuser__student(self): } user = User.objects.create_superuser( - **user_fields - ) # type: ignore[arg-type] + **user_fields # type: ignore[arg-type] + ) assert user.first_name == user_fields["first_name"] assert user.password != user_fields["password"] assert user.check_password(user_fields["password"]) @@ -208,12 +263,14 @@ def test_objects__create_superuser__indy(self): user_fields = { "first_name": "first_name", + "last_name": "last_name", "email": "example@codeforlife.com", "password": "password", } user = User.objects.create_superuser(**user_fields) assert user.first_name == user_fields["first_name"] + assert user.last_name == user_fields["last_name"] assert user.email == user_fields["email"] assert user.password != user_fields["password"] assert user.check_password(user_fields["password"]) diff --git a/codeforlife/user/tests/models/test_abstract.py b/codeforlife/user/tests/models/test_warehouse.py similarity index 98% rename from codeforlife/user/tests/models/test_abstract.py rename to codeforlife/user/tests/models/test_warehouse.py index 59571ace..a0ebea97 100644 --- a/codeforlife/user/tests/models/test_abstract.py +++ b/codeforlife/user/tests/models/test_warehouse.py @@ -12,7 +12,7 @@ from ...models import User -class TestAbstract(ModelTestCase[User]): +class TestWarehouse(ModelTestCase[User]): """ Tests the abstract model inherited by other models. diff --git a/codeforlife/user/tests/views/test_klass.py b/codeforlife/user/tests/views/test_klass.py index 87ba9d6b..5118ca5c 100644 --- a/codeforlife/user/tests/views/test_klass.py +++ b/codeforlife/user/tests/views/test_klass.py @@ -63,3 +63,4 @@ def test_retrieve__student__same_school__in_class(self): self._retrieve_class(user.student.class_field) # TODO: other retrieve and list tests + # TODO: fix unit tests.