From b562b8dbd07094def7d36e3afc4ab5b6111a5fa5 Mon Sep 17 00:00:00 2001
From: SKairinos <skairinos@gmail.com>
Date: Fri, 15 Dec 2023 11:16:44 +0000
Subject: [PATCH 1/3] feedback

---
 codeforlife/models/__init__.py              |  2 +-
 codeforlife/user/migrations/0001_initial.py |  6 +++---
 codeforlife/user/models/otp_bypass_token.py | 14 ++++----------
 codeforlife/user/models/student.py          |  8 ++++----
 codeforlife/user/models/teacher.py          |  5 ++---
 codeforlife/user/models/user.py             |  3 +--
 6 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/codeforlife/models/__init__.py b/codeforlife/models/__init__.py
index 6cdb83e5..4618e0cb 100644
--- a/codeforlife/models/__init__.py
+++ b/codeforlife/models/__init__.py
@@ -48,7 +48,7 @@ def update(self, **kwargs):
                 last_saved_at: When these models were last modified.
 
             Returns:
-                The number of models updated.
+                The number of models matched.
             """
 
             kwargs["last_saved_at"] = timezone.now()
diff --git a/codeforlife/user/migrations/0001_initial.py b/codeforlife/user/migrations/0001_initial.py
index bce97322..33e9816b 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 17:42
+# Generated by Django 3.2.20 on 2023-12-15 11:16
 
 from django.conf import settings
 import django.core.validators
@@ -25,7 +25,7 @@ class Migration(migrations.Migration):
                 ('is_superuser', models.BooleanField(default=False, help_text='Designates that this user has all permissions without explicitly assigning them.', verbose_name='superuser status')),
                 ('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')),
-                ('first_name', models.CharField(blank=True, max_length=150, verbose_name='first name')),
+                ('first_name', models.CharField(max_length=150, verbose_name='first name')),
                 ('last_name', models.CharField(blank=True, max_length=150, null=True, verbose_name='last name')),
                 ('email', models.EmailField(blank=True, max_length=254, null=True, unique=True, verbose_name='email address')),
                 ('is_staff', models.BooleanField(default=False, help_text='Designates whether the user can log into this admin site.', verbose_name='staff status')),
@@ -114,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(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='teachers', to='user.school')),
+                ('school', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='teachers', to='user.school')),
             ],
             options={
                 'verbose_name': 'teacher',
diff --git a/codeforlife/user/models/otp_bypass_token.py b/codeforlife/user/models/otp_bypass_token.py
index 164f59b0..6fd303c8 100644
--- a/codeforlife/user/models/otp_bypass_token.py
+++ b/codeforlife/user/models/otp_bypass_token.py
@@ -65,10 +65,7 @@ def key(otp_bypass_token: OtpBypassToken):
             for user_id, group in groupby(otp_bypass_tokens, key=key):
                 if (
                     len(list(group))
-                    + OtpBypassToken.objects.filter(
-                        user_id=user_id,
-                        delete_after__isnull=True,
-                    ).count()
+                    + OtpBypassToken.objects.filter(user_id=user_id).count()
                     > OtpBypassToken.max_count
                 ):
                     raise OtpBypassToken.max_count_validation_error
@@ -99,10 +96,7 @@ class Meta(TypedModelMeta):
     def save(self, *args, **kwargs):
         if self.id is None:
             if (
-                OtpBypassToken.objects.filter(
-                    user=self.user,
-                    delete_after__isnull=True,
-                ).count()
+                OtpBypassToken.objects.filter(user=self.user).count()
                 >= OtpBypassToken.max_count
             ):
                 raise OtpBypassToken.max_count_validation_error
@@ -116,10 +110,10 @@ def check_token(self, token: str):
             token: Token to check.
 
         Returns:
-            A boolean designating if the token is matches.
+            A boolean designating if the token matches.
         """
 
-        if not self.delete_after and check_password(token, self.token):
+        if check_password(token, self.token):
             self.delete()
             return True
         return False
diff --git a/codeforlife/user/models/student.py b/codeforlife/user/models/student.py
index 2ab241c3..63b938dd 100644
--- a/codeforlife/user/models/student.py
+++ b/codeforlife/user/models/student.py
@@ -69,10 +69,10 @@ def create_user(self, student: t.Dict[str, t.Any], **fields):
             """Create a user with a student profile.
 
             Args:
-                user: The user fields.
+                student: The student fields.
 
             Returns:
-                A student profile.
+                A user with a student profile.
             """
 
             return _user.User.objects.create_user(
@@ -94,7 +94,7 @@ def bulk_create_users(
                     profile belongs to.
 
             Returns:
-                A list of users that have been assigned their student profile.
+                A list of users with a student profile.
             """
 
             students = [student for (student, _) in student_users]
@@ -141,7 +141,7 @@ class Meta(TypedModelMeta):
 
     @property
     def teacher(self):
-        """The student's teacher (if they have one).
+        """The student's teacher.
 
         Returns:
             The student's class-teacher.
diff --git a/codeforlife/user/models/teacher.py b/codeforlife/user/models/teacher.py
index 51b0bc15..2d1d78ac 100644
--- a/codeforlife/user/models/teacher.py
+++ b/codeforlife/user/models/teacher.py
@@ -50,6 +50,7 @@ def create_user(self, teacher: t.Dict[str, t.Any], **fields):
         "user.School",
         related_name="teachers",
         null=True,
+        blank=True,
         on_delete=models.SET_NULL,
     )
 
@@ -72,6 +73,4 @@ def students(self) -> QuerySet["_student.Student"]:
             A queryset
         """
 
-        return _student.Student.objects.filter(
-            klass_id__in=list(self.classes.values_list("id", flat=True)),
-        )
+        return _student.Student.objects.filter(klass__in=self.classes.all())
diff --git a/codeforlife/user/models/user.py b/codeforlife/user/models/user.py
index c4fb54a1..72ce2135 100644
--- a/codeforlife/user/models/user.py
+++ b/codeforlife/user/models/user.py
@@ -32,7 +32,7 @@
 class User(AbstractBaseUser, WarehouseModel, PermissionsMixin):
     """A user within the CFL system."""
 
-    USERNAME_FIELD = "email"
+    USERNAME_FIELD = "id"
 
     class Manager(BaseUserManager["User"], WarehouseModel.Manager["User"]):
         """
@@ -116,7 +116,6 @@ def create_superuser(self, password: str, first_name: str, **fields):
     first_name = models.CharField(
         _("first name"),
         max_length=150,
-        blank=True,
     )
 
     last_name = models.CharField(

From 33d8e8bbd33f5e6725fd8e80fb7e93cbe0bb595b Mon Sep 17 00:00:00 2001
From: SKairinos <skairinos@gmail.com>
Date: Fri, 15 Dec 2023 16:09:38 +0000
Subject: [PATCH 2/3] feedback

---
 codeforlife/user/fixtures/schools.json      |  2 +-
 codeforlife/user/migrations/0001_initial.py |  6 +++---
 codeforlife/user/models/school.py           |  2 +-
 codeforlife/user/models/user.py             | 16 +++-------------
 4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/codeforlife/user/fixtures/schools.json b/codeforlife/user/fixtures/schools.json
index 8e19682d..7d75eef3 100644
--- a/codeforlife/user/fixtures/schools.json
+++ b/codeforlife/user/fixtures/schools.json
@@ -5,7 +5,7 @@
     "fields": {
       "last_saved_at": "2023-01-01 00:00:00.0+00:00",
       "name": "Example School",
-      "country": "UK",
+      "country": "GB",
       "uk_county": "Surrey"
     }
   }
diff --git a/codeforlife/user/migrations/0001_initial.py b/codeforlife/user/migrations/0001_initial.py
index 33e9816b..561e97fd 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-15 11:16
+# Generated by Django 3.2.20 on 2023-12-15 16:05
 
 from django.conf import settings
 import django.core.validators
@@ -150,7 +150,7 @@ class Migration(migrations.Migration):
         ),
         migrations.AddConstraint(
             model_name='school',
-            constraint=models.CheckConstraint(check=models.Q(('uk_county__isnull', True), ('country', 'UK'), _connector='OR'), name='school__no_uk_county_if_country_not_uk'),
+            constraint=models.CheckConstraint(check=models.Q(('uk_county__isnull', True), ('country', 'GB'), _connector='OR'), name='school__no_uk_county_if_country_not_uk'),
         ),
         migrations.AddField(
             model_name='otpbypasstoken',
@@ -210,7 +210,7 @@ class Migration(migrations.Migration):
         ),
         migrations.AddConstraint(
             model_name='user',
-            constraint=models.CheckConstraint(check=models.Q(models.Q(('student__isnull', False), ('teacher__isnull', True)), models.Q(('student__isnull', True), ('teacher__isnull', False)), models.Q(('student__isnull', True), ('teacher__isnull', True)), _connector='OR'), name='user__profile'),
+            constraint=models.CheckConstraint(check=models.Q(('student__isnull', False), ('teacher__isnull', False), _negated=True), name='user__profile'),
         ),
         migrations.AddConstraint(
             model_name='user',
diff --git a/codeforlife/user/models/school.py b/codeforlife/user/models/school.py
index d5312922..7dbe7f2a 100644
--- a/codeforlife/user/models/school.py
+++ b/codeforlife/user/models/school.py
@@ -66,7 +66,7 @@ class Meta(TypedModelMeta):
         verbose_name_plural = _("schools")
         constraints = [
             models.CheckConstraint(
-                check=Q(uk_county__isnull=True) | Q(country="UK"),
+                check=Q(uk_county__isnull=True) | Q(country=Country.GB),
                 name="school__no_uk_county_if_country_not_uk",
             ),
         ]
diff --git a/codeforlife/user/models/user.py b/codeforlife/user/models/user.py
index 72ce2135..b29ce566 100644
--- a/codeforlife/user/models/user.py
+++ b/codeforlife/user/models/user.py
@@ -197,19 +197,9 @@ class Meta(TypedModelMeta):
         constraints = [
             # pylint: disable=unsupported-binary-operation
             models.CheckConstraint(
-                check=(
-                    Q(
-                        teacher__isnull=True,
-                        student__isnull=False,
-                    )
-                    | Q(
-                        teacher__isnull=False,
-                        student__isnull=True,
-                    )
-                    | Q(
-                        teacher__isnull=True,
-                        student__isnull=True,
-                    )
+                check=~Q(
+                    teacher__isnull=False,
+                    student__isnull=False,
                 ),
                 name="user__profile",
             ),

From 8e30eda6f1ee415b64c87274455b782498390fc7 Mon Sep 17 00:00:00 2001
From: SKairinos <skairinos@gmail.com>
Date: Fri, 15 Dec 2023 17:17:56 +0000
Subject: [PATCH 3/3] student cannot be a superuser

---
 codeforlife/user/migrations/0001_initial.py | 10 +++-
 codeforlife/user/models/user.py             | 14 +++++
 codeforlife/user/tests/models/test_user.py  | 61 ++++++++++++---------
 3 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/codeforlife/user/migrations/0001_initial.py b/codeforlife/user/migrations/0001_initial.py
index 561e97fd..b346fa28 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-15 16:05
+# Generated by Django 3.2.20 on 2023-12-15 17:11
 
 from django.conf import settings
 import django.core.validators
@@ -220,4 +220,12 @@ class Migration(migrations.Migration):
             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'),
         ),
+        migrations.AddConstraint(
+            model_name='user',
+            constraint=models.CheckConstraint(check=models.Q(('is_staff', True), ('student__isnull', False), _negated=True), name='user__is_staff'),
+        ),
+        migrations.AddConstraint(
+            model_name='user',
+            constraint=models.CheckConstraint(check=models.Q(('is_superuser', True), ('student__isnull', False), _negated=True), name='user__is_superuser'),
+        ),
     ]
diff --git a/codeforlife/user/models/user.py b/codeforlife/user/models/user.py
index b29ce566..5206df39 100644
--- a/codeforlife/user/models/user.py
+++ b/codeforlife/user/models/user.py
@@ -239,6 +239,20 @@ class Meta(TypedModelMeta):
                 ),
                 name="user__last_name",
             ),
+            models.CheckConstraint(
+                check=~Q(
+                    student__isnull=False,
+                    is_staff=True,
+                ),
+                name="user__is_staff",
+            ),
+            models.CheckConstraint(
+                check=~Q(
+                    student__isnull=False,
+                    is_superuser=True,
+                ),
+                name="user__is_superuser",
+            ),
             # pylint: enable=unsupported-binary-operation
         ]
 
diff --git a/codeforlife/user/tests/models/test_user.py b/codeforlife/user/tests/models/test_user.py
index 005690a9..11633193 100644
--- a/codeforlife/user/tests/models/test_user.py
+++ b/codeforlife/user/tests/models/test_user.py
@@ -165,10 +165,44 @@ def test_constraints__last_name__indy(self):
         with self.assert_raises_integrity_error():
             User.objects.create_user(
                 password="password",
-                first_name="teacher",
+                first_name="Indiana",
                 email="independent@codeforlife.com",
             )
 
+    def test_constraints__is_staff(self):
+        """
+        Students cannot be a staff user.
+        """
+
+        with self.assert_raises_integrity_error():
+            User.objects.create_user(
+                first_name="Indiana",
+                password="password",
+                student=Student.objects.create(
+                    auto_gen_password="password",
+                    klass=self.klass__AB123,
+                    school=self.school__1,
+                ),
+                is_staff=True,
+            )
+
+    def test_constraints__is_superuser(self):
+        """
+        Students cannot be a super user.
+        """
+
+        with self.assert_raises_integrity_error():
+            User.objects.create_user(
+                first_name="Indiana",
+                password="password",
+                student=Student.objects.create(
+                    auto_gen_password="password",
+                    klass=self.klass__AB123,
+                    school=self.school__1,
+                ),
+                is_superuser=True,
+            )
+
     def test_objects__create(self):
         """
         Cannot call objects.create.
@@ -263,31 +297,6 @@ def test_objects__create_superuser__teacher(self):
         assert user.is_staff
         assert user.is_superuser
 
-    def test_objects__create_superuser__student(self):
-        """
-        Create a student super user.
-        """
-
-        user_fields = {
-            "first_name": "first_name",
-            "password": "password",
-            "student": Student.objects.create(
-                auto_gen_password="password",
-                klass=self.klass__AB123,
-                school=self.school__1,
-            ),
-        }
-
-        user = User.objects.create_superuser(
-            **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"])
-        assert user.student == user_fields["student"]
-        assert user.is_staff
-        assert user.is_superuser
-
     def test_objects__create_superuser__indy(self):
         """
         Create an independent super user.