Refactor to builtin generic views where appropriate (#1964)

evap/evaluation/auth.py

@@ -1,7 +1,10 @@
+import inspect
 from functools import wraps
+from typing import Callable
 
 from django.contrib.auth.backends import ModelBackend
 from django.core.exceptions import PermissionDenied
+from django.utils.decorators import method_decorator
 from mozilla_django_oidc.auth import OIDCAuthenticationBackend
 
 from evap.evaluation.models import UserProfile
@@ -45,160 +48,87 @@ def authenticate(self, request, email=None, password=None):  # pylint: disable=a
         return None
 
 
-def user_passes_test(test_func):
+def class_or_function_check_decorator(test_func: Callable[[UserProfile], bool]):
     """
-    Decorator for views that checks whether a user passes a given test
-    (raising 403 if not). The test should be a callable that takes the
-    user object and returns True if the user passes.
-    """
-
-    def decorator(view_func):
-        @wraps(view_func)
-        def _wrapped_view(request, *args, **kwargs):
-            if not test_func(request.user):
-                raise PermissionDenied()
-            return view_func(request, *args, **kwargs)
-
-        return _wrapped_view
-
-    return decorator
-
-
-def internal_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and not an external user
-    """
-
-    def check_user(user):
-        return not user.is_external
-
-    return user_passes_test(check_user)(view_func)
-
+    Transforms a test function into a decorator that can be used on function-based and class-based views.
 
-def staff_permission_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and staff (regardless of staff mode!)
-    """
-
-    def check_user(user):
-        return user.has_staff_permission
-
-    return user_passes_test(check_user)(view_func)
-
-
-def manager_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and a manager
-    """
-
-    def check_user(user):
-        return user.is_manager
-
-    return user_passes_test(check_user)(view_func)
-
-
-def reviewer_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and a reviewer
+    Using the returned decorator on a view enhances the view to return a "Permission Denied" response if the requesting
+    user does not pass the test function.
     """
 
-    def check_user(user):
-        return user.is_reviewer
-
-    return user_passes_test(check_user)(view_func)
-
-
-def grade_publisher_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and a grade publisher
-    """
-
-    def check_user(user):
-        return user.is_grade_publisher
-
-    return user_passes_test(check_user)(view_func)
-
-
-def grade_publisher_or_manager_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and a grade publisher or a manager
-    """
+    def function_decorator(func):
+        @wraps(func)
+        def wrapped(request, *args, **kwargs):
+            if not test_func(request.user):
+                raise PermissionDenied
+            return func(request, *args, **kwargs)
 
-    def check_user(user):
-        return user.is_grade_publisher or user.is_manager
+        return wrapped
 
-    return user_passes_test(check_user)(view_func)
+    def decorator(class_or_function):
+        if inspect.isclass(class_or_function):
+            # See https://docs.djangoproject.com/en/4.2/topics/class-based-views/intro/#decorating-the-class
+            return method_decorator(function_decorator, name="dispatch")(class_or_function)
 
+        assert inspect.isfunction(class_or_function)
+        return function_decorator(class_or_function)
 
-def grade_downloader_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and can download grades
-    """
+    return decorator
 
-    def check_user(user):
-        return user.can_download_grades
 
-    return user_passes_test(check_user)(view_func)
+@class_or_function_check_decorator
+def internal_required(user):
+    return not user.is_external
 
 
-def responsible_or_contributor_or_delegate_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in, is responsible for a course, or is a contributor, or is
-    a delegate.
-    """
+@class_or_function_check_decorator
+def staff_permission_required(user):
+    return user.has_staff_permission
 
-    def check_user(user):
-        return user.is_responsible_or_contributor_or_delegate
 
-    return user_passes_test(check_user)(view_func)
+@class_or_function_check_decorator
+def manager_required(user):
+    return user.is_manager
 
 
-def editor_or_delegate_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in, has edit rights
-    for at least one evaluation or is a delegate for such a person.
-    """
+@class_or_function_check_decorator
+def reviewer_required(user):
+    return user.is_reviewer
 
-    def check_user(user):
-        return user.is_editor_or_delegate
 
-    return user_passes_test(check_user)(view_func)
+@class_or_function_check_decorator
+def grade_publisher_required(user):
+    return user.is_grade_publisher
 
 
-def editor_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and has edit
-    right for at least one evaluation.
-    """
+@class_or_function_check_decorator
+def grade_publisher_or_manager_required(user):
+    return user.is_grade_publisher or user.is_manager
 
-    def check_user(user):
-        return user.is_editor
 
-    return user_passes_test(check_user)(view_func)
+@class_or_function_check_decorator
+def grade_downloader_required(user):
+    return user.can_download_grades
 
 
-def participant_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and
-    participates in at least one evaluation.
-    """
+@class_or_function_check_decorator
+def responsible_or_contributor_or_delegate_required(user):
+    return user.is_responsible_or_contributor_or_delegate
 
-    def check_user(user):
-        return user.is_participant
 
-    return user_passes_test(check_user)(view_func)
+@class_or_function_check_decorator
+def editor_or_delegate_required(user):
+    return user.is_editor_or_delegate
 
 
-def reward_user_required(view_func):
-    """
-    Decorator for views that checks that the user is logged in and can use
-    reward points.
-    """
+@class_or_function_check_decorator
+def participant_required(user):
+    return user.is_participant
 
-    def check_user(user):
-        return can_reward_points_be_used_by(user)
 
-    return user_passes_test(check_user)(view_func)
+@class_or_function_check_decorator
+def reward_user_required(user):
+    return can_reward_points_be_used_by(user)
 
 
 # see https://mozilla-django-oidc.readthedocs.io/en/stable/

evap/evaluation/tests/test_auth.py

@@ -4,11 +4,15 @@
 from django.conf import settings
 from django.contrib.auth.models import Group
 from django.core import mail
+from django.core.exceptions import PermissionDenied
+from django.http import HttpRequest, HttpResponse
 from django.test import override_settings
 from django.urls import reverse
+from django.views import View
 from model_bakery import baker
 
 from evap.evaluation import auth
+from evap.evaluation.auth import class_or_function_check_decorator
 from evap.evaluation.models import Contribution, Evaluation, UserProfile
 from evap.evaluation.tests.tools import WebTest
 
@@ -176,3 +180,51 @@ def test_entering_staff_mode_after_logout_and_login(self):
         page = page.forms["enter-staff-mode-form"].submit().follow().follow()
         self.assertTrue("staff_mode_start_time" in self.app.session)
         self.assertContains(page, "Users")
+
+
+class TestAuthDecorators(WebTest):
+    @classmethod
+    def setUpTestData(cls):
+        @class_or_function_check_decorator
+        def check_decorator(user: UserProfile) -> bool:
+            return getattr(user, "some_condition")  # mocked later
+
+        @check_decorator
+        def function_based_view(_request):
+            return HttpResponse()
+
+        @check_decorator
+        class ClassBasedView(View):
+            def get(self, _request):
+                return HttpResponse()
+
+        cls.user = baker.make(UserProfile, email="[email protected]")
+        cls.function_based_view = function_based_view
+        cls.class_based_view = ClassBasedView.as_view()
+
+    @classmethod
+    def make_request(cls):
+        request = HttpRequest()
+        request.method = "GET"
+        request.user = cls.user
+        return request
+
+    @patch("evap.evaluation.models.UserProfile.some_condition", True, create=True)
+    def test_passing_user_function_based(self):
+        response = self.function_based_view(self.make_request())  # pylint: disable=too-many-function-args
+        self.assertEqual(response.status_code, 200)
+
+    @patch("evap.evaluation.models.UserProfile.some_condition", True, create=True)
+    def test_passing_user_class_based(self):
+        response = self.class_based_view(self.make_request())
+        self.assertEqual(response.status_code, 200)
+
+    @patch("evap.evaluation.models.UserProfile.some_condition", False, create=True)
+    def test_failing_user_function_based(self):
+        with self.assertRaises(PermissionDenied):
+            self.function_based_view(self.make_request())  # pylint: disable=too-many-function-args
+
+    @patch("evap.evaluation.models.UserProfile.some_condition", False, create=True)
+    def test_failing_user_class_based(self):
+        with self.assertRaises(PermissionDenied):
+            self.class_based_view(self.make_request())

evap/evaluation/tools.py

@@ -12,6 +12,7 @@
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from django.utils.translation import get_language
+from django.views.generic import FormView
 
 M = TypeVar("M", bound=Model)
 T = TypeVar("T")
@@ -138,6 +139,50 @@ def assert_not_none(value: T | None) -> T:
     return value
 
 
+class FormsetView(FormView):
+    """
+    Just like `FormView`, but with a renaming from "form" to "formset".
+    """
+
+    @property
+    def form_class(self):
+        return self.formset_class
+
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+        context["formset"] = context.pop("form")
+        return context
+
+    # As an example for the logic, consider the following: Django calls `get_form_kwargs`, which we delegate to
+    # `get_formset_kwargs`. Users can thus override `get_formset_kwargs` instead. If it is not overridden, we delegate
+    # to the original `get_form_kwargs` instead. The same approach is used for the other renamed methods.
+
+    def get_form_kwargs(self):
+        return self.get_formset_kwargs()
+
+    def get_formset_kwargs(self):
+        return super().get_form_kwargs()
+
+    def form_valid(self, form):
+        return self.formset_valid(form)
+
+    def formset_valid(self, formset):
+        return super().form_valid(formset)
+
+
+class SaveValidFormMixin:
+    """
+    Call `form.save()` if the submitted form is valid.
+
+    Django's `ModelFormMixin` (which inherits from `SingleObjectMixin`) does the same, but cannot always be used, for
+    example if a formset for a collection of objects is submitted.
+    """
+
+    def form_valid(self, form):
+        form.save()
+        return super().form_valid(form)
+
+
 class AttachmentResponse(HttpResponse):
     """
     Helper class that sets the correct Content-Disposition header for a given

evap/grades/templates/grades_course_view.html

@@ -30,7 +30,7 @@ <h3 class="mb-3">{{ course.name }} ({{ semester.name }})</h3>
                                     <a href="{% url 'grades:download_grades' grade_document.id %}" class="btn btn-sm btn-light" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Download' %}"><span class="fas fa-download"></span></a>
                                     {% if user.is_grade_publisher %}
                                         <a href="{% url 'grades:edit_grades' grade_document.id %}" class="btn btn-sm btn-light" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Edit' %}"><span class="fas fa-pencil"></span></a>
-                                        <button type="button" {{ disable_if_archived }} onclick="deleteGradedocumentModalShow({{ grade_document.id }}, '{{ grade_document.description|escapejs }}');" class="btn btn-sm btn-danger" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Delete' %}">
+                                        <button type="button" onclick="deleteGradedocumentModalShow({{ grade_document.id }}, '{{ grade_document.description|escapejs }}');" class="btn btn-sm btn-danger" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Delete' %}">
                                             <span class="fas fa-trash"></span>
                                         </button>
                                     {% endif %}
@@ -45,8 +45,8 @@ <h3 class="mb-3">{{ course.name }} ({{ semester.name }})</h3>
         </div>
     </div>
     {% if user.is_grade_publisher %}
-        <a href="{% url 'grades:upload_grades' course.id %}" class="btn btn-dark {{ disable_if_archived }}">{% trans 'Upload new midterm grades' %}</a>
-        <a href="{% url 'grades:upload_grades' course.id %}?final=true" class="btn btn-dark {{ disable_if_archived }}">{% trans 'Upload new final grades' %}</a>
+        <a href="{% url 'grades:upload_grades' course.id %}" class="btn btn-dark">{% trans 'Upload new midterm grades' %}</a>
+        <a href="{% url 'grades:upload_grades' course.id %}?final=true" class="btn btn-dark">{% trans 'Upload new final grades' %}</a>
     {% endif %}
 {% endblock %}
 

evap/grades/templates/grades_semester_view.html

@@ -72,7 +72,7 @@ <h3 class="col-8 mb-0">
                                 <td class="text-end" style="min-width:72px">
                                     {% if not course.gets_no_grade_documents %}
                                         {% if num_final_grades == 0 %}
-                                            <button type="button" {{ disable_if_archived }} onclick="confirmNouploadModalShow({{ course.id }}, '{{ course.name|escapejs }}');" class="btn btn-sm btn-secondary" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Confirm that final grades have been submitted but will not be uploaded.' %}">
+                                            <button type="button" onclick="confirmNouploadModalShow({{ course.id }}, '{{ course.name|escapejs }}');" class="btn btn-sm btn-secondary" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Confirm that final grades have been submitted but will not be uploaded.' %}">
                                                 <span class="fas fa-xmark"></span>
                                             </button>
                                             <div class="btn-group" role="group" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Upload grade document' %}">
@@ -85,7 +85,7 @@ <h3 class="col-8 mb-0">
                                                 </div>
                                             </div>
                                         {% else %}
-                                            <a href="{% url 'grades:course_view' course.id %}" class="btn btn-sm btn-light {{ disable_if_archived }}" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Change grade document' %}"><span class="fas fa-pencil"></span></a>
+                                            <a href="{% url 'grades:course_view' course.id %}" class="btn btn-sm btn-light" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Change grade document' %}"><span class="fas fa-pencil"></span></a>
                                         {% endif %}
                                     {% endif %}
                                 </td>