feat!: new data models

[SKairinos]

This change is 

[faucomte97]

Reviewed 37 of 40 files at r1, all commit messages.
Reviewable status: 37 of 40 files reviewed, 23 unresolved discussions (waiting on @SKairinos)

---

Pipfile line 7 at r1 (raw file):

[packages]
django = "==3.2.20"

Optional, update to 3.2.23

---

Pipfile line 24 at r1 (raw file):

aimmo = "==2.10.6" # TODO: remove
rapid-router = "==5.11.3" # TODO: remove
phonenumbers = "==8.12.12" # TODO: remove

All these should be removed too

Code quote:

importlib-metadata = "==4.13.0" # TODO: remove. needed by old portal
django-formtools = "==2.2" # TODO: remove. needed by old portal
django-otp = "==1.0.2" # TODO: remove. needed by old portal
# https://pypi.org/user/codeforlife/
cfl-common = "==6.37.1" # TODO: remove
codeforlife-portal = "==6.37.1" # TODO: remove
aimmo = "==2.10.6" # TODO: remove
rapid-router = "==5.11.3" # TODO: remove
phonenumbers = "==8.12.12" # TODO: remove

---

codeforlife/models/__init__.py line 51 at r1 (raw file):

            Returns:
                The number of models updated.

The number of models matched, more so than the number of models "updated": https://docs.djangoproject.com/en/3.2/ref/models/querysets/#update

---

codeforlife/models/__init__.py line 57 at r1 (raw file):

            return super().update(**kwargs)

        def delete(self, wait: t.Optional[timedelta] = None):

Having trouble understanding how this function works, maybe you can re-explain it to me.

---

codeforlife/models/__init__.py line 91 at r1 (raw file):

        def filter(self, *args, **kwargs):
            """A stub that return our custom queryset."""

returns

---

codeforlife/models/__init__.py line 99 at r1 (raw file):

        def exclude(self, *args, **kwargs):
            """A stub that return our custom queryset."""

returns

---

codeforlife/models/__init__.py line 107 at r1 (raw file):

        def all(self):
            """A stub that return our custom queryset."""

returns

---

codeforlife/tests/model.py line 30 at r1 (raw file):

        return t.get_args(cls.__orig_bases__[0])[  # type: ignore[attr-defined]
            0
        ]

Why this format?

Code quote:

        return t.get_args(cls.__orig_bases__[0])[  # type: ignore[attr-defined]
            0
        ]

---

codeforlife/user/models/klass.py line 19 at r1 (raw file):

from . import school as _school
from . import student as _student
from . import teacher as _teacher

Two questions here I need you to remind me the answers for:

1. Why are you importing them as _x? Was it to avoid circular imports?
2. Where are these imports used? Is it in the typing strings only? If so, do you need to import them if it's only strings?

Code quote:

from . import school as _school
from . import student as _student
from . import teacher as _teacher

---

codeforlife/user/models/otp_bypass_token.py line 70 at r1 (raw file):

                    + OtpBypassToken.objects.filter(
                        user_id=user_id,
                        delete_after__isnull=True,

Where does delete_after come from? This model doesn't extend WarehouseModel.

---

codeforlife/user/models/otp_bypass_token.py line 119 at r1 (raw file):

        Returns:
            A boolean designating if the token is matches.

remove "is"

---

codeforlife/user/models/school.py line 36 at r1 (raw file):

    # Shortcuts for convenience.
    Country = Country
    UkCounty = UkCounty

Please re-explain theses shortcuts to me, I forgot what they do.

Code quote:

    # Shortcuts for convenience.
    Country = Country
    UkCounty = UkCounty

---

codeforlife/user/models/student.py line 27 at r1 (raw file):

    # pylint: disable-next=missing-class-docstring
    class Manager(WarehouseModel.Manager["Student"]):
        def create(  # type: ignore[override]

Please run me through the process of those 4 create functions again because I don't understand a) why we're passing the auto gen password and b) why we're passing in student instances when we're creating students

---

codeforlife/user/models/student.py line 144 at r1 (raw file):

    @property
    def teacher(self):
        """The student's teacher (if they have one).

They'll always have one.

---

codeforlife/user/models/teacher.py line 52 at r1 (raw file):

        "user.School",
        related_name="teachers",
        null=True,

Could you please make this blank=True as well, if I remember correctly it's so that we can edit Teacher objects in the admin without it complaining that the school field is blank.

---

codeforlife/user/models/teacher.py line 72 at r1 (raw file):

        Returns:
            A queryset

Just a thought as I read this - some of these docstings + typings combos seem a bit much sometimes. For example here, it's pretty clear purely from the typings that the prop returns a QuerySet of Students, feels redundant to mention it in the docstring.

---

codeforlife/user/models/teacher.py line 76 at r1 (raw file):

        return _student.Student.objects.filter(
            klass_id__in=list(self.classes.values_list("id", flat=True)),

Would doing something like klass_in=self.classes not work?

---

codeforlife/user/models/user.py line 35 at r1 (raw file):

    """A user within the CFL system."""

    USERNAME_FIELD = "email"

How does this work with Student users, since the email is optional? In our current app, the Student Manager generates a random username value for a new student.

---

codeforlife/user/models/user.py line 119 at r1 (raw file):

        _("first name"),
        max_length=150,
        blank=True,

Why?

---

codeforlife/user/models/user.py line 136 at r1 (raw file):

    )

    is_staff = models.BooleanField(

I might have already asked you this, but if is_staff is here, where is is_superuser?

---

codeforlife/user/models/user.py line 149 at r1 (raw file):

        help_text=_(
            "Designates whether this user should be treated as active."
            " Unselect this instead of deleting accounts."

I thought we were going to delete accounts once users are no longer active?

---

codeforlife/user/models/user.py line 192 at r1 (raw file):

        null=True,
        editable=False,
        on_delete=models.CASCADE,

Not sure about this one, only because of the case where students become independent - in this case, I imagine, the Student object would get deleted but we'd like to retain the associated User object.

---

codeforlife/user/models/user.py line 200 at r1 (raw file):

        constraints = [
            # pylint: disable=unsupported-binary-operation
            models.CheckConstraint(

It seems like you can do "nots" with Q lookups using ~: https://docs.djangoproject.com/en/3.2/topics/db/queries/#complex-lookups-with-q-objects

If so, let's replace this with simply ~Q(teacher__isnull=False, student__isnull=False)

[SKairinos]

Reviewable status: 37 of 40 files reviewed, 23 unresolved discussions (waiting on @faucomte97)

---

Pipfile line 7 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Optional, update to 3.2.23

will update in my next PR

---

Pipfile line 24 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

All these should be removed too

I removed these in my next PR

---

codeforlife/models/__init__.py line 51 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

The number of models matched, more so than the number of models "updated": https://docs.djangoproject.com/en/3.2/ref/models/querysets/#update

Done.

---

codeforlife/models/__init__.py line 57 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Having trouble understanding how this function works, maybe you can re-explain it to me.

By default, deleting warehouse models only schedules them for deletion by setting delete_after. You need to specify how long to wait before the data is actually deleted (schedule = now + wait). You can set a class-level default for the delete's wait timedelta (delete_wait). Or you can set it when calling delete in case you don't want to the default. Finally, if you want to actually delete (and not just schedule it), you call .delete(wait=timedelta()). The empty timedelta means your wait is basically 0 (there is no wait)

---

codeforlife/models/__init__.py line 91 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

returns

fixed in my next PR

---

codeforlife/models/__init__.py line 99 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

returns

fixed in my next PR

---

codeforlife/models/__init__.py line 107 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

returns

fixed in my next PR

---

codeforlife/tests/model.py line 30 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why this format?

Agreed its weird but that's how black does it. The formaters, linters and type-checkers sometimes want weird things.

---

codeforlife/user/models/klass.py line 19 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Two questions here I need you to remind me the answers for:

1. Why are you importing them as _x? Was it to avoid circular imports?
2. Where are these imports used? Is it in the typing strings only? If so, do you need to import them if it's only strings?

Question 1: Good question! I used private imports to:

1. discourage importing sub imports. for example, you shouldn't do this:
   from codeforlife.user.models.teacher import user
   instead it's private to communicate you shouldn't do this
   from codeforlife.user.models.teacher import _user

2. it allows me to reuse the models name for a variable name, instead of it already being used as a package name. for exanmple:
   from . import user as _user
   user = _user.User.objects.get(pk=1)

Question 2

In this case, yes, it's only used for type hinting. Yes, we still need to import the modules even if it's only used in string-based type hints. The python interpreter still needs the imports to follow the references. It's just by wrapping them in strings, you're lazy-loading the type hints at the end instead of order of execution.

---

codeforlife/user/models/otp_bypass_token.py line 70 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Where does delete_after come from? This model doesn't extend WarehouseModel.

Done. It was a mistake!

---

codeforlife/user/models/otp_bypass_token.py line 119 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

remove "is"

Done.

---

codeforlife/user/models/school.py line 36 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Please re-explain theses shortcuts to me, I forgot what they do.

It helps you do the following.
School.objects.create(
country=School.Country.US
)

---

codeforlife/user/models/student.py line 27 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Please run me through the process of those 4 create functions again because I don't understand a) why we're passing the auto gen password and b) why we're passing in student instances when we're creating students

Of course!

We need to pass in the auto-gen password because when creating a student we need to know the unhashed auto-gen password to return it back to the UI and generate login links. Then, when saving to the db, we hash the password for security.

The 4 different helper functions do the following:

create - creates a student with a hashed auto-gen password
bulk_create - creates many students with a hashed auto-gen password
create_user - creates a user with a student profile.
bulk_create_users - creates many users with a student profile.

In general, when bulk creating, you need to pass in instances the object your creating. Bulk create's responsibility is just to mass-save the instances to the db, not create the class instances. It's 'creating' data rows in the db.
https://docs.djangoproject.com/en/5.0/ref/models/querysets/#bulk-create

---

codeforlife/user/models/student.py line 144 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

They'll always have one.

Done.

---

codeforlife/user/models/teacher.py line 52 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Could you please make this blank=True as well, if I remember correctly it's so that we can edit Teacher objects in the admin without it complaining that the school field is blank.

Done.

---

codeforlife/user/models/teacher.py line 72 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Just a thought as I read this - some of these docstings + typings combos seem a bit much sometimes. For example here, it's pretty clear purely from the typings that the prop returns a QuerySet of Students, feels redundant to mention it in the docstring.

I know what you mean. It can be a bit tedious sometimes as some things are self explanatory. You can do:

pylint: disable-next:missing-function-docstring

this will ignore docstring for the next function. We can make it so we add docstrings for things that need explanation. Need to be careful to find the balance and not become lazy by over-omitting doc strings

---

codeforlife/user/models/teacher.py line 76 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Would doing something like klass_in=self.classes not work?

Done. You're genius! That worked!

---

codeforlife/user/models/user.py line 35 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

How does this work with Student users, since the email is optional? In our current app, the Student Manager generates a random username value for a new student.

Done. You make a good point. I used the id field instead.

---

codeforlife/user/models/user.py line 119 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why?

Done. Mistake.

---

codeforlife/user/models/user.py line 136 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I might have already asked you this, but if is_staff is here, where is is_superuser?

In the permissions mixin.

---

codeforlife/user/models/user.py line 149 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I thought we were going to delete accounts once users are no longer active?

No. A user can be inactive but still retained. For example, you create your account but haven't verified your email. We only delete your account if is_active=False and last_login > 3 years ago.

---

codeforlife/user/models/user.py line 192 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Not sure about this one, only because of the case where students become independent - in this case, I imagine, the Student object would get deleted but we'd like to retain the associated User object.

You are right to an extent. When making students independent, we shouldn't do

student.delete(now=timedelta())

we should do:

user.student = None # NEED to make this none first!!
student.delete(now=timedelta()) # now user won't be cascade deleted

---

codeforlife/user/models/user.py line 200 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

It seems like you can do "nots" with Q lookups using ~: https://docs.djangoproject.com/en/3.2/topics/db/queries/#complex-lookups-with-q-objects

If so, let's replace this with simply ~Q(teacher__isnull=False, student__isnull=False)

awesome! Thank you for sharing! will keep in mind for the future

[SKairinos]

Reviewable status: 37 of 40 files reviewed, 23 unresolved discussions (waiting on @faucomte97)

---

codeforlife/user/models/user.py line 192 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

You are right to an extent. When making students independent, we shouldn't do

student.delete(now=timedelta())

we should do:

user.student = None # NEED to make this none first!!
student.delete(now=timedelta()) # now user won't be cascade deleted

correction:

user.student = None # NEED to make this none first!!
user.save()
student.delete(now=timedelta()) # now user won't be cascade deleted

[faucomte97]

Reviewed 5 of 6 files at r2.
Reviewable status: 36 of 40 files reviewed, 4 unresolved discussions (waiting on @SKairinos)

---

codeforlife/models/__init__.py line 57 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

By default, deleting warehouse models only schedules them for deletion by setting delete_after. You need to specify how long to wait before the data is actually deleted (schedule = now + wait). You can set a class-level default for the delete's wait timedelta (delete_wait). Or you can set it when calling delete in case you don't want to the default. Finally, if you want to actually delete (and not just schedule it), you call .delete(wait=timedelta()). The empty timedelta means your wait is basically 0 (there is no wait)

I'm still confused. I'm confused by the fact that the delete only happens if wait == timedelta() Where is the delete that happens if wait > delete_after?

---

codeforlife/user/models/klass.py line 19 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Question 1: Good question! I used private imports to:

1. discourage importing sub imports. for example, you shouldn't do this:
   from codeforlife.user.models.teacher import user
   instead it's private to communicate you shouldn't do this
   from codeforlife.user.models.teacher import _user

2. it allows me to reuse the models name for a variable name, instead of it already being used as a package name. for exanmple:
   from . import user as _user
   user = _user.User.objects.get(pk=1)

Question 2

In this case, yes, it's only used for type hinting. Yes, we still need to import the modules even if it's only used in string-based type hints. The python interpreter still needs the imports to follow the references. It's just by wrapping them in strings, you're lazy-loading the type hints at the end instead of order of execution.

But then, why not just import User for example so you could do user = User.objects.get(pk=1), which is cleaner?

---

codeforlife/user/models/user.py line 149 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

No. A user can be inactive but still retained. For example, you create your account but haven't verified your email. We only delete your account if is_active=False and last_login > 3 years ago.

Why don't we just use is_active to check for email verification and last_login for account deletion? We don't need to set is_active to False again if last_login is over 3 years.

---

codeforlife/user/models/user.py line 200 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

awesome! Thank you for sharing! will keep in mind for the future

Still need to do

[faucomte97]

Reviewed 1 of 6 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: 37 of 40 files reviewed, all discussions resolved

[faucomte97]

Reviewed 3 of 40 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)

---

codeforlife/user/tests/models/test_user.py line 168 at r3 (raw file):

            User.objects.create_user(
                password="password",
                first_name="teacher",

I know it doesn't really matter but let's put Indiana here or independent

---

codeforlife/user/tests/models/test_user.py line 266 at r3 (raw file):

        assert user.is_superuser

    def test_objects__create_superuser__student(self):

Actually that reminds of something. Do we want to allow for students (of any kind) to be superusers? So far we've only ever had Teacher objects for our superuser accounts, mainly because they also require 2FA to access the admin. Do we want to remove the functionality to create student superusers?

[SKairinos]

Reviewable status: 37 of 40 files reviewed, 2 unresolved discussions (waiting on @faucomte97)

---

codeforlife/models/__init__.py line 57 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I'm still confused. I'm confused by the fact that the delete only happens if wait == timedelta() Where is the delete that happens if wait > delete_after?

Done.

---

codeforlife/user/models/klass.py line 19 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

But then, why not just import User for example so you could do user = User.objects.get(pk=1), which is cleaner?

Done.

---

codeforlife/user/models/user.py line 149 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why don't we just use is_active to check for email verification and last_login for account deletion? We don't need to set is_active to False again if last_login is over 3 years.

Done.

---

codeforlife/user/tests/models/test_user.py line 168 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I know it doesn't really matter but let's put Indiana here or independent

Done.

---

codeforlife/user/tests/models/test_user.py line 266 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Actually that reminds of something. Do we want to allow for students (of any kind) to be superusers? So far we've only ever had Teacher objects for our superuser accounts, mainly because they also require 2FA to access the admin. Do we want to remove the functionality to create student superusers?

Done.

[faucomte97]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

[SKairinos]

will reopen when ready to merge