From 8adbcfdcad01d468b4b4a83aa17f6316e47acbe4 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Thu, 7 Mar 2024 19:41:02 +0100 Subject: [PATCH 01/12] feat: IsStudent permission class and ability to join courses (#60) --- backend/api/permissions/__init__.py | 0 backend/api/permissions/role_permissions.py | 9 +++++ backend/api/views/course_view.py | 38 ++++++++++++++++----- backend/authentication/models.py | 6 ---- 4 files changed, 39 insertions(+), 14 deletions(-) create mode 100644 backend/api/permissions/__init__.py create mode 100644 backend/api/permissions/role_permissions.py diff --git a/backend/api/permissions/__init__.py b/backend/api/permissions/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/backend/api/permissions/role_permissions.py b/backend/api/permissions/role_permissions.py new file mode 100644 index 00000000..dcd85686 --- /dev/null +++ b/backend/api/permissions/role_permissions.py @@ -0,0 +1,9 @@ +from rest_framework.permissions import IsAuthenticated +from api.models.student import Student + +class IsStudent(IsAuthenticated): + def has_permission(self, request, view): + """Returns true if the request contains a user, + with said user being a student""" + return super().has_permission(request, view) and \ + Student.objects.filter(id=request.user.id).exists() \ No newline at end of file diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 54b1fcf2..05614ced 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,12 +1,16 @@ +from django.utils.translation import gettext from rest_framework import viewsets, status from rest_framework.decorators import action from rest_framework.response import Response -from ..models.course import Course -from ..serializers.course_serializer import CourseSerializer -from ..serializers.teacher_serializer import TeacherSerializer -from ..serializers.assistant_serializer import AssistantSerializer -from ..serializers.student_serializer import StudentSerializer -from ..serializers.project_serializer import ProjectSerializer +from rest_framework.exceptions import NotFound +from api.models.student import Student +from api.models.course import Course +from api.permissions.role_permissions import IsStudent +from api.serializers.course_serializer import CourseSerializer +from api.serializers.teacher_serializer import TeacherSerializer +from api.serializers.assistant_serializer import AssistantSerializer +from api.serializers.student_serializer import StudentSerializer +from api.serializers.project_serializer import ProjectSerializer class CourseViewSet(viewsets.ModelViewSet): @@ -89,6 +93,24 @@ def projects(self, request, pk=None): except Course.DoesNotExist: # Invalid course ID - return Response( - status=status.HTTP_404_NOT_FOUND, data={"message": "Course not found"} + raise NotFound(gettext("courses.errors.not_found")) + + @action(detail=True, methods=['get'], permission_classes=[IsStudent]) + def join(self, request, pk=None): + try: + # Add the course to the student's enrollment list. + Student.objects.get(id=request.user.id).courses.add( + Course.objects.get(id=pk) ) + + return Response({ + "message": gettext("courses.messages.successful_join") + }) + + except Course.DoesNotExist: + # Invalid course ID + raise NotFound(gettext("courses.errors.not_found")) + except Student.DoesNotExist: + # Invalid student user, this should not happen + # since the IsStudent permission class already checks this. + raise NotFound(gettext("students.errors.not_found")) \ No newline at end of file diff --git a/backend/authentication/models.py b/backend/authentication/models.py index 8a8787f4..dcf28b4d 100644 --- a/backend/authentication/models.py +++ b/backend/authentication/models.py @@ -35,12 +35,6 @@ class User(AbstractBaseUser): USERNAME_FIELD = "username" EMAIL_FIELD = "email" - def has_role(self, model: Type[Self]): - """Simple generic implementation of roles. - This function looks if there exists a model (inheriting from User) with the same ID. - """ - model.objects.exists(self.id) - @staticmethod def get_dummy_admin(): return User( From 350ec00cf8157992da9990097f3b0781cd50bd79 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 13:52:37 +0100 Subject: [PATCH 02/12] feat: (wip) add assistants, course permissiosn --- backend/api/permissions/course_permissions.py | 34 ++++++++++ backend/api/permissions/role_permissions.py | 18 +++++- backend/api/views/course_view.py | 62 +++++++++---------- 3 files changed, 82 insertions(+), 32 deletions(-) create mode 100644 backend/api/permissions/course_permissions.py diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py new file mode 100644 index 00000000..afdfa71b --- /dev/null +++ b/backend/api/permissions/course_permissions.py @@ -0,0 +1,34 @@ +from rest_framework.permissions import BasePermission, SAFE_METHODS +from rest_framework.request import Request +from rest_framework.viewsets import ViewSet +from authentication.models import User +from api.models.teacher import Teacher +from api.models.assistant import Assistant +from api.models.course import Course + + +class CoursePermission(BasePermission): + def has_permission(self, request: Request, view: ViewSet) -> bool: + """Check if user has permission to view a general course endpoint.""" + user: User = request.user + + if request.method in SAFE_METHODS: + # Logged-in users can fetch course lists. + return request.user.is_authenticated + + # We only allow teachers to create new courses. + return user.teacher.exists() + + def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: + """Check if user has permission to view a detailed course endpoint""" + user: User = request.user + + if request.method in SAFE_METHODS: + # Logged-in users can fetch course details. + return request.user.is_authenticated + + # We only allow teachers and assistants to modify specified courses. + role: Teacher|Assistant = user.teacher or user.assistant + + return role is not None and \ + role.courses.filter(id=course.id).exists() \ No newline at end of file diff --git a/backend/api/permissions/role_permissions.py b/backend/api/permissions/role_permissions.py index dcd85686..2a023a9d 100644 --- a/backend/api/permissions/role_permissions.py +++ b/backend/api/permissions/role_permissions.py @@ -1,9 +1,25 @@ from rest_framework.permissions import IsAuthenticated from api.models.student import Student +from api.models.assistant import Assistant +from api.models.teacher import Teacher class IsStudent(IsAuthenticated): def has_permission(self, request, view): """Returns true if the request contains a user, with said user being a student""" return super().has_permission(request, view) and \ - Student.objects.filter(id=request.user.id).exists() \ No newline at end of file + Student.objects.filter(id=request.user.id).exists() + +class IsTeacher(IsAuthenticated): + def has_permission(self, request, view): + """Returns true if the request contains a user, + with said user being a student""" + return super().has_permission(request, view) and \ + Teacher.objects.filter(id=request.user.id).exists() + +class IsAssistant(IsAuthenticated): + def has_permission(self, request, view): + """Returns true if the request contains a user, + with said user being a student""" + return super().has_permission(request, view) and \ + Assistant.objects.filter(id=request.user.id).exists() \ No newline at end of file diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 05614ced..5d80662d 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,10 +1,12 @@ from django.utils.translation import gettext from rest_framework import viewsets, status +from rest_framework.permissions import IsAdminUser from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.exceptions import NotFound from api.models.student import Student from api.models.course import Course +from api.permissions.course_permissions import CoursePermission from api.permissions.role_permissions import IsStudent from api.serializers.course_serializer import CourseSerializer from api.serializers.teacher_serializer import TeacherSerializer @@ -16,50 +18,48 @@ class CourseViewSet(viewsets.ModelViewSet): queryset = Course.objects.all() serializer_class = CourseSerializer + permission_classes = [IsAdminUser | CoursePermission] @action(detail=True, methods=["get"]) - def teachers(self, request, pk=None): + def teachers(self, request, **_): """Returns a list of teachers for the given course""" + # This automatically fetches the course from the URL. + # It automatically gives back a 404 HTTP response in case of not found. + course = self.get_object() + teachers = course.teachers.all() - try: - queryset = Course.objects.get(id=pk) - teachers = queryset.teachers.all() + # Serialize the teacher objects + serializer = TeacherSerializer( + teachers, many=True, context={"request": request} + ) - # Serialize the teacher objects - serializer = TeacherSerializer( - teachers, many=True, context={"request": request} - ) - return Response(serializer.data) + return Response(serializer.data) - except Course.DoesNotExist: - # Invalid course ID - return Response( - status=status.HTTP_404_NOT_FOUND, data={"message": "Course not found"} - ) - - @action(detail=True, methods=["get"]) - def assistants(self, request, pk=None): + @action(detail=True, methods=["get", "post"]) + def assistants(self, request, **_): """Returns a list of assistants for the given course""" + # This automatically fetches the course from the URL. + # It automatically gives back a 404 HTTP response in case of not found. + course = self.get_object() + assistants = course.assistants.all() - try: - queryset = Course.objects.get(id=pk) - assistants = queryset.assistants.all() - + if request.method == "GET": # Serialize the assistant objects serializer = AssistantSerializer( assistants, many=True, context={"request": request} ) + return Response(serializer.data) - except Course.DoesNotExist: - # Invalid course ID - return Response( - status=status.HTTP_404_NOT_FOUND, data={"message": "Course not found"} - ) + # Add a new assistant to the course, assistant ID in request.get("assistant_id") + + + @action(detail=True, methods=["get"]) def students(self, request, pk=None): """Returns a list of students for the given course""" + course = self.get_object() try: queryset = Course.objects.get(id=pk) @@ -73,9 +73,7 @@ def students(self, request, pk=None): except Course.DoesNotExist: # Invalid course ID - return Response( - status=status.HTTP_404_NOT_FOUND, data={"message": "Course not found"} - ) + raise NotFound(gettext("courses.errors.not_found")) @action(detail=True, methods=["get"]) def projects(self, request, pk=None): @@ -95,8 +93,10 @@ def projects(self, request, pk=None): # Invalid course ID raise NotFound(gettext("courses.errors.not_found")) - @action(detail=True, methods=['get'], permission_classes=[IsStudent]) + @action(detail=True, methods=["post"], permission_classes=[IsStudent]) def join(self, request, pk=None): + """Enrolls the authenticated student in the project""" + try: # Add the course to the student's enrollment list. Student.objects.get(id=request.user.id).courses.add( @@ -111,6 +111,6 @@ def join(self, request, pk=None): # Invalid course ID raise NotFound(gettext("courses.errors.not_found")) except Student.DoesNotExist: - # Invalid student user, this should not happen + # Invalid student user, this should not happen, # since the IsStudent permission class already checks this. raise NotFound(gettext("students.errors.not_found")) \ No newline at end of file From 009895eb25914a4e4675d79906445cf238d652c1 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 14:45:06 +0100 Subject: [PATCH 03/12] feat: add assistants, course permissions --- backend/api/permissions/course_permissions.py | 15 ++- backend/api/signals.py | 6 +- backend/api/views/course_view.py | 108 +++++++++--------- backend/authentication/models.py | 1 - 4 files changed, 71 insertions(+), 59 deletions(-) diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index afdfa71b..e2f7d987 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -8,6 +8,7 @@ class CoursePermission(BasePermission): + """Permission class used as default policy for course endpoints.""" def has_permission(self, request: Request, view: ViewSet) -> bool: """Check if user has permission to view a general course endpoint.""" user: User = request.user @@ -31,4 +32,16 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) role: Teacher|Assistant = user.teacher or user.assistant return role is not None and \ - role.courses.filter(id=course.id).exists() \ No newline at end of file + role.courses.filter(id=course.id).exists() + +class CourseTeacherPermission(CoursePermission): + """Permission class for teacher-only course endpoints.""" + def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: + user: User = request.user + + if request.method in SAFE_METHODS: + # Logged-in users can still fetch course details. + return request.user.is_authenticated + + return user.teacher.exists() and \ + user.teacher.courses.filter(id=course.id).exists() \ No newline at end of file diff --git a/backend/api/signals.py b/backend/api/signals.py index 85f94211..6395ea75 100644 --- a/backend/api/signals.py +++ b/backend/api/signals.py @@ -2,9 +2,9 @@ from api.models.student import Student -def user_creation(user: User, attributes: dict, **kwargs): +def user_creation(user: User, attributes: dict, **_): """Upon user creation, auto-populate additional properties""" - student_id = attributes.get("ugentStudentID") + student_id: str = attributes.get("ugentStudentID") - if student_id: + if student_id is not None: Student(user_ptr=user, student_id=student_id).save_base(raw=True) diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 5d80662d..bf4320d2 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,12 +1,13 @@ from django.utils.translation import gettext -from rest_framework import viewsets, status +from rest_framework import viewsets +from rest_framework.exceptions import NotFound from rest_framework.permissions import IsAdminUser from rest_framework.decorators import action from rest_framework.response import Response -from rest_framework.exceptions import NotFound -from api.models.student import Student +from rest_framework.request import Request from api.models.course import Course -from api.permissions.course_permissions import CoursePermission +from api.models.assistant import Assistant +from api.permissions.course_permissions import CoursePermission, CourseTeacherPermission from api.permissions.role_permissions import IsStudent from api.serializers.course_serializer import CourseSerializer from api.serializers.teacher_serializer import TeacherSerializer @@ -35,82 +36,81 @@ def teachers(self, request, **_): return Response(serializer.data) - @action(detail=True, methods=["get", "post"]) - def assistants(self, request, **_): - """Returns a list of assistants for the given course""" + @action(detail=True, methods=["get", "post", "delete"], permission_classes=[IsAdminUser | CourseTeacherPermission]) + def assistants(self, request: Request, **_) -> Response: + """Action for managing assistants associated to a course""" # This automatically fetches the course from the URL. # It automatically gives back a 404 HTTP response in case of not found. course = self.get_object() - assistants = course.assistants.all() if request.method == "GET": - # Serialize the assistant objects + # Return assistants of a course. + assistants = course.assistants.all() + serializer = AssistantSerializer( assistants, many=True, context={"request": request} ) return Response(serializer.data) - # Add a new assistant to the course, assistant ID in request.get("assistant_id") + try: + assistant = Assistant.objects.get( + id=request.query_params.get("id") + ) + + if request.method == "POST": + # Add a new assistant to the course. + course.assistants.add(assistant) + return Response({ + "message": gettext("courses.success.assistants.add") + }) + elif request.method == "DELETE": + # Remove an assistant from the course. + course.assistants.remove(assistant) + return Response({ + "message": gettext("courses.success.assistants.remove") + }) + except Assistant.DoesNotExist: + # Not found + raise NotFound(gettext("assistants.error.404")) @action(detail=True, methods=["get"]) - def students(self, request, pk=None): + def students(self, request, **_): """Returns a list of students for the given course""" course = self.get_object() + students = course.students.all() - try: - queryset = Course.objects.get(id=pk) - students = queryset.students.all() - - # Serialize the student objects - serializer = StudentSerializer( - students, many=True, context={"request": request} - ) - return Response(serializer.data) + # Serialize the student objects + serializer = StudentSerializer( + students, many=True, context={"request": request} + ) - except Course.DoesNotExist: - # Invalid course ID - raise NotFound(gettext("courses.errors.not_found")) + return Response(serializer.data) @action(detail=True, methods=["get"]) - def projects(self, request, pk=None): + def projects(self, request, **_): """Returns a list of projects for the given course""" + course = self.get_object() + projects = course.projects.all() - try: - queryset = Course.objects.get(id=pk) - projects = queryset.projects.all() - - # Serialize the project objects - serializer = ProjectSerializer( - projects, many=True, context={"request": request} - ) - return Response(serializer.data) + # Serialize the project objects + serializer = ProjectSerializer( + projects, many=True, context={"request": request} + ) - except Course.DoesNotExist: - # Invalid course ID - raise NotFound(gettext("courses.errors.not_found")) + return Response(serializer.data) @action(detail=True, methods=["post"], permission_classes=[IsStudent]) - def join(self, request, pk=None): + def join(self, request, **_): """Enrolls the authenticated student in the project""" + # Add the course to the student's enrollment list. + self.get_object().students.add( + request.user.student + ) - try: - # Add the course to the student's enrollment list. - Student.objects.get(id=request.user.id).courses.add( - Course.objects.get(id=pk) - ) - - return Response({ - "message": gettext("courses.messages.successful_join") - }) - - except Course.DoesNotExist: - # Invalid course ID - raise NotFound(gettext("courses.errors.not_found")) - except Student.DoesNotExist: - # Invalid student user, this should not happen, - # since the IsStudent permission class already checks this. - raise NotFound(gettext("students.errors.not_found")) \ No newline at end of file + return Response({ + "message": gettext("courses.success.join") + }) \ No newline at end of file diff --git a/backend/authentication/models.py b/backend/authentication/models.py index dcf28b4d..066d6fbb 100644 --- a/backend/authentication/models.py +++ b/backend/authentication/models.py @@ -1,5 +1,4 @@ from datetime import MINYEAR -from typing import Self, Type from django.db import models from django.db.models import CharField, EmailField, IntegerField, DateTimeField, BooleanField, Model from django.contrib.auth.models import AbstractBaseUser, AbstractUser, PermissionsMixin From 155683d3ccc74dffc88f1a658e169b4a20a72104 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 14:46:29 +0100 Subject: [PATCH 04/12] chore: linting --- backend/api/permissions/course_permissions.py | 5 +++-- backend/api/permissions/role_permissions.py | 5 ++++- backend/api/views/course_view.py | 3 +-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index e2f7d987..46ee6f50 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -29,11 +29,12 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) return request.user.is_authenticated # We only allow teachers and assistants to modify specified courses. - role: Teacher|Assistant = user.teacher or user.assistant + role: Teacher | Assistant = user.teacher or user.assistant return role is not None and \ role.courses.filter(id=course.id).exists() + class CourseTeacherPermission(CoursePermission): """Permission class for teacher-only course endpoints.""" def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: @@ -44,4 +45,4 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) return request.user.is_authenticated return user.teacher.exists() and \ - user.teacher.courses.filter(id=course.id).exists() \ No newline at end of file + user.teacher.courses.filter(id=course.id).exists() diff --git a/backend/api/permissions/role_permissions.py b/backend/api/permissions/role_permissions.py index 2a023a9d..94ace65c 100644 --- a/backend/api/permissions/role_permissions.py +++ b/backend/api/permissions/role_permissions.py @@ -3,6 +3,7 @@ from api.models.assistant import Assistant from api.models.teacher import Teacher + class IsStudent(IsAuthenticated): def has_permission(self, request, view): """Returns true if the request contains a user, @@ -10,6 +11,7 @@ def has_permission(self, request, view): return super().has_permission(request, view) and \ Student.objects.filter(id=request.user.id).exists() + class IsTeacher(IsAuthenticated): def has_permission(self, request, view): """Returns true if the request contains a user, @@ -17,9 +19,10 @@ def has_permission(self, request, view): return super().has_permission(request, view) and \ Teacher.objects.filter(id=request.user.id).exists() + class IsAssistant(IsAuthenticated): def has_permission(self, request, view): """Returns true if the request contains a user, with said user being a student""" return super().has_permission(request, view) and \ - Assistant.objects.filter(id=request.user.id).exists() \ No newline at end of file + Assistant.objects.filter(id=request.user.id).exists() diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index bf4320d2..c7123fa1 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -76,7 +76,6 @@ def assistants(self, request: Request, **_) -> Response: # Not found raise NotFound(gettext("assistants.error.404")) - @action(detail=True, methods=["get"]) def students(self, request, **_): """Returns a list of students for the given course""" @@ -113,4 +112,4 @@ def join(self, request, **_): return Response({ "message": gettext("courses.success.join") - }) \ No newline at end of file + }) From 171f12d546b65bbff402172a22a0d7c73eba059a Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 15:51:11 +0100 Subject: [PATCH 05/12] feat: course cloning --- backend/api/models/course.py | 10 ++ backend/api/permissions/course_permissions.py | 13 ++- backend/api/views/course_view.py | 108 ++++++++++++------ 3 files changed, 89 insertions(+), 42 deletions(-) diff --git a/backend/api/models/course.py b/backend/api/models/course.py index f4ec41f2..57ff10ff 100644 --- a/backend/api/models/course.py +++ b/backend/api/models/course.py @@ -1,3 +1,4 @@ +from typing import Self from django.db import models @@ -30,6 +31,15 @@ def __str__(self) -> str: """The string representation of the course.""" return str(self.name) + def clone(self, year=None) -> Self: + # To-do: add more control over the cloning process. + return Course( + name=self.name, + description=self.description, + academic_startyear=year or self.academic_startyear + 1, + parent_course=self + ) + @property def academic_year(self) -> str: """The academic year of the course.""" diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index 46ee6f50..cabf379f 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -18,7 +18,7 @@ def has_permission(self, request: Request, view: ViewSet) -> bool: return request.user.is_authenticated # We only allow teachers to create new courses. - return user.teacher.exists() + return hasattr(user, "teacher") and user.teacher.exists() def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: """Check if user has permission to view a detailed course endpoint""" @@ -26,12 +26,13 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) if request.method in SAFE_METHODS: # Logged-in users can fetch course details. - return request.user.is_authenticated + return user.is_authenticated # We only allow teachers and assistants to modify specified courses. - role: Teacher | Assistant = user.teacher or user.assistant + role: Teacher | Assistant = hasattr(user, "teacher") and user.teacher or \ + hasattr(user, "assistant") and user.assistant - return role is not None and \ + return role and \ role.courses.filter(id=course.id).exists() @@ -42,7 +43,7 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) if request.method in SAFE_METHODS: # Logged-in users can still fetch course details. - return request.user.is_authenticated + return user.is_authenticated - return user.teacher.exists() and \ + return hasattr(user, "teacher") and \ user.teacher.courses.filter(id=course.id).exists() diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index c7123fa1..4414ffa1 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,7 +1,7 @@ from django.utils.translation import gettext from rest_framework import viewsets from rest_framework.exceptions import NotFound -from rest_framework.permissions import IsAdminUser +from rest_framework.permissions import IsAdminUser, IsAuthenticated from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.request import Request @@ -17,65 +17,80 @@ class CourseViewSet(viewsets.ModelViewSet): + """Actions for general course logic""" queryset = Course.objects.all() serializer_class = CourseSerializer permission_classes = [IsAdminUser | CoursePermission] - @action(detail=True, methods=["get"]) - def teachers(self, request, **_): - """Returns a list of teachers for the given course""" - # This automatically fetches the course from the URL. - # It automatically gives back a 404 HTTP response in case of not found. + @action(detail=True, permission_classes=[IsAdminUser | CourseTeacherPermission]) + def assistants(self, request: Request, **_): + """Returns a list of assistants for the given course""" course = self.get_object() - teachers = course.teachers.all() + assistants = course.assistants.all() - # Serialize the teacher objects - serializer = TeacherSerializer( - teachers, many=True, context={"request": request} + # Serialize assistants + serializer = AssistantSerializer( + assistants, many=True, context={"request": request} ) return Response(serializer.data) - @action(detail=True, methods=["get", "post", "delete"], permission_classes=[IsAdminUser | CourseTeacherPermission]) - def assistants(self, request: Request, **_) -> Response: - """Action for managing assistants associated to a course""" - # This automatically fetches the course from the URL. - # It automatically gives back a 404 HTTP response in case of not found. + @assistants.mapping.post + @assistants.mapping.put + def _add_assistant(self, request: Request, **_): + """Add an assistant to the course""" course = self.get_object() - if request.method == "GET": - # Return assistants of a course. - assistants = course.assistants.all() - - serializer = AssistantSerializer( - assistants, many=True, context={"request": request} + try: + # Add assistant to course + assistant = Assistant.objects.get( + id=request.data.get("id") ) - return Response(serializer.data) + course.assistants.add(assistant) + + return Response({ + "message": gettext("courses.success.assistants.add") + }) + except Assistant.DoesNotExist: + # Not found + raise NotFound(gettext("assistants.error.404")) + + @assistants.mapping.delete + def _remove_assistant(self, request: Request, **_): + """Remove an assistant from the course""" + course = self.get_object() try: + # Add assistant to course assistant = Assistant.objects.get( - id=request.query_params.get("id") + id=request.data.get("id") ) - if request.method == "POST": - # Add a new assistant to the course. - course.assistants.add(assistant) + course.assistants.remove(assistant) - return Response({ - "message": gettext("courses.success.assistants.add") - }) - elif request.method == "DELETE": - # Remove an assistant from the course. - course.assistants.remove(assistant) - - return Response({ - "message": gettext("courses.success.assistants.remove") - }) + return Response({ + "message": gettext("courses.success.assistants.delete") + }) except Assistant.DoesNotExist: # Not found raise NotFound(gettext("assistants.error.404")) + @action(detail=True, methods=["get"]) + def teachers(self, request, **_): + """Returns a list of teachers for the given course""" + # This automatically fetches the course from the URL. + # It automatically gives back a 404 HTTP response in case of not found. + course = self.get_object() + teachers = course.teachers.all() + + # Serialize the teacher objects + serializer = TeacherSerializer( + teachers, many=True, context={"request": request} + ) + + return Response(serializer.data) + @action(detail=True, methods=["get"]) def students(self, request, **_): """Returns a list of students for the given course""" @@ -113,3 +128,24 @@ def join(self, request, **_): return Response({ "message": gettext("courses.success.join") }) + + @action(detail=True, methods=["post"], permission_classes=[IsAdminUser | CourseTeacherPermission]) + def clone(self, request: Request, **__): + """Copy the course to a new course with the same fields""" + course: Course = self.get_object() + + try: + course_serializer = CourseSerializer( + course.child_course, context={"request": request} + ) + except Course.DoesNotExist: + course_serializer = CourseSerializer( + course.clone( + year=request.data.get("academic_startyear") + ), + context={"request": request} + ) + + course_serializer.save() + + return Response(course_serializer.data) \ No newline at end of file From b2aba5eb6dfe6c170fc867b9746b2f26ccbd1ddc Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 15:53:20 +0100 Subject: [PATCH 06/12] chore: linting --- backend/api/views/course_view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 4414ffa1..aa2d9219 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -148,4 +148,4 @@ def clone(self, request: Request, **__): course_serializer.save() - return Response(course_serializer.data) \ No newline at end of file + return Response(course_serializer.data) From fe9c1706f943f6b15b0a95fa2cf0d19e80f09133 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 20:48:22 +0100 Subject: [PATCH 07/12] chore: finished course logic with sensible permissions --- backend/api/models/course.py | 13 ++- backend/api/permissions/course_permissions.py | 60 +++++++--- backend/api/permissions/role_permissions.py | 24 ++-- backend/api/views/course_view.py | 107 +++++++++++------- 4 files changed, 134 insertions(+), 70 deletions(-) diff --git a/backend/api/models/course.py b/backend/api/models/course.py index 57ff10ff..6c657c2c 100644 --- a/backend/api/models/course.py +++ b/backend/api/models/course.py @@ -31,15 +31,20 @@ def __str__(self) -> str: """The string representation of the course.""" return str(self.name) - def clone(self, year=None) -> Self: - # To-do: add more control over the cloning process. - return Course( + def clone(self, clone_assistants=True) -> Self: + """Clone the course to the next academic start year""" + course = Course( name=self.name, description=self.description, - academic_startyear=year or self.academic_startyear + 1, + academic_startyear=self.academic_startyear + 1, parent_course=self ) + if clone_assistants: + course.assistants.add(self.assistants) + + return course + @property def academic_year(self) -> str: """The academic year of the course.""" diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index cabf379f..fd0c0f9c 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -2,8 +2,7 @@ from rest_framework.request import Request from rest_framework.viewsets import ViewSet from authentication.models import User -from api.models.teacher import Teacher -from api.models.assistant import Assistant +from api.permissions.role_permissions import is_student, is_assistant, is_teacher from api.models.course import Course @@ -13,37 +12,64 @@ def has_permission(self, request: Request, view: ViewSet) -> bool: """Check if user has permission to view a general course endpoint.""" user: User = request.user + # Logged-in users can fetch course information. if request.method in SAFE_METHODS: - # Logged-in users can fetch course lists. return request.user.is_authenticated - # We only allow teachers to create new courses. - return hasattr(user, "teacher") and user.teacher.exists() + # Only teachers can create courses. + return is_teacher(user) def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: """Check if user has permission to view a detailed course endpoint""" user: User = request.user + # Logged-in users can fetch course details. if request.method in SAFE_METHODS: - # Logged-in users can fetch course details. return user.is_authenticated - # We only allow teachers and assistants to modify specified courses. - role: Teacher | Assistant = hasattr(user, "teacher") and user.teacher or \ - hasattr(user, "assistant") and user.assistant + # We only allow teachers and assistants to modify their own courses. + return is_teacher(user) and user.teacher.courses.contains(course) or \ + is_assistant(user) and user.assistant.courses.contains(course) - return role and \ - role.courses.filter(id=course.id).exists() - -class CourseTeacherPermission(CoursePermission): - """Permission class for teacher-only course endpoints.""" +class CourseAssistantPermission(CoursePermission): + """Permission class for assistant related endpoints.""" def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: user: User = request.user + # Logged-in users can fetch course assistants. + if request.method in SAFE_METHODS: + return user.is_authenticated + + # Only teachers can modify assistants of their own courses. + return is_teacher(user) and user.teacher.courses.contains(course) + + +class CourseStudentPermission(CoursePermission): + """Permission class for student related endpoints.""" + def has_object_permission(self, request: Request, view: ViewSet, course: Course): + user: User = request.user + + # Logged-in users can fetch course students. + if request.method in SAFE_METHODS: + return user.is_authenticated + + # Only students can add or remove themselves from a course. + if is_student(user) and request.data.get("id") == user.id: + return True + + # Teachers and assistants can add and remove any student. + return super().has_object_permission(request, view, course) + + +class CourseProjectPermission(CoursePermission): + """Permission class for project related endpoints.""" + def has_object_permission(self, request: Request, view: ViewSet, course: Course): + user: User = request.user + + # Logged-in users can fetch course projects. if request.method in SAFE_METHODS: - # Logged-in users can still fetch course details. return user.is_authenticated - return hasattr(user, "teacher") and \ - user.teacher.courses.filter(id=course.id).exists() + # Teachers and assistants can modify projects. + return super().has_object_permission(request, view, course) diff --git a/backend/api/permissions/role_permissions.py b/backend/api/permissions/role_permissions.py index 94ace65c..b196747c 100644 --- a/backend/api/permissions/role_permissions.py +++ b/backend/api/permissions/role_permissions.py @@ -1,28 +1,36 @@ from rest_framework.permissions import IsAuthenticated +from rest_framework.request import Request +from authentication.models import User from api.models.student import Student from api.models.assistant import Assistant from api.models.teacher import Teacher +def is_student(user: User): + return Student.objects.filter(id=user.id).exists() + +def is_assistant(user: User): + return Assistant.objects.filter(id=user.id).exists() + +def is_teacher(user: User): + return Teacher.objects.filter(id=user.id).exists() + class IsStudent(IsAuthenticated): - def has_permission(self, request, view): + def has_permission(self, request: Request, view): """Returns true if the request contains a user, with said user being a student""" - return super().has_permission(request, view) and \ - Student.objects.filter(id=request.user.id).exists() + return super().has_permission(request, view) and is_student(request.user) class IsTeacher(IsAuthenticated): - def has_permission(self, request, view): + def has_permission(self, request: Request, view): """Returns true if the request contains a user, with said user being a student""" - return super().has_permission(request, view) and \ - Teacher.objects.filter(id=request.user.id).exists() + return super().has_permission(request, view) and is_teacher(request.user) class IsAssistant(IsAuthenticated): def has_permission(self, request, view): """Returns true if the request contains a user, with said user being a student""" - return super().has_permission(request, view) and \ - Assistant.objects.filter(id=request.user.id).exists() + return super().has_permission(request, view) and is_assistant(request.user) diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index aa2d9219..13dfbae4 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,14 +1,15 @@ from django.utils.translation import gettext from rest_framework import viewsets from rest_framework.exceptions import NotFound -from rest_framework.permissions import IsAdminUser, IsAuthenticated +from rest_framework.permissions import IsAdminUser from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.request import Request from api.models.course import Course from api.models.assistant import Assistant -from api.permissions.course_permissions import CoursePermission, CourseTeacherPermission -from api.permissions.role_permissions import IsStudent +from api.models.student import Student +from api.permissions.course_permissions import CoursePermission, CourseAssistantPermission, CourseStudentPermission +from api.permissions.role_permissions import IsTeacher from api.serializers.course_serializer import CourseSerializer from api.serializers.teacher_serializer import TeacherSerializer from api.serializers.assistant_serializer import AssistantSerializer @@ -22,7 +23,7 @@ class CourseViewSet(viewsets.ModelViewSet): serializer_class = CourseSerializer permission_classes = [IsAdminUser | CoursePermission] - @action(detail=True, permission_classes=[IsAdminUser | CourseTeacherPermission]) + @action(detail=True, permission_classes=[IsAdminUser | CourseAssistantPermission]) def assistants(self, request: Request, **_): """Returns a list of assistants for the given course""" course = self.get_object() @@ -76,22 +77,8 @@ def _remove_assistant(self, request: Request, **_): # Not found raise NotFound(gettext("assistants.error.404")) - @action(detail=True, methods=["get"]) - def teachers(self, request, **_): - """Returns a list of teachers for the given course""" - # This automatically fetches the course from the URL. - # It automatically gives back a 404 HTTP response in case of not found. - course = self.get_object() - teachers = course.teachers.all() - # Serialize the teacher objects - serializer = TeacherSerializer( - teachers, many=True, context={"request": request} - ) - - return Response(serializer.data) - - @action(detail=True, methods=["get"]) + @action(detail=True, methods=["get"], permission_classes=[IsAdminUser | CourseStudentPermission]) def students(self, request, **_): """Returns a list of students for the given course""" course = self.get_object() @@ -104,6 +91,58 @@ def students(self, request, **_): return Response(serializer.data) + @students.mapping.post + @students.mapping.put + def _add_student(self, request: Request, **_): + """Add a student to the course""" + course = self.get_object() + + try: + # Add student to course + student = Student.objects.get( + id=request.data.get("id") + ) + + course.students.add(student) + + return Response({ + "message": gettext("courses.success.students.add") + }) + except Student.DoesNotExist: + raise NotFound(gettext("students.error.404")) + + @students.mapping.delete + def _remove_student(self, request: Request, **_): + """Remove a student from the course""" + course = self.get_object() + + try: + # Add student to course + student = Student.objects.get( + id=request.data.get("id") + ) + + course.students.remove(student) + + return Response({ + "message": gettext("courses.success.students.remove") + }) + except Student.DoesNotExist: + raise NotFound(gettext("students.error.404")) + + @action(detail=True, methods=["get"]) + def teachers(self, request, **_): + """Returns a list of teachers for the given course""" + course = self.get_object() + teachers = course.teachers.all() + + # Serialize the teacher objects + serializer = TeacherSerializer( + teachers, many=True, context={"request": request} + ) + + return Response(serializer.data) + @action(detail=True, methods=["get"]) def projects(self, request, **_): """Returns a list of projects for the given course""" @@ -117,35 +156,21 @@ def projects(self, request, **_): return Response(serializer.data) - @action(detail=True, methods=["post"], permission_classes=[IsStudent]) - def join(self, request, **_): - """Enrolls the authenticated student in the project""" - # Add the course to the student's enrollment list. - self.get_object().students.add( - request.user.student - ) - - return Response({ - "message": gettext("courses.success.join") - }) - - @action(detail=True, methods=["post"], permission_classes=[IsAdminUser | CourseTeacherPermission]) + @action(detail=True, methods=["post"], permission_classes=[IsAdminUser | IsTeacher]) def clone(self, request: Request, **__): """Copy the course to a new course with the same fields""" course: Course = self.get_object() try: - course_serializer = CourseSerializer( - course.child_course, context={"request": request} - ) + course = course.child_course except Course.DoesNotExist: - course_serializer = CourseSerializer( - course.clone( - year=request.data.get("academic_startyear") - ), - context={"request": request} + course = course.clone( + clone_assistants=request.data.get("clone_assistants") ) - course_serializer.save() + course.save() + + # Return serialized cloned course + course_serializer = CourseSerializer(course, context={"request": request}) return Response(course_serializer.data) From bdb9d9e272a328037207493947b5a5f71f8e4438 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 20:52:00 +0100 Subject: [PATCH 08/12] chore: linting --- backend/api/permissions/course_permissions.py | 4 ++++ backend/api/permissions/role_permissions.py | 3 +++ backend/api/views/course_view.py | 1 - 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index fd0c0f9c..3c2543b5 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -8,6 +8,7 @@ class CoursePermission(BasePermission): """Permission class used as default policy for course endpoints.""" + def has_permission(self, request: Request, view: ViewSet) -> bool: """Check if user has permission to view a general course endpoint.""" user: User = request.user @@ -34,6 +35,7 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) class CourseAssistantPermission(CoursePermission): """Permission class for assistant related endpoints.""" + def has_object_permission(self, request: Request, view: ViewSet, course: Course) -> bool: user: User = request.user @@ -47,6 +49,7 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) class CourseStudentPermission(CoursePermission): """Permission class for student related endpoints.""" + def has_object_permission(self, request: Request, view: ViewSet, course: Course): user: User = request.user @@ -64,6 +67,7 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) class CourseProjectPermission(CoursePermission): """Permission class for project related endpoints.""" + def has_object_permission(self, request: Request, view: ViewSet, course: Course): user: User = request.user diff --git a/backend/api/permissions/role_permissions.py b/backend/api/permissions/role_permissions.py index b196747c..1e9c275c 100644 --- a/backend/api/permissions/role_permissions.py +++ b/backend/api/permissions/role_permissions.py @@ -9,12 +9,15 @@ def is_student(user: User): return Student.objects.filter(id=user.id).exists() + def is_assistant(user: User): return Assistant.objects.filter(id=user.id).exists() + def is_teacher(user: User): return Teacher.objects.filter(id=user.id).exists() + class IsStudent(IsAuthenticated): def has_permission(self, request: Request, view): """Returns true if the request contains a user, diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 13dfbae4..416ab1f5 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -77,7 +77,6 @@ def _remove_assistant(self, request: Request, **_): # Not found raise NotFound(gettext("assistants.error.404")) - @action(detail=True, methods=["get"], permission_classes=[IsAdminUser | CourseStudentPermission]) def students(self, request, **_): """Returns a list of students for the given course""" From 4082b4a57d40f0c7ce611e799864ecc3a6259587 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Fri, 8 Mar 2024 21:51:47 +0100 Subject: [PATCH 09/12] chore: serializers for validation --- backend/api/models/course.py | 5 + .../api/serializers/assistant_serializer.py | 6 + backend/api/serializers/course_serializer.py | 49 +++++++- backend/api/serializers/student_serializer.py | 6 + backend/api/views/course_view.py | 107 +++++++++--------- 5 files changed, 120 insertions(+), 53 deletions(-) diff --git a/backend/api/models/course.py b/backend/api/models/course.py index 6c657c2c..02f346e8 100644 --- a/backend/api/models/course.py +++ b/backend/api/models/course.py @@ -1,3 +1,4 @@ +from datetime import datetime from typing import Self from django.db import models @@ -31,6 +32,10 @@ def __str__(self) -> str: """The string representation of the course.""" return str(self.name) + def is_past(self) -> bool: + """Returns whether the course is from a past academic year""" + return datetime(self.academic_startyear + 1, 10, 1) < datetime.now() + def clone(self, clone_assistants=True) -> Self: """Clone the course to the next academic start year""" course = Course( diff --git a/backend/api/serializers/assistant_serializer.py b/backend/api/serializers/assistant_serializer.py index 16e26206..639b184b 100644 --- a/backend/api/serializers/assistant_serializer.py +++ b/backend/api/serializers/assistant_serializer.py @@ -24,3 +24,9 @@ class Meta: "create_time", "courses", ] + + +class AssistantIDSerializer(serializers.Serializer): + assistant_id = serializers.PrimaryKeyRelatedField( + queryset=Assistant.objects.all() + ) diff --git a/backend/api/serializers/course_serializer.py b/backend/api/serializers/course_serializer.py index 4d6edede..de7d0b70 100644 --- a/backend/api/serializers/course_serializer.py +++ b/backend/api/serializers/course_serializer.py @@ -1,5 +1,8 @@ +from django.utils.translation import gettext from rest_framework import serializers -from ..models.course import Course +from rest_framework.exceptions import ValidationError +from api.serializers.student_serializer import StudentIDSerializer +from api.models.course import Course class CourseSerializer(serializers.ModelSerializer): @@ -40,3 +43,47 @@ class Meta: "students", "projects", ] + + +class CourseIDSerializer(serializers.Serializer): + student_id = serializers.PrimaryKeyRelatedField( + queryset=Course.objects.all() + ) + + +class StudentJoinSerializer(StudentIDSerializer): + def validate(self, data): + # The validator needs the course context. + if "course" not in self.context: + raise ValidationError(gettext("courses.error.context")) + + course: Course = self.context["course"] + + # Check if the student isn't already enrolled. + if course.students.contains(data["student_id"]): + raise ValidationError(gettext("courses.error.students.already_present")) + + # Check if the course is not from a past academic year. + if course.is_past(): + raise ValidationError(gettext("courses.error.students.past_course")) + + return data + + +class StudentLeaveSerializer(StudentIDSerializer): + def validate(self, data): + # The validator needs the course context. + if "course" not in self.context: + raise ValidationError(gettext("courses.error.context")) + + course: Course = self.context["course"] + + # Check if the student isn't already enrolled. + if not course.students.contains(data["student_id"]): + raise ValidationError(gettext("courses.error.students.not_present")) + + # Check if the course is not from a past academic year. + if course.is_past(): + raise ValidationError(gettext("courses.error.students.past_course")) + + return data diff --git a/backend/api/serializers/student_serializer.py b/backend/api/serializers/student_serializer.py index 9cd1f245..2c46593f 100644 --- a/backend/api/serializers/student_serializer.py +++ b/backend/api/serializers/student_serializer.py @@ -20,3 +20,9 @@ class StudentSerializer(serializers.ModelSerializer): class Meta: model = Student fields = '__all__' + + +class StudentIDSerializer(serializers.Serializer): + student_id = serializers.PrimaryKeyRelatedField( + queryset=Student.objects.all() + ) diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 416ab1f5..be9bd152 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,18 +1,19 @@ from django.utils.translation import gettext from rest_framework import viewsets -from rest_framework.exceptions import NotFound from rest_framework.permissions import IsAdminUser from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.request import Request from api.models.course import Course -from api.models.assistant import Assistant -from api.models.student import Student -from api.permissions.course_permissions import CoursePermission, CourseAssistantPermission, CourseStudentPermission +from api.permissions.course_permissions import ( + CoursePermission, + CourseAssistantPermission, + CourseStudentPermission +) from api.permissions.role_permissions import IsTeacher -from api.serializers.course_serializer import CourseSerializer +from api.serializers.course_serializer import CourseSerializer, StudentJoinSerializer, StudentLeaveSerializer from api.serializers.teacher_serializer import TeacherSerializer -from api.serializers.assistant_serializer import AssistantSerializer +from api.serializers.assistant_serializer import AssistantSerializer, AssistantIDSerializer from api.serializers.student_serializer import StudentSerializer from api.serializers.project_serializer import ProjectSerializer @@ -42,40 +43,38 @@ def _add_assistant(self, request: Request, **_): """Add an assistant to the course""" course = self.get_object() - try: - # Add assistant to course - assistant = Assistant.objects.get( - id=request.data.get("id") - ) + # Add assistant to course + serializer = AssistantIDSerializer( + data=request.data + ) - course.assistants.add(assistant) + if serializer.is_valid(raise_exception=True): + course.assistants.add( + serializer.validated_data["assistant_id"] + ) - return Response({ - "message": gettext("courses.success.assistants.add") - }) - except Assistant.DoesNotExist: - # Not found - raise NotFound(gettext("assistants.error.404")) + return Response({ + "message": gettext("courses.success.assistants.add") + }) @assistants.mapping.delete def _remove_assistant(self, request: Request, **_): """Remove an assistant from the course""" course = self.get_object() - try: - # Add assistant to course - assistant = Assistant.objects.get( - id=request.data.get("id") - ) + # Remove assistant from course + serializer = AssistantIDSerializer( + data=request.data + ) - course.assistants.remove(assistant) + if serializer.is_valid(raise_exception=True): + course.assistants.remove( + serializer.validated_data["assistant_id"] + ) - return Response({ - "message": gettext("courses.success.assistants.delete") - }) - except Assistant.DoesNotExist: - # Not found - raise NotFound(gettext("assistants.error.404")) + return Response({ + "message": gettext("courses.success.assistants.add") + }) @action(detail=True, methods=["get"], permission_classes=[IsAdminUser | CourseStudentPermission]) def students(self, request, **_): @@ -94,40 +93,42 @@ def students(self, request, **_): @students.mapping.put def _add_student(self, request: Request, **_): """Add a student to the course""" + # Get the course course = self.get_object() - try: - # Add student to course - student = Student.objects.get( - id=request.data.get("id") - ) + # Add student to course + serializer = StudentJoinSerializer(data=request.data, context={ + "course": course + }) - course.students.add(student) + if serializer.is_valid(raise_exception=True): + course.students.add( + serializer.validated_data["student_id"] + ) - return Response({ - "message": gettext("courses.success.students.add") - }) - except Student.DoesNotExist: - raise NotFound(gettext("students.error.404")) + return Response({ + "message": gettext("courses.success.students.add") + }) @students.mapping.delete def _remove_student(self, request: Request, **_): """Remove a student from the course""" + # Get the course course = self.get_object() - try: - # Add student to course - student = Student.objects.get( - id=request.data.get("id") - ) + # Add student to course + serializer = StudentLeaveSerializer(data=request.data, context={ + "course": course + }) - course.students.remove(student) + if serializer.is_valid(raise_exception=True): + course.students.remove( + serializer.validated_data["student_id"] + ) - return Response({ - "message": gettext("courses.success.students.remove") - }) - except Student.DoesNotExist: - raise NotFound(gettext("students.error.404")) + return Response({ + "message": gettext("courses.success.students.add") + }) @action(detail=True, methods=["get"]) def teachers(self, request, **_): @@ -161,8 +162,10 @@ def clone(self, request: Request, **__): course: Course = self.get_object() try: + # We should return the already cloned course, if present course = course.child_course except Course.DoesNotExist: + # Else, we clone the course course = course.clone( clone_assistants=request.data.get("clone_assistants") ) From af385e67e1875f49535188179b60298922577658 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Sat, 9 Mar 2024 12:52:59 +0100 Subject: [PATCH 10/12] fix: course permissions --- backend/api/permissions/course_permissions.py | 2 +- backend/api/views/course_view.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index 3c2543b5..7f88d7a9 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -58,7 +58,7 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) return user.is_authenticated # Only students can add or remove themselves from a course. - if is_student(user) and request.data.get("id") == user.id: + if is_student(user) and request.data.get("student_id") == user.id: return True # Teachers and assistants can add and remove any student. diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index be9bd152..84a50ae6 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,7 +1,7 @@ from django.utils.translation import gettext from rest_framework import viewsets from rest_framework.permissions import IsAdminUser -from rest_framework.decorators import action +from rest_framework.decorators import action, permission_classes from rest_framework.response import Response from rest_framework.request import Request from api.models.course import Course @@ -164,6 +164,7 @@ def clone(self, request: Request, **__): try: # We should return the already cloned course, if present course = course.child_course + except Course.DoesNotExist: # Else, we clone the course course = course.clone( From 3c5969a432119ebe402cf7c982a7a2379bde1559 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Sat, 9 Mar 2024 12:54:34 +0100 Subject: [PATCH 11/12] chore: linting --- backend/api/views/course_view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/api/views/course_view.py b/backend/api/views/course_view.py index 84a50ae6..cea2b180 100644 --- a/backend/api/views/course_view.py +++ b/backend/api/views/course_view.py @@ -1,7 +1,7 @@ from django.utils.translation import gettext from rest_framework import viewsets from rest_framework.permissions import IsAdminUser -from rest_framework.decorators import action, permission_classes +from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.request import Request from api.models.course import Course From bd769fbfef95d98706bb902f1574be582526bcf6 Mon Sep 17 00:00:00 2001 From: EwoutV Date: Sat, 9 Mar 2024 13:05:39 +0100 Subject: [PATCH 12/12] fix: permission classes --- backend/api/permissions/course_permissions.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/backend/api/permissions/course_permissions.py b/backend/api/permissions/course_permissions.py index 7f88d7a9..5bce23e7 100644 --- a/backend/api/permissions/course_permissions.py +++ b/backend/api/permissions/course_permissions.py @@ -49,6 +49,8 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) class CourseStudentPermission(CoursePermission): """Permission class for student related endpoints.""" + def has_permission(self, request: Request, view: ViewSet) -> bool: + return request.user and request.user.is_authenticated def has_object_permission(self, request: Request, view: ViewSet, course: Course): user: User = request.user @@ -67,6 +69,8 @@ def has_object_permission(self, request: Request, view: ViewSet, course: Course) class CourseProjectPermission(CoursePermission): """Permission class for project related endpoints.""" + def has_permission(self, request: Request, view: ViewSet) -> bool: + return request.user and request.user.is_authenticated def has_object_permission(self, request: Request, view: ViewSet, course: Course): user: User = request.user