diff --git a/backend/Pipfile b/backend/Pipfile index 06d3f99e..b0507c23 100644 --- a/backend/Pipfile +++ b/backend/Pipfile @@ -7,7 +7,7 @@ name = "pypi" # Before adding a new package, check it's not listed under [packages] at # https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile # Replace "{ref}" in the above URL with the ref set below. -codeforlife = {ref = "v0.13.4", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.13.6", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} # TODO: check if we need the below packages whitenoise = "==6.5.0" django-pipeline = "==2.0.8" @@ -34,7 +34,7 @@ google-cloud-container = "==2.3.0" # Before adding a new package, check it's not listed under [dev-packages] at # https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile # Replace "{ref}" in the above URL with the ref set below. -codeforlife = {ref = "v0.13.4", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.13.6", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} # TODO: check if we need the below packages django-selenium-clean = "==0.3.3" django-test-migrations = "==1.2.0" diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 5d6b7532..390b76b6 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "a4f65f80c5f383809d2b3acdc82ede3c6cd65b81150d4d9bcd0a8a19a4d1942c" + "sha256": "87409c54e5f41ceef8b81accc69edfa6fc7fd474d9c22c4e0d018938b57b52a1" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "e5801cf92dc7d3edb3e20aca76a031ca8d618f76" + "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" }, "codeforlife-portal": { "hashes": [ @@ -375,11 +375,11 @@ "grpc" ], "hashes": [ - "sha256:6fb380f49d19ee1d09a9722d0379042b7edb06c0112e4796c7a395078a043e71", - "sha256:7421474c39d396a74dfa317dddbc69188f2336835f526087c7648f91105e32ff" + "sha256:3399c92887a97d33038baa4bfd3bf07acc05d474b0171f333e1f641c1364e552", + "sha256:52bcc9d9937735f8a3986fa0bbf9135ae9cf5393a722387e5eced520e39c774a" ], "markers": "python_version >= '3.7'", - "version": "==1.34.0" + "version": "==1.34.1" }, "google-auth": { "hashes": [ @@ -1512,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "e5801cf92dc7d3edb3e20aca76a031ca8d618f76" + "ref": "9fd3549f852745032d9d43174ec2f25ae29b6f8f" }, "codeforlife-portal": { "hashes": [ @@ -2492,7 +2492,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version < '3.10'", + "markers": "python_version >= '3.7'", "version": "==7.2.1" }, "pytest-cov": { diff --git a/backend/api/tests/views/test_klass.py b/backend/api/tests/views/test_klass.py index 550a8c84..5d30d1a3 100644 --- a/backend/api/tests/views/test_klass.py +++ b/backend/api/tests/views/test_klass.py @@ -5,10 +5,10 @@ from datetime import timedelta -from codeforlife.permissions import AllowNone +from codeforlife.permissions import OR, AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import Class, Teacher -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsStudent, IsTeacher from django.utils import timezone from ...views import ClassViewSet @@ -20,63 +20,60 @@ class TestClassViewSet(ModelViewSetTestCase[Class]): model_view_set_class = ClassViewSet fixtures = ["school_1"] - def test_get_permissions__bulk(self): - """ - No one is allowed to perform bulk actions. - """ + # test: get permissions + def test_get_permissions__bulk(self): + """No one is allowed to perform bulk actions.""" self.assert_get_permissions( permissions=[AllowNone()], action="bulk", ) def test_get_permissions__create(self): - """ - Only a school-teacher can create a class. - """ - + """Only school-teachers can create a class.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[IsTeacher(in_school=True)], action="create", ) def test_get_permissions__update(self): - """ - Only a school-teacher can update a class. - """ - + """Only admin-teachers or class-teachers can update a class.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="update", ) def test_get_permissions__destroy(self): - """ - Only a school-teacher can destroy a class. - """ - + """Only admin-teachers or class-teachers can destroy a class.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="destroy", ) def test_get_permissions__list(self): - """ - Only a school-teacher can list classes. - """ - + """Only admin-teachers and class-teachers can list classes.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="list", ) def test_get_permissions__retrieve(self): """ - Any school-user can retrieve a class. + Only students, admin-teachers or class-teachers can retrieve a class. """ - self.assert_get_permissions( - permissions=[InSchool()], + permissions=[ + OR( + IsStudent(), + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), + ) + ], action="retrieve", ) @@ -104,10 +101,9 @@ def test_create__other(self): Teacher can create a class with another teacher as the class owner. """ - user = self.client.login_school_teacher( + user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", - is_admin=True, ) teacher = ( diff --git a/backend/api/tests/views/test_school.py b/backend/api/tests/views/test_school.py index f5c84f5c..03e644cd 100644 --- a/backend/api/tests/views/test_school.py +++ b/backend/api/tests/views/test_school.py @@ -3,10 +3,10 @@ Created on 02/02/2024 at 15:31:21(+00:00). """ -from codeforlife.permissions import NOT, AllowNone +from codeforlife.permissions import OR, AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import School -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsStudent, IsTeacher from ...views import SchoolViewSet @@ -37,7 +37,7 @@ def test_get_permissions__create(self): """Only teachers not in a school can create a school.""" self.assert_get_permissions( - permissions=[IsTeacher(), NOT(InSchool())], + permissions=[IsTeacher(in_school=False)], action="create", ) @@ -45,7 +45,7 @@ def test_get_permissions__update(self): """Only admin-teachers in a school can update a school.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="update", ) @@ -53,7 +53,7 @@ def test_get_permissions__retrieve(self): """Anyone in a school can retrieve a school.""" self.assert_get_permissions( - permissions=[InSchool()], + permissions=[OR(IsStudent(), IsTeacher(in_school=True))], action="retrieve", ) @@ -76,8 +76,7 @@ def test_create(self): def test_partial_update(self): """Can successfully update a school.""" - user = self.client.login_school_teacher( - is_admin=True, + user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", ) diff --git a/backend/api/tests/views/test_school_teacher_invitation.py b/backend/api/tests/views/test_school_teacher_invitation.py index 01bf2b25..97d0ec1d 100644 --- a/backend/api/tests/views/test_school_teacher_invitation.py +++ b/backend/api/tests/views/test_school_teacher_invitation.py @@ -6,7 +6,7 @@ from codeforlife.permissions import AllowNone from codeforlife.tests import ModelViewSetTestCase from codeforlife.user.models import User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from rest_framework import status from ...models import SchoolTeacherInvitation @@ -24,10 +24,9 @@ class TestSchoolTeacherInvitationViewSet( non_school_teacher_email = "teacher@noschool.com" def _login_admin_school_teacher(self): - return self.client.login_school_teacher( + return self.client.login_admin_school_teacher( email=self.school_admin_teacher_email, password="password", - is_admin=True, ) def setUp(self): @@ -60,35 +59,35 @@ def test_get_permissions__bulk(self): def test_get_permissions__create(self): """Only admin-teachers can create an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="create", ) def test_get_permissions__partial_update(self): """Only admin-teachers can update an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="partial_update", ) def test_get_permissions__retrieve(self): """Only admin-teachers can retrieve an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="retrieve", ) def test_get_permissions__list(self): """Only admin-teachers can list invitations.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="list", ) def test_get_permissions__destroy(self): """Only admin-teachers can destroy an invitation.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="destroy", ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index cb0d9bb4..41560f1c 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -5,9 +5,15 @@ import typing as t +from codeforlife.permissions import OR from codeforlife.tests import ModelViewSetTestCase -from codeforlife.user.models import Class, SchoolTeacherUser, User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.models import ( + AdminSchoolTeacherUser, + Class, + SchoolTeacherUser, + User, +) +from codeforlife.user.permissions import IsTeacher from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, default_token_generator, @@ -30,11 +36,15 @@ class TestUserViewSet(ModelViewSetTestCase[User]): school_teacher_email = "teacher@school1.com" indy_email = "indy@man.com" - def _login_school_teacher(self): - return self.client.login_school_teacher( + def setUp(self): + self.admin_school_teacher_user = AdminSchoolTeacherUser.objects.get( + email="admin.teacher@school1.com" + ) + + def _login_non_admin_school_teacher(self): + return self.client.login_non_admin_school_teacher( email=self.school_teacher_email, password="password", - is_admin=False, ) def _get_pk_and_token_for_user(self, email: str): @@ -43,17 +53,21 @@ def _get_pk_and_token_for_user(self, email: str): return user.pk, token + # test: get permissions + def test_get_permissions__bulk(self): """Only school-teachers can perform bulk actions.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="bulk", ) def test_get_permissions__partial_update__teacher(self): """Only admin-school-teachers can update a teacher.""" self.assert_get_permissions( - permissions=[IsTeacher(is_admin=True), InSchool()], + permissions=[IsTeacher(is_admin=True)], action="partial_update", request=self.client.request_factory.patch(data={"teacher": {}}), ) @@ -61,14 +75,48 @@ def test_get_permissions__partial_update__teacher(self): def test_get_permissions__partial_update__student(self): """Only school-teachers can update a student.""" self.assert_get_permissions( - permissions=[IsTeacher(), InSchool()], + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], action="partial_update", request=self.client.request_factory.patch(data={"student": {}}), ) + # test: get queryset + + def _test_get_queryset__bulk(self, request_method: str): + assert User.objects.filter( + new_teacher__school=self.admin_school_teacher_user.teacher.school + ).exists() + + student_users = list( + User.objects.filter( + new_student__class_field__teacher__school=( + self.admin_school_teacher_user.teacher.school + ) + ) + ) + assert student_users + + request = self.client.request_factory.generic( + request_method, user=self.admin_school_teacher_user + ) + + self.assert_get_queryset(student_users, action="bulk", request=request) + + def test_get_queryset__bulk__patch(self): + """Bulk partial-update can only target student-users.""" + self._test_get_queryset__bulk("patch") + + def test_get_queryset__bulk__delete(self): + """Bulk destroy can only target student-users.""" + self._test_get_queryset__bulk("delete") + + # test: bulk actions + def test_bulk_create__students(self): """Teacher can bulk create students.""" - user = self._login_school_teacher() + user = self._login_non_admin_school_teacher() klass: t.Optional[Class] = user.teacher.class_teacher.first() assert klass is not None @@ -89,6 +137,31 @@ def test_bulk_create__students(self): response.json() # TODO: make custom assertions and check for password + def test_bulk_destroy(self): + """School-teacher can bulk anonymize students.""" + self.client.login_as(self.admin_school_teacher_user) + + student_user_queryset = User.objects.filter( + new_student__class_field__teacher__school=( + self.admin_school_teacher_user.teacher.school + ), + ) + student_user_ids = list( + student_user_queryset.values_list("id", flat=True) + ) + assert student_user_ids + + self.client.bulk_destroy(student_user_ids, make_assertions=False) + + assert ( + len(student_user_ids) + == student_user_queryset.filter( + first_name="", is_active=False + ).count() + ) + + # test: reset password actions + def test_request_password_reset__invalid_email(self): """ Request password reset doesn't generate reset password URL if email @@ -202,12 +275,13 @@ def test_reset_password__patch__indy(self): self.client.patch(viewname, data={"password": "N3wPassword"}) self.client.login(email=self.indy_email, password="N3wPassword") + # test: generic actions + def test_partial_update__teacher(self): """Admin-school-teacher can update another teacher's profile.""" - admin_school_teacher_user = self.client.login_school_teacher( + admin_school_teacher_user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", - is_admin=True, ) other_school_teacher_user = ( diff --git a/backend/api/views/klass.py b/backend/api/views/klass.py index 17e5d1ed..58c0cac4 100644 --- a/backend/api/views/klass.py +++ b/backend/api/views/klass.py @@ -3,8 +3,8 @@ Created on 23/01/2024 at 17:53:37(+00:00). """ -from codeforlife.permissions import AllowNone -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.permissions import OR, AllowNone +from codeforlife.user.permissions import IsTeacher from codeforlife.user.views import ClassViewSet as _ClassViewSet from ..serializers import ClassSerializer @@ -19,7 +19,9 @@ def get_permissions(self): # Bulk actions not allowed for classes. if self.action == "bulk": return [AllowNone()] - if self.action in ["create", "update", "destroy"]: - return [IsTeacher(), InSchool()] + if self.action == "create": + return [IsTeacher(in_school=True)] + if self.action in ["update", "destroy"]: + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() diff --git a/backend/api/views/school.py b/backend/api/views/school.py index 30df17f5..f4c2400a 100644 --- a/backend/api/views/school.py +++ b/backend/api/views/school.py @@ -3,8 +3,8 @@ Created on 23/01/2024 at 17:53:50(+00:00). """ -from codeforlife.permissions import NOT, AllowNone -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.permissions import AllowNone +from codeforlife.user.permissions import IsTeacher from codeforlife.user.views import SchoolViewSet as _SchoolViewSet from ..serializers import SchoolSerializer @@ -21,9 +21,9 @@ def get_permissions(self): return [AllowNone()] # Only teachers not in a school can create a school. if self.action == "create": - return [IsTeacher(), NOT(InSchool())] + return [IsTeacher(in_school=False)] # Only admin-teachers in a school can update a school. if self.action == "update": - return [IsTeacher(is_admin=True), InSchool()] + return [IsTeacher(is_admin=True)] return super().get_permissions() diff --git a/backend/api/views/school_teacher_invitation.py b/backend/api/views/school_teacher_invitation.py index 2039981c..bff91cb3 100644 --- a/backend/api/views/school_teacher_invitation.py +++ b/backend/api/views/school_teacher_invitation.py @@ -6,7 +6,7 @@ from codeforlife.permissions import AllowNone from codeforlife.request import Request from codeforlife.user.models import User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from codeforlife.views import ModelViewSet from django.contrib.auth.hashers import check_password from rest_framework import status @@ -31,7 +31,7 @@ def get_permissions(self): "partial_update", "destroy", ]: - return [IsTeacher(is_admin=True), InSchool()] + return [IsTeacher(is_admin=True)] if self.action == "bulk": return [AllowNone()] diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 7a9346e1..cd0b80a4 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -5,9 +5,10 @@ import typing as t +from codeforlife.permissions import OR from codeforlife.request import Request from codeforlife.user.models import User -from codeforlife.user.permissions import InSchool, IsTeacher +from codeforlife.user.permissions import IsTeacher from codeforlife.user.views import UserViewSet as _UserViewSet from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, @@ -31,15 +32,27 @@ class UserViewSet(_UserViewSet): def get_permissions(self): if self.action == "bulk": - return [IsTeacher(), InSchool()] + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] if self.action == "partial_update": if "teacher" in self.request.data: - return [IsTeacher(is_admin=True), InSchool()] + return [IsTeacher(is_admin=True)] if "student" in self.request.data: - return [IsTeacher(), InSchool()] + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() + def get_queryset(self): + queryset = super().get_queryset() + if self.action == "bulk" and self.request.method in ["PATCH", "DELETE"]: + queryset = queryset.filter( + new_student__isnull=False, + new_student__class_field__isnull=False, + ) + return queryset + + def perform_bulk_destroy(self, queryset): + queryset.update(first_name="", is_active=False) + @action( detail=True, methods=["get", "patch"], @@ -89,7 +102,12 @@ def reset_password( return Response() - @action(detail=False, methods=["post"], permission_classes=[AllowAny]) + @action( + detail=False, + methods=["post"], + url_path="request-password-reset", + permission_classes=[AllowAny], + ) def request_password_reset(self, request: Request): """ Generates a reset password URL to be emailed to the user if the