New data model permissions

[SKairinos]

This change is 

[faucomte97]

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

---

codeforlife/user/permissions/is_teacher.py line 29 at r1 (raw file):

                teacher. Else, check if the user is the specific teacher.
            is_admin: If the teacher is an admin. If None, don't check if the
                teacher is an admin. Else, check if the teacher is (not) an

what do you mean "check if not an admin", isn't it the other way around?

---

codeforlife/user/permissions/is_teacher.py line 45 at r1 (raw file):

            and (
                self.is_admin is None
                or t.cast(Teacher, user.teacher).is_admin == self.is_admin

Does the code not recognise user.teacher, hence the casting? 😕

---

codeforlife/user/views/user.py line 24 at r1 (raw file):

            return User.objects.none()

        if user.student:

Wanted to bring this up again actually. In other cases we've been using user.student is not None but here we're doing just user.student, for example. While I was advocating for the latter because it's simpler and easier to read, I remembered that they are not technically equivalent as Python will go through a series of checks if we don't specify is (not) None. If we think it's safe enough to leave it as it is though, we can see. Happy to discuss this further.

[SKairinos]

codeforlife/user/views/user.py line 24 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Wanted to bring this up again actually. In other cases we've been using user.student is not None but here we're doing just user.student, for example. While I was advocating for the latter because it's simpler and easier to read, I remembered that they are not technically equivalent as Python will go through a series of checks if we don't specify is (not) None. If we think it's safe enough to leave it as it is though, we can see. Happy to discuss this further.

I agree with you. Let's make it explicit with "is not None" to be safe.

[SKairinos]

Reviewable status: 38 of 39 files reviewed, 3 unresolved discussions (waiting on @faucomte97)

---

codeforlife/user/permissions/is_teacher.py line 29 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

what do you mean "check if not an admin", isn't it the other way around?

if is_admin=None, it will not check admin status.
if is_admin=True, it will check that the teacher is an admin
if is_admin=False, it will check that the teacher is not an admin.

---

codeforlife/user/permissions/is_teacher.py line 45 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Does the code not recognise user.teacher, hence the casting? 😕

yes, exactly. Normally, teacher is None or instance of Teacher(). However, mypy does not know that teacher is not None at this point in the if check because mypy is ignorant to Django's internal workings. But we know it will not be none so I cast it so it knows it's an instance of Teacher() and not None.

[faucomte97]

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

---

codeforlife/user/permissions/is_teacher.py line 29 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

if is_admin=None, it will not check admin status.
if is_admin=True, it will check that the teacher is an admin
if is_admin=False, it will check that the teacher is not an admin.

I see, so that (not) is a optional not in the sentence based on whether it's True or False. OK