fix: Portal frontend19

[SKairinos]

This change is 

[faucomte97]

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

---

codeforlife/user/filters/user.py line 44 at r1 (raw file):

        return queryset.filter(
            first_name__icontains=first_name
        ) | queryset.filter(last_name__icontains=last_name)

Should we do a Q lookup here instead of performing 2 filters? I'm not 100% sure it's better but I have a feeling it is. Something like this:

return queryset.filter(Q(first_name__icontains=first_name) | Q(last_name__icontains=last_name))

Code quote:

        return queryset.filter(
            first_name__icontains=first_name
        ) | queryset.filter(last_name__icontains=last_name)

---

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

                new_student__isnull=True,
                new_teacher__school=self.school,
            )

Same comment here about the Q lookup

Code quote:

            # student-users
            User.objects.filter(
                new_teacher__isnull=True,
                **(
                    {"new_student__class_field__teacher__school": self.school}
                    if self.is_admin
                    else {"new_student__class_field__teacher": self}
                )
            )
            # school-teacher-users
            | User.objects.filter(
                new_student__isnull=True,
                new_teacher__school=self.school,
            )

---

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

        self.client.list(
            models=classes,
            filters={"teacher": str(user.teacher.id)},

This might be a silly question, you probably just need to remind me how this works exactly.

What is the "point" of the this filters arg here exactly, isn't classes already filtered to be just the classes that the teacher owns? (on the second line of this def)

Or, is the point that classes should actually be all the classes in the school first, instead of only the teacher's classes?

---

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

        self.client.list(
            models=school_teacher_users,
            filters={"only_teachers": str(True)},

why str(True) and not just "True"?

[SKairinos]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faucomte97)

---

codeforlife/user/filters/user.py line 44 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Should we do a Q lookup here instead of performing 2 filters? I'm not 100% sure it's better but I have a feeling it is. Something like this:

return queryset.filter(Q(first_name__icontains=first_name) | Q(last_name__icontains=last_name))

functionally equivalent but since it's more compact, I went with your approach

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

Same comment here about the Q lookup

functionally equivalent but since it's more compact, I went with your approach

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

This might be a silly question, you probably just need to remind me how this works exactly.

What is the "point" of the this filters arg here exactly, isn't classes already filtered to be just the classes that the teacher owns? (on the second line of this def)

Or, is the point that classes should actually be all the classes in the school first, instead of only the teacher's classes?

Admin teachers can list all the classes in a school. If the admin wants to get a subset of those classes, where the class teacher = {ID}, they can use this filter.

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

why str(True) and not just "True"?

Filters are search params. Therefore, we need the string equivalent of True. Search params values are always strings (a url is a string).

GET  .../api/users/?only_teachers=True

This will list only teacher users

[faucomte97]

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