From 5460bbfa86bc777211d630d28fa3e761d6f8948a Mon Sep 17 00:00:00 2001 From: Stefan Kairinos Date: Wed, 25 Sep 2024 14:05:12 +0100 Subject: [PATCH] fix: Portal frontend 52 (#136) * fix: class queryset and filters * search classes by their id or name * feedback --- codeforlife/user/filters/klass.py | 23 ++++++-- codeforlife/user/models/teacher.py | 6 +-- codeforlife/user/views/klass.py | 15 ++---- codeforlife/user/views/klass_test.py | 79 +++++++++++++++++----------- 4 files changed, 71 insertions(+), 52 deletions(-) diff --git a/codeforlife/user/filters/klass.py b/codeforlife/user/filters/klass.py index 2e85aa98..10bfcc91 100644 --- a/codeforlife/user/filters/klass.py +++ b/codeforlife/user/filters/klass.py @@ -3,12 +3,29 @@ Created on 24/07/2024 at 13:19:57(+01:00). """ -from ...filters import FilterSet -from ..models import Class +from django.db.models import Q # isort: skip +from django.db.models.query import QuerySet # isort: skip +from django_filters import ( # type: ignore[import-untyped] # isort: skip + rest_framework as filters, +) + +from ...filters import FilterSet # isort: skip +from ..models import Class # isort: skip # pylint: disable-next=missing-class-docstring class ClassFilterSet(FilterSet): + _id = filters.CharFilter(method="_id__method") + _id__method = FilterSet.make_exclude_field_list_method("access_code") + + id_or_name = filters.CharFilter(method="id_or_name__method") + + def id_or_name__method(self, queryset: QuerySet[Class], _: str, value: str): + """Get classes where the id or the name contain a substring.""" + return queryset.filter( + Q(access_code__icontains=value) | Q(name__icontains=value) + ) + class Meta: model = Class - fields = ["teacher"] + fields = ["_id", "teacher", "id_or_name"] diff --git a/codeforlife/user/models/teacher.py b/codeforlife/user/models/teacher.py index 3ce2d699..660220da 100644 --- a/codeforlife/user/models/teacher.py +++ b/codeforlife/user/models/teacher.py @@ -66,11 +66,7 @@ def students(self): @property def classes(self): """All classes the teacher can query.""" - return ( - Class.objects.filter(teacher__school=self.school) - if self.is_admin - else self.class_teacher.all() - ) + return Class.objects.filter(teacher__school=self.school) @property def indy_users(self): diff --git a/codeforlife/user/views/klass.py b/codeforlife/user/views/klass.py index ddfdd71a..3da9c783 100644 --- a/codeforlife/user/views/klass.py +++ b/codeforlife/user/views/klass.py @@ -22,14 +22,9 @@ class ClassViewSet(ModelViewSet[RequestUser, Class]): def get_permissions(self): # Only school-teachers can list classes. if self.action == "list": - return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] + return [IsTeacher(in_school=True)] - return [ - OR( - IsStudent(), - OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), - ) - ] + return [OR(IsStudent(), IsTeacher(in_school=True))] # pylint: disable-next=missing-function-docstring def get_queryset(self): @@ -37,8 +32,4 @@ def get_queryset(self): if user.student: return Class.objects.filter(students=user.student) - user = self.request.school_teacher_user - if user.teacher.is_admin: - return Class.objects.filter(teacher__school=user.teacher.school) - - return Class.objects.filter(teacher=user.teacher) + return self.request.school_teacher_user.teacher.classes diff --git a/codeforlife/user/views/klass_test.py b/codeforlife/user/views/klass_test.py index c84bcd25..f9275a70 100644 --- a/codeforlife/user/views/klass_test.py +++ b/codeforlife/user/views/klass_test.py @@ -5,13 +5,7 @@ from ...permissions import OR from ...tests import ModelViewSetTestCase -from ..models import ( - AdminSchoolTeacherUser, - Class, - NonAdminSchoolTeacherUser, - StudentUser, - User, -) +from ..models import AdminSchoolTeacherUser, Class, StudentUser, User from ..permissions import IsStudent, IsTeacher from ..views import ClassViewSet @@ -34,21 +28,14 @@ def setUp(self): def test_get_permissions__list(self): """Only school-teachers can list classes.""" self.assert_get_permissions( - permissions=[ - OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) - ], + permissions=[IsTeacher(in_school=True)], action="list", ) def test_get_permissions__retrieve(self): """Anyone in a school can retrieve a class.""" self.assert_get_permissions( - permissions=[ - OR( - IsStudent(), - OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), - ) - ], + permissions=[OR(IsStudent(), IsTeacher(in_school=True))], action="retrieve", ) @@ -64,10 +51,8 @@ def test_get_queryset__student(self): request=self.client.request_factory.get(user=user), ) - def test_get_queryset__teacher__admin(self): - """ - Admin-teacher-users can only target all the classes in their school. - """ + def test_get_queryset__teacher(self): + """Teacher-users can only target all the classes in their school.""" user = AdminSchoolTeacherUser.objects.first() assert user @@ -76,18 +61,6 @@ def test_get_queryset__teacher__admin(self): request=self.client.request_factory.get(user=user), ) - def test_get_queryset__teacher__non_admin(self): - """ - Non-admin-teacher-users can only target all the classes they teach. - """ - user = NonAdminSchoolTeacherUser.objects.first() - assert user - - self.assert_get_queryset( - values=user.teacher.classes, - request=self.client.request_factory.get(user=user), - ) - # test: actions def test_retrieve(self): @@ -107,6 +80,48 @@ def test_list(self): self.client.login_as(user) self.client.list(models=user.teacher.classes.all()) + def test_list___id(self): + """Can successfully list classes in a school, excluding some by ID.""" + user = self.admin_school_teacher_user + assert user + + classes = user.teacher.classes + assert classes.count() >= 2 + + first_class = classes[0] + classes = classes[1:] + + self.client.login_as(user) + self.client.list( + models=classes, + filters={"_id": first_class.access_code}, + ) + + def test_list__id_or_name(self): + """ + Can successfully list classes in a school, filtered by their ID or name. + """ + user = self.admin_school_teacher_user + assert user + + klass = user.teacher.classes.first() + assert klass + + partial_access_code = klass.access_code[:-1] + partial_name = klass.name[:-1] + + self.client.login_as(user) + self.client.list( + models=user.teacher.classes.filter( + access_code__icontains=partial_access_code + ), + filters={"id_or_name": partial_access_code}, + ) + self.client.list( + models=user.teacher.classes.filter(name__icontains=partial_name), + filters={"id_or_name": partial_name}, + ) + def test_list__teacher(self): """Can successfully list classes assigned to a teacher.""" user = self.admin_school_teacher_user