chore: went through some todos (#426)

* chore: went through some todos

* chore: allow docker image updates

backend/api/models/checks.py

@@ -14,8 +14,6 @@ class FileExtension(models.Model):
         unique=True
     )
 
-# TODO: Remove zip.* translations
-
 
 class StructureCheck(models.Model):
     """Model that represents a structure check for a project.
@@ -100,7 +98,6 @@ class ExtraCheck(models.Model):
     )
 
     # Maximum memory the container uses in MB
-    # TODO: Set max and min
     memory_limit = models.PositiveSmallIntegerField(
         default=128,
         blank=False,

backend/api/permissions/group_permissions.py

@@ -1,8 +1,10 @@
+from api.models.group import Group
 from api.permissions.role_permissions import (is_assistant, is_student,
                                               is_teacher)
 from authentication.models import User
 from rest_framework.permissions import SAFE_METHODS, BasePermission
 from rest_framework.request import Request
+from rest_framework.views import APIView
 from rest_framework.viewsets import ViewSet
 
 
@@ -59,6 +61,23 @@ def has_object_permission(self, request: Request, view: ViewSet, group) -> bool:
 class GroupSubmissionPermission(BasePermission):
     """Permission class for submission related group endpoints"""
 
+    def has_permission(self, request: Request, view: APIView) -> bool:
+        user: User = request.user
+        group_id = view.kwargs.get('pk')
+        group: Group | None = Group.objects.get(id=group_id) if group_id else None
+
+        if group is None:
+            return True
+
+        # Teachers and assistants of that course can view all submissions
+        if is_teacher(user):
+            return group.project.course.teachers.filter(id=user.teacher.id).exists()
+
+        if is_assistant(user):
+            return group.project.course.assistants.filter(id=user.assistant.id).exists()
+
+        return is_student(user) and group.students.filter(id=user.student.id).exists()
+
     def had_object_permission(self, request: Request, view: ViewSet, group) -> bool:
         user: User = request.user
         course = group.project.course

backend/api/permissions/project_permissions.py

@@ -13,11 +13,6 @@ def has_permission(self, request: Request, view: ViewSet) -> bool:
         """Check if user has permission to view a general project endpoint."""
         user: User = request.user
 
-        # TODO: Sure return True corresponds with the comments made above
-        # The general project endpoint that lists all projects is not accessible for any role.
-        if request.method in SAFE_METHODS:
-            return True
-
         # We only allow teachers and assistants to create new projects.
         return is_teacher(user) or is_assistant(user)
 

backend/api/signals.py

@@ -7,24 +7,26 @@
 from api.models.student import Student
 from api.models.submission import (ExtraCheckResult, StateEnum,
                                    StructureCheckResult, Submission)
-from api.tasks.docker_image import task_docker_image_build
+from api.tasks.docker_image import (task_docker_image_build,
+                                    task_docker_image_remove)
 from api.tasks.extra_check import task_extra_check_start
 from api.tasks.structure_check import task_structure_check_start
 from authentication.models import User
 from authentication.signals import user_created
-from django.db.models.signals import post_delete, post_save
+from django.db.models.signals import post_delete, post_save, pre_delete
 from django.dispatch import Signal, receiver
 
-# Signals
+# MARK: Signals
 
 
 run_docker_image_build = Signal()
+run_docker_image_remove = Signal()
 run_all_checks = Signal()
 run_structure_checks = Signal()
 run_extra_checks = Signal()
 
 
-# Receivers
+# MARK: Receivers
 
 
 @receiver(user_created)
@@ -44,6 +46,11 @@ def _run_docker_image_build(docker_image: DockerImage, **_):
     task_docker_image_build.apply_async((docker_image,))
 
 
+@receiver(run_docker_image_remove)
+def _run_docker_image_remove(docker_image: DockerImage, **_):
+    task_docker_image_remove.apply_async((docker_image,))
+
+
 @receiver(run_all_checks)
 def _run_all_checks(submission: Submission, **_):
     # Get all checks
@@ -75,7 +82,7 @@ def _run_extra_checks(submission: Submission, **_):
         task_extra_check_start.apply_async((True, extra_check_result,))
 
 
-# Hooks
+# MARK: Hooks
 
 
 @receiver(post_save, sender=StructureCheck)
@@ -108,19 +115,23 @@ def hook_extra_check(sender, instance: ExtraCheck, **kwargs):
 
 @receiver(post_save, sender=Submission)
 def hook_submission(sender, instance: Submission, created: bool, **kwargs):
-    # TODO: Maybe remove the raw check
     if created and not kwargs.get('raw', False):
         run_all_checks.send(sender=Submission, submission=instance)
         pass
 
 
 @receiver(post_save, sender=DockerImage)
 def hook_docker_image(sender, instance: DockerImage, created: bool, **kwargs):
-    # Run when it's created
     if created:
         run_docker_image_build.send(sender=DockerImage, docker_image=instance)
 
-# Helpers
+
+@receiver(pre_delete, sender=DockerImage)
+def hook_docker_image_delete(sender, instance: DockerImage, **kwargs):
+    run_docker_image_remove.send(sender=DockerImage, docker_image=instance)
+
+
+# MARK: Helpers
 
 
 # Get all structure checks and create a result for each one

backend/api/tasks/docker_image.py

@@ -6,7 +6,6 @@
 from ypovoli.settings import MEDIA_ROOT
 
 
-# TODO: Remove built image when it's deleted from the database
 @shared_task
 def task_docker_image_build(docker_image: DockerImage):
     # Set state
@@ -19,9 +18,18 @@ def task_docker_image_build(docker_image: DockerImage):
         client.images.build(path=MEDIA_ROOT, dockerfile=docker_image.file.path,
                             tag=get_docker_image_tag(docker_image), rm=True, quiet=True, forcerm=True)
         docker_image.state = StateEnum.READY
-    except (docker.errors.APIError, docker.errors.BuildError, docker.errors.APIError, TypeError):
+    except (docker.errors.APIError, docker.errors.BuildError, TypeError):
         docker_image.state = StateEnum.ERROR
         # TODO: Sent notification
 
     # Update the state
     docker_image.save()
+
+
+@shared_task
+def task_docker_image_remove(docker_image: DockerImage):
+    try:
+        client = docker.from_env()
+        client.images.remove(get_docker_image_tag(docker_image))
+    except docker.errors.APIError:
+        pass

backend/api/views/course_view.py

@@ -10,9 +10,9 @@
 from api.serializers.assistant_serializer import (AssistantIDSerializer,
                                                   AssistantSerializer)
 from api.serializers.course_serializer import (CourseCloneSerializer,
-                                               SaveInvitationLinkSerializer,
                                                CourseSerializer,
                                                CreateCourseSerializer,
+                                               SaveInvitationLinkSerializer,
                                                StudentJoinSerializer,
                                                StudentLeaveSerializer,
                                                TeacherJoinSerializer,
@@ -39,7 +39,6 @@ class CourseViewSet(viewsets.ModelViewSet):
     serializer_class = CourseSerializer
     permission_classes = [IsAdminUser | CoursePermission]
 
-    # TODO: Creating should return the info of the new object and not a message "created" (General TODO)
     def create(self, request: Request, *_):
         """Override the create method to add the teacher to the course"""
         serializer = CreateCourseSerializer(data=request.data, context={"request": request})

backend/api/views/docker_view.py

@@ -1,22 +1,19 @@
 from api.models.docker import DockerImage
 from api.permissions.docker_permissions import DockerPermission
 from api.serializers.docker_serializer import DockerImageSerializer
-from rest_framework.permissions import IsAdminUser
+from api.views.pagination.basic_pagination import BasicPagination
 from django.db.models import Q
 from django.db.models.manager import BaseManager
 from rest_framework.decorators import action
 from rest_framework.mixins import (CreateModelMixin, DestroyModelMixin,
                                    RetrieveModelMixin, UpdateModelMixin)
+from rest_framework.permissions import IsAdminUser
 from rest_framework.request import Request
 from rest_framework.response import Response
 from rest_framework.viewsets import GenericViewSet
 
-from api.views.pagination.basic_pagination import BasicPagination
-
-
-# TODO: Remove update abilities, maybe?
 
-class DockerImageViewSet(RetrieveModelMixin, CreateModelMixin, UpdateModelMixin, DestroyModelMixin, GenericViewSet):
+class DockerImageViewSet(RetrieveModelMixin, UpdateModelMixin, CreateModelMixin, DestroyModelMixin, GenericViewSet):
 
     queryset = DockerImage.objects.all()
     serializer_class = DockerImageSerializer
@@ -64,8 +61,6 @@ def patch_public(self, request: Request, **_) -> Response:
 
         return Response(serializer.data)
 
-    # TODO: Maybe not necessary
-    # https://www.django-rest-framework.org/api-guide/permissions/#overview-of-access-restriction-methods
     def list(self, request: Request) -> Response:
         images: BaseManager[DockerImage] = DockerImage.objects.all()
         if not request.user.is_staff:

backend/api/views/group_view.py

@@ -40,7 +40,6 @@ def students(self, request, **_):
         )
         return Response(serializer.data)
 
-    # TODO: I can access this endpoint unauthorized
     @action(detail=True, permission_classes=[IsAdminUser | GroupSubmissionPermission])
     def submissions(self, request, **_):
         """Returns a list of submissions for the given group"""

backend/api/views/submission_view.py

@@ -16,7 +16,6 @@
 from rest_framework.viewsets import GenericViewSet
 
 
-# TODO: Permission to ask for logs
 class SubmissionViewSet(RetrieveModelMixin, GenericViewSet):
     queryset = Submission.objects.all()
     serializer_class = SubmissionSerializer