feedback

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'),
+        ),
     ]

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"]

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.
 

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

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,

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

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)

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": "[email protected]",
             "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"])

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="[email protected]",
+                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="[email protected]",
             )
 
     def test_objects__create(self):
@@ -103,13 +152,15 @@ def test_objects__create_user__teacher(self):
 
         user_fields = {
             "first_name": "first_name",
+            "last_name": "last_name",
             "email": "[email protected]",
             "password": "password",
             "teacher": Teacher.objects.create(),
         }
 
         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": "[email protected]",
             "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": "[email protected]",
             "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": "[email protected]",
             "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"])

codeforlife/user/tests/models/test_abstract.py → 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.
 

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.