From 87c6bf5a1f2c5a53c3edbe7951cd89e51284a590 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 2 Feb 2024 16:56:57 +0000 Subject: [PATCH 1/6] fix: permission checking --- codeforlife/permissions/allow_none.py | 3 +++ .../permissions/is_cron_request_from_google.py | 3 +++ codeforlife/tests/model_view_set.py | 17 +++++++++++++++++ codeforlife/user/permissions/in_class.py | 6 ++++++ codeforlife/user/permissions/in_school.py | 6 ++++++ codeforlife/user/permissions/is_independent.py | 3 +++ codeforlife/user/permissions/is_student.py | 6 ++++++ codeforlife/user/permissions/is_teacher.py | 7 +++++++ codeforlife/views/model.py | 4 ++++ 9 files changed, 55 insertions(+) diff --git a/codeforlife/permissions/allow_none.py b/codeforlife/permissions/allow_none.py index 6bc21734..ef2cbb83 100644 --- a/codeforlife/permissions/allow_none.py +++ b/codeforlife/permissions/allow_none.py @@ -14,5 +14,8 @@ class AllowNone(BasePermission): https://www.django-rest-framework.org/api-guide/permissions/#allowany """ + def __eq__(self, other): + return isinstance(other, self.__class__) + def has_permission(self, request, view): return False diff --git a/codeforlife/permissions/is_cron_request_from_google.py b/codeforlife/permissions/is_cron_request_from_google.py index b8f49f7c..c6cdf254 100644 --- a/codeforlife/permissions/is_cron_request_from_google.py +++ b/codeforlife/permissions/is_cron_request_from_google.py @@ -14,6 +14,9 @@ class IsCronRequestFromGoogle(BasePermission): https://cloud.google.com/appengine/docs/flexible/scheduling-jobs-with-cron-yaml#securing_urls_for_cron """ + def __eq__(self, other): + return isinstance(other, self.__class__) + def has_permission(self, request, view): return ( settings.DEBUG diff --git a/codeforlife/tests/model_view_set.py b/codeforlife/tests/model_view_set.py index eee9dffa..36db8841 100644 --- a/codeforlife/tests/model_view_set.py +++ b/codeforlife/tests/model_view_set.py @@ -17,6 +17,7 @@ from django.utils.http import urlencode from pyotp import TOTP from rest_framework import status +from rest_framework.permissions import BasePermission from rest_framework.response import Response from rest_framework.test import APIClient, APIRequestFactory, APITestCase @@ -689,6 +690,22 @@ def setUpClass(cls): return super().setUpClass() + def assert_get_permissions( + self, + permissions: t.List[BasePermission], + *args, + **kwargs, + ): + """Assert that we get the expected permissions. + + Args: + permissions: The expected permissions. + """ + + model_view_set = self.model_view_set_class(*args, **kwargs) + actual_permissions = model_view_set.get_permissions() + self.assertListEqual(permissions, actual_permissions) + def get_other_user( self, user: User, diff --git a/codeforlife/user/permissions/in_class.py b/codeforlife/user/permissions/in_class.py index c030e6d0..2c5ceac6 100644 --- a/codeforlife/user/permissions/in_class.py +++ b/codeforlife/user/permissions/in_class.py @@ -26,6 +26,12 @@ def __init__(self, class_id: t.Optional[str] = None): super().__init__() self.class_id = class_id + def __eq__(self, other): + return ( + isinstance(other, self.__class__) + and self.class_id == other.class_id + ) + def has_permission(self, request: Request, view: APIView): user = request.user if super().has_permission(request, view) and isinstance(user, User): diff --git a/codeforlife/user/permissions/in_school.py b/codeforlife/user/permissions/in_school.py index 1b866d21..2fde40c2 100644 --- a/codeforlife/user/permissions/in_school.py +++ b/codeforlife/user/permissions/in_school.py @@ -26,6 +26,12 @@ def __init__(self, school_id: t.Optional[int] = None): super().__init__() self.school_id = school_id + def __eq__(self, other): + return ( + isinstance(other, self.__class__) + and self.school_id == other.school_id + ) + def has_permission(self, request: Request, view: APIView): def in_school(school_id: int): return self.school_id is None or self.school_id == school_id diff --git a/codeforlife/user/permissions/is_independent.py b/codeforlife/user/permissions/is_independent.py index 5d0f5a8e..0ad94565 100644 --- a/codeforlife/user/permissions/is_independent.py +++ b/codeforlife/user/permissions/is_independent.py @@ -13,6 +13,9 @@ class IsIndependent(IsAuthenticated): """Request's user must be independent.""" + def __eq__(self, other): + return isinstance(other, self.__class__) + def has_permission(self, request: Request, view: APIView): user = request.user return ( diff --git a/codeforlife/user/permissions/is_student.py b/codeforlife/user/permissions/is_student.py index a4a43e9e..2f0a1f9b 100644 --- a/codeforlife/user/permissions/is_student.py +++ b/codeforlife/user/permissions/is_student.py @@ -26,6 +26,12 @@ def __init__(self, student_id: t.Optional[int] = None): super().__init__() self.student_id = student_id + def __eq__(self, other): + return ( + isinstance(other, self.__class__) + and self.student_id == other.student_id + ) + def has_permission(self, request: Request, view: APIView): user = request.user return ( diff --git a/codeforlife/user/permissions/is_teacher.py b/codeforlife/user/permissions/is_teacher.py index 255d9c6d..c7a8ec65 100644 --- a/codeforlife/user/permissions/is_teacher.py +++ b/codeforlife/user/permissions/is_teacher.py @@ -34,6 +34,13 @@ def __init__( self.teacher_id = teacher_id self.is_admin = is_admin + def __eq__(self, other): + return ( + isinstance(other, self.__class__) + and self.teacher_id == other.teacher_id + and self.is_admin == other.is_admin + ) + def has_permission(self, request: Request, view: APIView): user = request.user return ( diff --git a/codeforlife/views/model.py b/codeforlife/views/model.py index 80560a0a..5e568e9f 100644 --- a/codeforlife/views/model.py +++ b/codeforlife/views/model.py @@ -9,6 +9,7 @@ from django.db.models.query import QuerySet from rest_framework import status from rest_framework.decorators import action +from rest_framework.permissions import BasePermission from rest_framework.request import Request from rest_framework.response import Response from rest_framework.serializers import ListSerializer @@ -49,6 +50,9 @@ def get_model_class(cls) -> t.Type[AnyModel]: 0 ] + def get_permissions(self): + return t.cast(t.List[BasePermission], super().get_permissions()) + def get_serializer(self, *args, **kwargs): serializer = super().get_serializer(*args, **kwargs) From 0970b8983a6ce5b9c9f8cd3f5bdc414c4e313cbf Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 2 Feb 2024 17:08:49 +0000 Subject: [PATCH 2/6] fix: assert get query set --- codeforlife/tests/model_view_set.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/codeforlife/tests/model_view_set.py b/codeforlife/tests/model_view_set.py index 36db8841..e856b7c7 100644 --- a/codeforlife/tests/model_view_set.py +++ b/codeforlife/tests/model_view_set.py @@ -696,7 +696,7 @@ def assert_get_permissions( *args, **kwargs, ): - """Assert that we get the expected permissions. + """Assert that the expected permissions are returned. Args: permissions: The expected permissions. @@ -706,6 +706,25 @@ def assert_get_permissions( actual_permissions = model_view_set.get_permissions() self.assertListEqual(permissions, actual_permissions) + def assert_get_queryset( + self, + values: t.Collection[AnyModel], + *args, + ordered: bool = True, + **kwargs, + ): + """Assert that the expected queryset is returned. + + Args: + values: The values we expect the queryset to contain. + ordered: Whether the queryset provides an implicit ordering. + """ + + model_view_set = self.model_view_set_class(*args, **kwargs) + queryset = model_view_set.get_queryset() + # pylint: disable-next=no-member + self.assertQuerySetEqual(queryset, values, ordered=ordered) + def get_other_user( self, user: User, From 6ddd7df6ddc6b36bf399516ae4c289430864ef59 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 2 Feb 2024 17:13:31 +0000 Subject: [PATCH 3/6] add TODOs --- codeforlife/user/tests/views/test_klass.py | 1 + codeforlife/user/tests/views/test_school.py | 2 ++ codeforlife/user/tests/views/test_user.py | 2 ++ 3 files changed, 5 insertions(+) diff --git a/codeforlife/user/tests/views/test_klass.py b/codeforlife/user/tests/views/test_klass.py index eda41b04..86452206 100644 --- a/codeforlife/user/tests/views/test_klass.py +++ b/codeforlife/user/tests/views/test_klass.py @@ -58,3 +58,4 @@ def test_retrieve__student__same_school__in_class(self): self.client.retrieve(user.student.class_field) # TODO: other retrieve and list tests + # TODO: replace above tests with get_queryset() tests diff --git a/codeforlife/user/tests/views/test_school.py b/codeforlife/user/tests/views/test_school.py index d5b80db4..222c3726 100644 --- a/codeforlife/user/tests/views/test_school.py +++ b/codeforlife/user/tests/views/test_school.py @@ -193,3 +193,5 @@ def test_list__student(self): user = self._login_student() self.client.list([user.student.class_field.teacher.school]) + + # TODO: replace above tests with get_queryset() tests diff --git a/codeforlife/user/tests/views/test_user.py b/codeforlife/user/tests/views/test_user.py index 793dfc63..cacb475a 100644 --- a/codeforlife/user/tests/views/test_user.py +++ b/codeforlife/user/tests/views/test_user.py @@ -519,3 +519,5 @@ def test_all__only_http_get(self): assert [name.lower() for name in UserViewSet.http_method_names] == [ "get" ] + + # TODO: replace above tests with get_queryset() tests From 804728fe2d3ae3a5790877c2b954ed9c17adf468 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Fri, 2 Feb 2024 18:09:34 +0000 Subject: [PATCH 4/6] fix: permission operators --- codeforlife/permissions/__init__.py | 1 + codeforlife/permissions/operators.py | 50 ++++++++++++++++++++++++++++ codeforlife/tests/model_view_set.py | 4 +-- codeforlife/views/model.py | 4 +-- 4 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 codeforlife/permissions/operators.py diff --git a/codeforlife/permissions/__init__.py b/codeforlife/permissions/__init__.py index ddb7bf5b..d3bc6f9a 100644 --- a/codeforlife/permissions/__init__.py +++ b/codeforlife/permissions/__init__.py @@ -7,3 +7,4 @@ from .allow_none import AllowNone from .is_cron_request_from_google import IsCronRequestFromGoogle +from .operators import AND, NOT, OR, Permission diff --git a/codeforlife/permissions/operators.py b/codeforlife/permissions/operators.py new file mode 100644 index 00000000..81176d06 --- /dev/null +++ b/codeforlife/permissions/operators.py @@ -0,0 +1,50 @@ +""" +© Ocado Group +Created on 02/02/2024 at 17:52:37(+00:00). + +Extends the permission operands provided by Django REST framework. +""" + +import typing as t + +from rest_framework.permissions import AND as _AND +from rest_framework.permissions import NOT as _NOT +from rest_framework.permissions import OR as _OR +from rest_framework.permissions import BasePermission + + +# pylint: disable-next=missing-class-docstring +class AND(_AND): + op1: BasePermission + op2: BasePermission + + def __eq__(self, other): + return ( + isinstance(other, self.__class__) + and self.op1 == other.op1 + and self.op2 == other.op2 + ) + + +# pylint: disable-next=missing-class-docstring +class NOT(_NOT): + op1: BasePermission + + def __eq__(self, other): + return isinstance(other, self.__class__) and self.op1 == other.op1 + + +# pylint: disable-next=missing-class-docstring +class OR(_OR): + op1: BasePermission + op2: BasePermission + + def __eq__(self, other): + return ( + isinstance(other, self.__class__) + and self.op1 == other.op1 + and self.op2 == other.op2 + ) + + +Permission = t.Union[BasePermission, AND, NOT, OR] diff --git a/codeforlife/tests/model_view_set.py b/codeforlife/tests/model_view_set.py index e856b7c7..cb9290c2 100644 --- a/codeforlife/tests/model_view_set.py +++ b/codeforlife/tests/model_view_set.py @@ -17,10 +17,10 @@ from django.utils.http import urlencode from pyotp import TOTP from rest_framework import status -from rest_framework.permissions import BasePermission from rest_framework.response import Response from rest_framework.test import APIClient, APIRequestFactory, APITestCase +from ..permissions import Permission from ..serializers import ModelSerializer from ..user.models import AuthFactor, User from ..views import ModelViewSet @@ -692,7 +692,7 @@ def setUpClass(cls): def assert_get_permissions( self, - permissions: t.List[BasePermission], + permissions: t.List[Permission], *args, **kwargs, ): diff --git a/codeforlife/views/model.py b/codeforlife/views/model.py index 5e568e9f..c0173844 100644 --- a/codeforlife/views/model.py +++ b/codeforlife/views/model.py @@ -9,12 +9,12 @@ from django.db.models.query import QuerySet from rest_framework import status from rest_framework.decorators import action -from rest_framework.permissions import BasePermission from rest_framework.request import Request from rest_framework.response import Response from rest_framework.serializers import ListSerializer from rest_framework.viewsets import ModelViewSet as DrfModelViewSet +from ..permissions import Permission from ..serializers import ModelListSerializer, ModelSerializer AnyModel = t.TypeVar("AnyModel", bound=Model) @@ -51,7 +51,7 @@ def get_model_class(cls) -> t.Type[AnyModel]: ] def get_permissions(self): - return t.cast(t.List[BasePermission], super().get_permissions()) + return t.cast(t.List[Permission], super().get_permissions()) def get_serializer(self, *args, **kwargs): serializer = super().get_serializer(*args, **kwargs) From faf764f72be7f3d5f127df970b961a88b28b0ab6 Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 5 Feb 2024 11:30:17 +0000 Subject: [PATCH 5/6] fix: proxy models for users and teachers, reverse_action helper --- codeforlife/tests/model_view_set.py | 153 ++++++++++++------ ...hoolteacheruser_studentuser_teacheruser.py | 93 +++++++++++ codeforlife/user/models/__init__.py | 11 +- codeforlife/user/models/teacher.py | 75 ++------- codeforlife/user/models/user.py | 108 ++++++++----- codeforlife/user/tests/views/test_user.py | 28 ++-- 6 files changed, 302 insertions(+), 166 deletions(-) create mode 100644 codeforlife/user/migrations/0002_nonschoolteacher_nonschoolteacheruser_schoolteacher_schoolteacheruser_studentuser_teacheruser.py diff --git a/codeforlife/tests/model_view_set.py b/codeforlife/tests/model_view_set.py index cb9290c2..76b74949 100644 --- a/codeforlife/tests/model_view_set.py +++ b/codeforlife/tests/model_view_set.py @@ -12,7 +12,7 @@ from django.db.models import Model from django.db.models.query import QuerySet -from django.urls import reverse as _reverse +from django.urls import reverse from django.utils import timezone from django.utils.http import urlencode from pyotp import TOTP @@ -22,7 +22,14 @@ from ..permissions import Permission from ..serializers import ModelSerializer -from ..user.models import AuthFactor, User +from ..user.models import ( + AuthFactor, + NonSchoolTeacherUser, + SchoolTeacherUser, + StudentUser, + TeacherUser, + User, +) from ..views import ModelViewSet AnyModel = t.TypeVar("AnyModel", bound=Model) @@ -181,39 +188,6 @@ def parse_data(data): "Data does not equal serialized model.", ) - def reverse( - self, - action: str, - model: t.Optional[AnyModel] = None, - **kwargs, - ): - """Get the reverse URL for the model view set's action. - - Args: - action: The name of the action. - model: The model to look up. - - Returns: - The reversed URL. - """ - - reverse_kwargs = kwargs.pop("kwargs", {}) - if model is not None: - reverse_kwargs[self._model_view_set_class.lookup_field] = getattr( - model, - self._model_view_set_class.lookup_field, - ) - - return _reverse( - viewname=kwargs.pop( - "viewname", - # pylint: disable-next=no-member - f"{self._test_case.basename}-{action}", - ), - kwargs=reverse_kwargs, - **kwargs, - ) - # pylint: disable-next=too-many-arguments def generic( self, @@ -278,7 +252,8 @@ def create( # pylint: enable=line-too-long response: Response = self.post( - self.reverse("list"), + # pylint: disable-next=no-member + self._test_case.reverse_action("list"), data=json.dumps(data), content_type="application/json", status_code_assertion=status_code_assertion, @@ -316,7 +291,8 @@ def bulk_create( # pylint: enable=line-too-long response: Response = self.post( - self.reverse("bulk"), + # pylint: disable-next=no-member + self._test_case.reverse_action("bulk"), data=json.dumps(data), content_type="application/json", status_code_assertion=status_code_assertion, @@ -356,7 +332,8 @@ def retrieve( # pylint: enable=line-too-long response: Response = self.get( - self.reverse("detail", model), + # pylint: disable-next=no-member + self._test_case.reverse_action("detail", model), status_code_assertion=status_code_assertion, **kwargs, ) @@ -403,7 +380,11 @@ def list( ).exists(), "List must exclude some models for a valid test." response: Response = self.get( - f"{self.reverse('list')}?{urlencode(filters or {})}", + ( + # pylint: disable-next=no-member + self._test_case.reverse_action("list") + + f"?{urlencode(filters or {})}" + ), status_code_assertion=status_code_assertion, **kwargs, ) @@ -445,7 +426,8 @@ def partial_update( # pylint: enable=line-too-long response: Response = self.patch( - self.reverse("detail", model), + # pylint: disable-next=no-member + self._test_case.reverse_action("detail", model), data=json.dumps(data), content_type="application/json", status_code_assertion=status_code_assertion, @@ -490,7 +472,8 @@ def bulk_partial_update( # pylint: enable=line-too-long response: Response = self.patch( - self.reverse("bulk"), + # pylint: disable-next=no-member + self._test_case.reverse_action("bulk"), data=json.dumps(data), content_type="application/json", status_code_assertion=status_code_assertion, @@ -532,7 +515,8 @@ def destroy( """ response: Response = self.delete( - self.reverse("detail", model), + # pylint: disable-next=no-member + self._test_case.reverse_action("detail", model), status_code_assertion=status_code_assertion, **kwargs, ) @@ -569,7 +553,8 @@ def bulk_destroy( """ response: Response = self.delete( - self.reverse("bulk"), + # pylint: disable-next=no-member + self._test_case.reverse_action("bulk"), data=json.dumps(lookup_values), content_type="application/json", status_code_assertion=status_code_assertion, @@ -612,12 +597,16 @@ def login(self, **credentials): return user - def login_teacher(self, is_admin: t.Optional[bool] = None, **credentials): + def login_teacher( + self, + is_admin: t.Optional[bool] = None, + **credentials, + ) -> TeacherUser: # pylint: disable=line-too-long """Log in a user and assert they are a teacher. Args: - is_admin: Whether or not the teacher is an admin. Set none if a teacher can be either or. + is_admin: Whether or not the teacher is an admin. Set to None if the teacher can be either or. Returns: The teacher-user. @@ -626,12 +615,52 @@ def login_teacher(self, is_admin: t.Optional[bool] = None, **credentials): user = self.login(**credentials) assert user.teacher - assert user.teacher.school if is_admin is not None: assert is_admin == user.teacher.is_admin + return user - def login_student(self, **credentials): + def login_school_teacher( + self, + is_admin: t.Optional[bool] = None, + **credentials, + ): + # pylint: disable=line-too-long + """Log in a user and assert they are a school-teacher. + + Args: + is_admin: Whether or not the teacher is an admin. Set to None if the teacher can be either or. + + Returns: + The school-teacher-user. + """ + # pylint: enable=line-too-long + + user = self.login_teacher(is_admin, **credentials) + assert user.teacher.school + return t.cast(SchoolTeacherUser, user) + + def login_non_school_teacher( + self, + is_admin: t.Optional[bool] = None, + **credentials, + ): + # pylint: disable=line-too-long + """Log in a user and assert they are a non-school-teacher. + + Args: + is_admin: Whether or not the teacher is an admin. Set to None if the teacher can be either or. + + Returns: + The non-school-teacher-user. + """ + # pylint: enable=line-too-long + + user = self.login_teacher(is_admin, **credentials) + assert not user.teacher.school + return t.cast(NonSchoolTeacherUser, user) + + def login_student(self, **credentials) -> StudentUser: """Log in a user and assert they are a student. Returns: @@ -643,7 +672,8 @@ def login_student(self, **credentials): assert user.student.class_field.teacher.school return user - def login_indy(self, **credentials): + # TODO: set return type to IndependentUser when we use the new data models. + def login_indy(self, **credentials) -> StudentUser: """Log in an independent and assert they are a student. Returns: @@ -690,6 +720,33 @@ def setUpClass(cls): return super().setUpClass() + def reverse_action( + self, + name: str, + model: t.Optional[AnyModel] = None, + **kwargs, + ): + """Get the reverse URL for the model view set's action. + + Args: + name: The name of the action. + model: The model to look up. + + Returns: + The reversed URL for the model view set's action. + """ + + reverse_kwargs = kwargs.pop("kwargs", {}) + if model is not None: + lookup_field = self.model_view_set_class.lookup_field + reverse_kwargs[lookup_field] = getattr(model, lookup_field) + + return reverse( + viewname=kwargs.pop("viewname", f"{self.basename}-{name}"), + kwargs=reverse_kwargs, + **kwargs, + ) + def assert_get_permissions( self, permissions: t.List[Permission], diff --git a/codeforlife/user/migrations/0002_nonschoolteacher_nonschoolteacheruser_schoolteacher_schoolteacheruser_studentuser_teacheruser.py b/codeforlife/user/migrations/0002_nonschoolteacher_nonschoolteacheruser_schoolteacher_schoolteacheruser_studentuser_teacheruser.py new file mode 100644 index 00000000..561c58f3 --- /dev/null +++ b/codeforlife/user/migrations/0002_nonschoolteacher_nonschoolteacheruser_schoolteacher_schoolteacheruser_studentuser_teacheruser.py @@ -0,0 +1,93 @@ +# Generated by Django 3.2.20 on 2024-02-05 10:04 + +import django.contrib.auth.models +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('common', '0045_otp'), + ('user', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='NonSchoolTeacher', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('common.teacher',), + ), + migrations.CreateModel( + name='NonSchoolTeacherUser', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('user.user',), + managers=[ + ('objects', django.contrib.auth.models.UserManager()), + ], + ), + migrations.CreateModel( + name='SchoolTeacher', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('common.teacher',), + ), + migrations.CreateModel( + name='SchoolTeacherUser', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('user.user',), + managers=[ + ('objects', django.contrib.auth.models.UserManager()), + ], + ), + migrations.CreateModel( + name='StudentUser', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('user.user',), + managers=[ + ('objects', django.contrib.auth.models.UserManager()), + ], + ), + migrations.CreateModel( + name='TeacherUser', + fields=[ + ], + options={ + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('user.user',), + managers=[ + ('objects', django.contrib.auth.models.UserManager()), + ], + ), + ] diff --git a/codeforlife/user/models/__init__.py b/codeforlife/user/models/__init__.py index b61114af..fdf82727 100644 --- a/codeforlife/user/models/__init__.py +++ b/codeforlife/user/models/__init__.py @@ -8,5 +8,12 @@ from .session import Session from .session_auth_factor import SessionAuthFactor from .student import Student -from .teacher import Teacher -from .user import User, UserProfile # TODO: remove UserProfile +from .teacher import NonSchoolTeacher, SchoolTeacher, Teacher +from .user import ( # TODO: remove UserProfile + NonSchoolTeacherUser, + SchoolTeacherUser, + StudentUser, + TeacherUser, + User, + UserProfile, +) diff --git a/codeforlife/user/models/teacher.py b/codeforlife/user/models/teacher.py index d53ab4f1..4c8dea44 100644 --- a/codeforlife/user/models/teacher.py +++ b/codeforlife/user/models/teacher.py @@ -1,68 +1,27 @@ -# from django.db import models +""" +© Ocado Group +Created on 05/02/2024 at 09:49:56(+00:00). +""" -# from .user import User -# from .school import School - - -# class TeacherModelManager(models.Manager): -# def factory(self, first_name, last_name, email, password): -# user = User.objects.create_user( -# username=email, -# email=email, -# password=password, -# first_name=first_name, -# last_name=last_name, -# ) - -# return Teacher.objects.create(user=user) +from common.models import Teacher +from django_stubs_ext.db.models import TypedModelMeta -# # Filter out non active teachers by default -# def get_queryset(self): -# return super().get_queryset().filter(user__is_active=True) +from .school import School -# class Teacher(models.Model): -# user = models.OneToOneField( -# User, -# related_name="teacher", -# null=True, -# blank=True, -# on_delete=models.CASCADE, -# ) -# school = models.ForeignKey( -# School, -# related_name="school_teacher", -# null=True, -# blank=True, -# on_delete=models.SET_NULL, -# ) -# is_admin = models.BooleanField(default=False) -# blocked_time = models.DateTimeField(null=True, blank=True) -# invited_by = models.ForeignKey( -# "self", -# related_name="invited_teachers", -# null=True, -# blank=True, -# on_delete=models.SET_NULL, -# ) +class SchoolTeacher(Teacher): + """A teacher that is in a school.""" -# objects = TeacherModelManager() + school: School -# def teaches(self, userprofile): -# if hasattr(userprofile, "student"): -# student = userprofile.student -# return ( -# not student.is_independent() -# and student.class_field.teacher == self -# ) + class Meta(TypedModelMeta): + proxy = True -# def has_school(self): -# return self.school is not (None or "") -# def has_class(self): -# return self.class_teacher.exists() +class NonSchoolTeacher(Teacher): + """A teacher that is not in a school.""" -# def __str__(self): -# return f"{self.user.first_name} {self.user.last_name}" + school: None -from common.models import Teacher + class Meta(TypedModelMeta): + proxy = True diff --git a/codeforlife/user/models/user.py b/codeforlife/user/models/user.py index 79b24999..7df30615 100644 --- a/codeforlife/user/models/user.py +++ b/codeforlife/user/models/user.py @@ -1,64 +1,35 @@ -# from datetime import timedelta -# from enum import Enum - -# from django.contrib.auth.models import AbstractUser -# from django.contrib.auth.models import UserManager as AbstractUserManager -# from django.db import models -# from django.utils import timezone - - -# class UserManager(AbstractUserManager): -# def create_user(self, username, email=None, password=None, **extra_fields): -# return super().create_user(username, email, password, **extra_fields) - -# def create_superuser( -# self, username, email=None, password=None, **extra_fields -# ): -# return super().create_superuser( -# username, email, password, **extra_fields -# ) - - -# class User(AbstractUser): -# class Type(str, Enum): -# TEACHER = "teacher" -# DEP_STUDENT = "dependent-student" -# INDEP_STUDENT = "independent-student" - -# developer = models.BooleanField(default=False) -# is_verified = models.BooleanField(default=False) - -# objects: UserManager = UserManager() - -# def __str__(self): -# return self.get_full_name() - -# @property -# def joined_recently(self): -# return timezone.now() - timedelta(days=7) <= self.date_joined +""" +© Ocado Group +Created on 05/02/2024 at 09:50:04(+00:00). +""" import typing as t from common.models import UserProfile + +# pylint: disable-next=imported-auth-user from django.contrib.auth.models import User as _User from django.db.models.query import QuerySet from django.utils.translation import gettext_lazy as _ +from django_stubs_ext.db.models import TypedModelMeta from . import auth_factor, otp_bypass_token, session from .student import Student -from .teacher import Teacher +from .teacher import NonSchoolTeacher, SchoolTeacher, Teacher class User(_User): _password: t.Optional[str] - id: int - auth_factors: QuerySet["auth_factor.AuthFactor"] - otp_bypass_tokens: QuerySet["otp_bypass_token.OtpBypassToken"] - session: "session.Session" + id: int # type: ignore[assignment] + auth_factors: QuerySet["auth_factor.AuthFactor"] # type: ignore[assignment] + otp_bypass_tokens: QuerySet[ # type: ignore[assignment] + "otp_bypass_token.OtpBypassToken" + ] + session: "session.Session" # type: ignore[assignment] userprofile: UserProfile - class Meta: + class Meta(TypedModelMeta): proxy = True @property @@ -109,3 +80,52 @@ def is_verified(self): @property def aimmo_badges(self): return self.userprofile.aimmo_badges + + +class TeacherUser(User): + """A user that is a teacher.""" + + teacher: Teacher + student: None + + class Meta(TypedModelMeta): + proxy = True + + +class SchoolTeacherUser(User): + """A user that is a teacher in a school.""" + + teacher: SchoolTeacher + student: None + + class Meta(TypedModelMeta): + proxy = True + + +class NonSchoolTeacherUser(User): + """A user that is a teacher not in a school.""" + + teacher: NonSchoolTeacher + student: None + + class Meta(TypedModelMeta): + proxy = True + + +class StudentUser(User): + """A user that is a student.""" + + teacher: None + student: Student + + class Meta(TypedModelMeta): + proxy = True + + +# TODO: uncomment this when using new models. +# class IndependentUser(User): +# teacher: None +# student: None + +# class Meta(TypedModelMeta): +# proxy = True diff --git a/codeforlife/user/tests/views/test_user.py b/codeforlife/user/tests/views/test_user.py index cacb475a..50f56fcb 100644 --- a/codeforlife/user/tests/views/test_user.py +++ b/codeforlife/user/tests/views/test_user.py @@ -67,15 +67,15 @@ def setUp(self): new_user=user, ) - def _login_teacher(self): - return self.client.login_teacher( + def _login_school_teacher(self): + return self.client.login_school_teacher( email="maxplanck@codeforlife.com", password="Password1", is_admin=False, ) - def _login_admin_teacher(self): - return self.client.login_teacher( + def _login_admin_school_teacher(self): + return self.client.login_school_teacher( email="alberteinstein@codeforlife.com", password="Password1", is_admin=True, @@ -123,7 +123,7 @@ def test_retrieve__teacher__self(self): Teacher can retrieve their own user data. """ - user = self._login_teacher() + user = self._login_school_teacher() self.client.retrieve(user) @@ -150,7 +150,7 @@ def test_retrieve__teacher__teacher__same_school(self): Teacher can retrieve another teacher from the same school. """ - user = self._login_teacher() + user = self._login_school_teacher() other_user = self.get_another_school_user( user, @@ -168,7 +168,7 @@ def test_retrieve__teacher__student__same_school__same_class(self): Teacher can retrieve a student from the same school and class. """ - user = self._login_teacher() + user = self._login_school_teacher() other_user = self.get_another_school_user( user, @@ -189,7 +189,7 @@ def test_retrieve__teacher__student__same_school__not_same_class(self): class. """ - user = self._login_teacher() + user = self._login_school_teacher() other_user = self.get_another_school_user( user, @@ -208,7 +208,7 @@ def test_retrieve__admin_teacher__student__same_school__same_class(self): Admin teacher can retrieve a student from the same school and class. """ - user = self._login_admin_teacher() + user = self._login_admin_school_teacher() other_user = self.get_another_school_user( user, @@ -231,7 +231,7 @@ def test_retrieve__admin_teacher__student__same_school__not_same_class( different class. """ - user = self._login_admin_teacher() + user = self._login_admin_school_teacher() other_user = self.get_another_school_user( user, @@ -332,7 +332,7 @@ def test_retrieve__teacher__teacher__not_same_school(self): Teacher cannot retrieve another teacher from another school. """ - user = self._login_teacher() + user = self._login_school_teacher() other_user = self.get_another_school_user( user, @@ -350,7 +350,7 @@ def test_retrieve__teacher__student__not_same_school(self): Teacher cannot retrieve a student from another school. """ - user = self._login_teacher() + user = self._login_school_teacher() other_user = self.get_another_school_user( user, @@ -449,7 +449,7 @@ def test_list__teacher(self): Teacher can list all the users in the same class. """ - user = self._login_teacher() + user = self._login_school_teacher() self.client.list( User.objects.filter(new_teacher__school=user.teacher.school) @@ -464,7 +464,7 @@ def test_list__teacher__students_in_class(self): Teacher can list all the users in a class they own. """ - user = self._login_teacher() + user = self._login_school_teacher() klass = user.teacher.class_teacher.first() assert klass From 067ad5d5fe9de3edbe2749d1175ec5bd64f7e39f Mon Sep 17 00:00:00 2001 From: SKairinos Date: Mon, 5 Feb 2024 12:04:56 +0000 Subject: [PATCH 6/6] feedback --- codeforlife/tests/model_view_set.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/codeforlife/tests/model_view_set.py b/codeforlife/tests/model_view_set.py index 76b74949..fd1dfd3c 100644 --- a/codeforlife/tests/model_view_set.py +++ b/codeforlife/tests/model_view_set.py @@ -640,23 +640,14 @@ def login_school_teacher( assert user.teacher.school return t.cast(SchoolTeacherUser, user) - def login_non_school_teacher( - self, - is_admin: t.Optional[bool] = None, - **credentials, - ): - # pylint: disable=line-too-long + def login_non_school_teacher(self, **credentials): """Log in a user and assert they are a non-school-teacher. - Args: - is_admin: Whether or not the teacher is an admin. Set to None if the teacher can be either or. - Returns: The non-school-teacher-user. """ - # pylint: enable=line-too-long - user = self.login_teacher(is_admin, **credentials) + user = self.login_teacher(is_admin=None, **credentials) assert not user.teacher.school return t.cast(NonSchoolTeacherUser, user)