From ebc511007dafaca504f9cf76c7cf0d51a4b4224e Mon Sep 17 00:00:00 2001 From: Niklas Mohrin Date: Mon, 9 Oct 2023 20:21:29 +0200 Subject: [PATCH] Refactor to builtin generic views where appropriate (#1964) --- evap/evaluation/auth.py | 178 +++------- evap/evaluation/tests/test_auth.py | 52 +++ evap/evaluation/tools.py | 45 +++ evap/grades/templates/grades_course_view.html | 6 +- .../templates/grades_semester_view.html | 4 +- evap/grades/urls.py | 7 +- evap/grades/views.py | 88 ++--- evap/rewards/urls.py | 4 +- evap/rewards/views.py | 39 +-- evap/staff/forms.py | 7 +- evap/staff/tests/test_views.py | 44 ++- evap/staff/urls.py | 20 +- evap/staff/views.py | 311 +++++++++--------- 13 files changed, 421 insertions(+), 384 deletions(-) diff --git a/evap/evaluation/auth.py b/evap/evaluation/auth.py index 085e8b45af..b71a8dea46 100644 --- a/evap/evaluation/auth.py +++ b/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/ diff --git a/evap/evaluation/tests/test_auth.py b/evap/evaluation/tests/test_auth.py index a3cf03d4cc..899b6e4753 100644 --- a/evap/evaluation/tests/test_auth.py +++ b/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="testuser@institution.example.com") + 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()) diff --git a/evap/evaluation/tools.py b/evap/evaluation/tools.py index dac303a995..68bb8f702b 100644 --- a/evap/evaluation/tools.py +++ b/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 diff --git a/evap/grades/templates/grades_course_view.html b/evap/grades/templates/grades_course_view.html index 7ab7b60fcb..1bf12162c8 100644 --- a/evap/grades/templates/grades_course_view.html +++ b/evap/grades/templates/grades_course_view.html @@ -30,7 +30,7 @@

{{ course.name }} ({{ semester.name }})

{% if user.is_grade_publisher %} - {% endif %} @@ -45,8 +45,8 @@

{{ course.name }} ({{ semester.name }})

{% if user.is_grade_publisher %} - {% trans 'Upload new midterm grades' %} - {% trans 'Upload new final grades' %} + {% trans 'Upload new midterm grades' %} + {% trans 'Upload new final grades' %} {% endif %} {% endblock %} diff --git a/evap/grades/templates/grades_semester_view.html b/evap/grades/templates/grades_semester_view.html index ae9d586115..0a026155f0 100644 --- a/evap/grades/templates/grades_semester_view.html +++ b/evap/grades/templates/grades_semester_view.html @@ -72,7 +72,7 @@

{% if not course.gets_no_grade_documents %} {% if num_final_grades == 0 %} -
@@ -85,7 +85,7 @@

{% else %} - + {% endif %} {% endif %} diff --git a/evap/grades/urls.py b/evap/grades/urls.py index c932577a57..541d26e2cb 100644 --- a/evap/grades/urls.py +++ b/evap/grades/urls.py @@ -5,11 +5,10 @@ app_name = "grades" urlpatterns = [ - path("", views.index, name="index"), - + path("", views.IndexView.as_view(), name="index"), path("download/", views.download_grades, name="download_grades"), - path("semester/", views.semester_view, name="semester_view"), - path("course/", views.course_view, name="course_view"), + path("semester/", views.SemesterView.as_view(), name="semester_view"), + path("course/", views.CourseView.as_view(), name="course_view"), path("course//upload", views.upload_grades, name="upload_grades"), path("grade_document//edit", views.edit_grades, name="edit_grades"), diff --git a/evap/grades/views.py b/evap/grades/views.py index 2e9556fe64..21f9f458ab 100644 --- a/evap/grades/views.py +++ b/evap/grades/views.py @@ -6,6 +6,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.utils.translation import gettext as _ from django.views.decorators.http import require_GET, require_POST +from django.views.generic import DetailView, TemplateView from evap.evaluation.auth import ( grade_downloader_required, @@ -19,12 +20,14 @@ @grade_publisher_required -def index(request): - template_data = { - "semesters": Semester.objects.filter(grade_documents_are_deleted=False), - "disable_breadcrumb_grades": True, - } - return render(request, "grades_index.html", template_data) +class IndexView(TemplateView): + template_name = "grades_index.html" + + def get_context_data(self, **kwargs): + return super().get_context_data(**kwargs) | { + "semesters": Semester.objects.filter(grade_documents_are_deleted=False), + "disable_breadcrumb_grades": True, + } def course_grade_document_count_tuples(courses: QuerySet[Course]) -> list[tuple[Course, int, int]]: @@ -41,42 +44,51 @@ def course_grade_document_count_tuples(courses: QuerySet[Course]) -> list[tuple[ @grade_publisher_required -def semester_view(request, semester_id): - semester = get_object_or_404(Semester, id=semester_id) - if semester.grade_documents_are_deleted: - raise PermissionDenied - - courses = ( - semester.courses.filter(evaluations__wait_for_grade_upload_before_publishing=True) - .exclude(evaluations__state=Evaluation.State.NEW) - .distinct() - ) - courses = course_grade_document_count_tuples(courses) +class SemesterView(DetailView): + template_name = "grades_semester_view.html" + model = Semester + pk_url_kwarg = "semester_id" + + object: Semester + + def get_object(self, *args, **kwargs): + semester = super().get_object(*args, **kwargs) + if semester.grade_documents_are_deleted: + raise PermissionDenied + return semester + + def get_context_data(self, **kwargs): + courses = ( + self.object.courses.filter(evaluations__wait_for_grade_upload_before_publishing=True) + .exclude(evaluations__state=Evaluation.State.NEW) + .distinct() + ) + courses = course_grade_document_count_tuples(courses) - template_data = { - "semester": semester, - "courses": courses, - "disable_if_archived": "disabled" if semester.grade_documents_are_deleted else "", - "disable_breadcrumb_semester": True, - } - return render(request, "grades_semester_view.html", template_data) + return super().get_context_data(**kwargs) | { + "courses": courses, + "disable_breadcrumb_semester": True, + } @grade_publisher_or_manager_required -def course_view(request, course_id): - course = get_object_or_404(Course, id=course_id) - semester = course.semester - if semester.grade_documents_are_deleted: - raise PermissionDenied - - template_data = { - "semester": semester, - "course": course, - "grade_documents": course.grade_documents.all(), - "disable_if_archived": "disabled" if semester.grade_documents_are_deleted else "", - "disable_breadcrumb_course": True, - } - return render(request, "grades_course_view.html", template_data) +class CourseView(DetailView): + template_name = "grades_course_view.html" + model = Course + pk_url_kwarg = "course_id" + + def get_object(self, *args, **kwargs): + course = super().get_object(*args, **kwargs) + if course.semester.grade_documents_are_deleted: + raise PermissionDenied + return course + + def get_context_data(self, **kwargs): + return super().get_context_data(**kwargs) | { + "semester": self.object.semester, + "grade_documents": self.object.grade_documents.all(), + "disable_breadcrumb_course": True, + } def on_grading_process_finished(course): diff --git a/evap/rewards/urls.py b/evap/rewards/urls.py index bc4c135a19..a30b734328 100644 --- a/evap/rewards/urls.py +++ b/evap/rewards/urls.py @@ -8,8 +8,8 @@ path("", views.index, name="index"), path("reward_point_redemption_events/", views.reward_point_redemption_events, name="reward_point_redemption_events"), - path("reward_point_redemption_event/create", views.reward_point_redemption_event_create, name="reward_point_redemption_event_create"), - path("reward_point_redemption_event//edit", views.reward_point_redemption_event_edit, name="reward_point_redemption_event_edit"), + path("reward_point_redemption_event/create", views.RewardPointRedemptionEventCreateView.as_view(), name="reward_point_redemption_event_create"), + path("reward_point_redemption_event//edit", views.RewardPointRedemptionEventEditView.as_view(), name="reward_point_redemption_event_edit"), path("reward_point_redemption_event//export", views.reward_point_redemption_event_export, name="reward_point_redemption_event_export"), path("reward_point_redemption_event/delete", views.reward_point_redemption_event_delete, name="reward_point_redemption_event_delete"), diff --git a/evap/rewards/views.py b/evap/rewards/views.py index 0dc58eee4f..027d2b75ef 100644 --- a/evap/rewards/views.py +++ b/evap/rewards/views.py @@ -1,13 +1,17 @@ from datetime import datetime from django.contrib import messages +from django.contrib.messages.views import SuccessMessageMixin from django.core.exceptions import BadRequest, SuspiciousOperation from django.db.models import Sum from django.http import HttpResponse from django.shortcuts import get_object_or_404, redirect, render +from django.urls import reverse_lazy from django.utils.translation import get_language from django.utils.translation import gettext as _ +from django.utils.translation import gettext_lazy from django.views.decorators.http import require_POST +from django.views.generic import CreateView, UpdateView from evap.evaluation.auth import manager_required, reward_user_required from evap.evaluation.models import Semester @@ -104,30 +108,23 @@ def reward_point_redemption_events(request): @manager_required -def reward_point_redemption_event_create(request): - event = RewardPointRedemptionEvent() - form = RewardPointRedemptionEventForm(request.POST or None, instance=event) - - if form.is_valid(): - form.save() - messages.success(request, _("Successfully created event.")) - return redirect("rewards:reward_point_redemption_events") - - return render(request, "rewards_reward_point_redemption_event_form.html", {"form": form}) +class RewardPointRedemptionEventCreateView(SuccessMessageMixin, CreateView): + model = RewardPointRedemptionEvent + form_class = RewardPointRedemptionEventForm + template_name = "rewards_reward_point_redemption_event_form.html" + success_url = reverse_lazy("rewards:reward_point_redemption_events") + success_message = gettext_lazy("Successfully created event.") @manager_required -def reward_point_redemption_event_edit(request, event_id): - event = get_object_or_404(RewardPointRedemptionEvent, id=event_id) - form = RewardPointRedemptionEventForm(request.POST or None, instance=event) - - if form.is_valid(): - event = form.save() - - messages.success(request, _("Successfully updated event.")) - return redirect("rewards:reward_point_redemption_events") - - return render(request, "rewards_reward_point_redemption_event_form.html", {"event": event, "form": form}) +class RewardPointRedemptionEventEditView(SuccessMessageMixin, UpdateView): + model = RewardPointRedemptionEvent + form_class = RewardPointRedemptionEventForm + template_name = "rewards_reward_point_redemption_event_form.html" + success_url = reverse_lazy("rewards:reward_point_redemption_events") + success_message = gettext_lazy("Successfully updated event.") + pk_url_kwarg = "event_id" + context_object_name = "event" @require_POST diff --git a/evap/staff/forms.py b/evap/staff/forms.py index e06898b3d0..33454971ff 100644 --- a/evap/staff/forms.py +++ b/evap/staff/forms.py @@ -1046,6 +1046,7 @@ def save(self, *args, **kw): cache_results(evaluation) self.instance.save() + return self.instance class UserMergeSelectionForm(forms.Form): @@ -1057,12 +1058,6 @@ class UserEditSelectionForm(forms.Form): user = UserModelChoiceField(UserProfile.objects.all()) -class EmailTemplateForm(forms.ModelForm): - class Meta: - model = EmailTemplate - fields = ("subject", "plain_content", "html_content") - - class FaqSectionForm(forms.ModelForm): class Meta: model = FaqSection diff --git a/evap/staff/tests/test_views.py b/evap/staff/tests/test_views.py index 462cd5d292..ee54d83851 100644 --- a/evap/staff/tests/test_views.py +++ b/evap/staff/tests/test_views.py @@ -1947,20 +1947,27 @@ def test_edit_course(self): self.course = Course.objects.get(pk=self.course.pk) self.assertEqual(self.course.name_en, "A different name") - @patch("evap.staff.views.redirect") - def test_operation_redirects(self, mock_redirect): - mock_redirect.side_effect = lambda *_args: HttpResponse() + @patch("evap.staff.views.reverse") + def test_operation_redirects(self, mock_reverse): + mock_reverse.return_value = "/very_legit_url" - self.prepare_form("a").submit("operation", value="save") - self.assertEqual(mock_redirect.call_args.args[0], "staff:semester_view") + response = self.prepare_form("a").submit("operation", value="save") + self.assertEqual(mock_reverse.call_args.args[0], "staff:semester_view") + self.assertRedirects(response, "/very_legit_url", fetch_redirect_response=False) - self.prepare_form("b").submit("operation", value="save_create_evaluation") - self.assertEqual(mock_redirect.call_args.args[0], "staff:evaluation_create_for_course") + response = self.prepare_form("b").submit("operation", value="save_create_evaluation") + self.assertEqual(mock_reverse.call_args.args[0], "staff:evaluation_create_for_course") + self.assertRedirects(response, "/very_legit_url", fetch_redirect_response=False) - self.prepare_form("c").submit("operation", value="save_create_single_result") - self.assertEqual(mock_redirect.call_args.args[0], "staff:single_result_create_for_course") + response = self.prepare_form("c").submit("operation", value="save_create_single_result") + self.assertEqual(mock_reverse.call_args.args[0], "staff:single_result_create_for_course") + self.assertRedirects(response, "/very_legit_url", fetch_redirect_response=False) - self.assertEqual(mock_redirect.call_count, 3) + self.assertEqual(mock_reverse.call_count, 3) + + @patch("evap.evaluation.models.Course.can_be_edited_by_manager", False) + def test_uneditable_course(self): + self.prepare_form(name_en="A different name").submit("operation", value="save", status=400) class TestCourseDeleteView(DeleteViewTestMixin, WebTestStaffMode): @@ -3480,11 +3487,20 @@ def test_emailtemplate(self): self.assertEqual(self.template.plain_content, "plain_content: mflkd862xmnbo5") self.assertEqual(self.template.html_content, "html_content:

mflkd862xmnbo5

") - def test_review_reminder_template_tag(self): - review_reminder_template = EmailTemplate.objects.get(name=EmailTemplate.TEXT_ANSWER_REVIEW_REMINDER) - page = self.app.get(f"/staff/template/{review_reminder_template.pk}", user=self.manager, status=200) + def test_available_variables(self): + # We want to trigger all paths to ensure there are no syntax errors. + expected_variables = { + EmailTemplate.STUDENT_REMINDER: "first_due_in_days", + EmailTemplate.EDITOR_REVIEW_NOTICE: "evaluations", + EmailTemplate.TEXT_ANSWER_REVIEW_REMINDER: "evaluation_url_tuples", + EmailTemplate.EVALUATION_STARTED: "due_evaluations", + EmailTemplate.DIRECT_DELEGATION: "delegate_user", + } - self.assertContains(page, "evaluation_url_tuples") + for name, variable in expected_variables.items(): + template = EmailTemplate.objects.get(name=name) + page = self.app.get(f"/staff/template/{template.pk}", user=self.manager, status=200) + self.assertContains(page, variable) class TestTextAnswerWarningsView(WebTestStaffMode): diff --git a/evap/staff/urls.py b/evap/staff/urls.py index 97603cb9ba..394d779cf4 100644 --- a/evap/staff/urls.py +++ b/evap/staff/urls.py @@ -9,9 +9,9 @@ path("", views.index, name="index"), path("semester/", RedirectView.as_view(url='/staff/', permanent=True)), - path("semester/create", views.semester_create, name="semester_create"), + path("semester/create", views.SemesterCreateView.as_view(), name="semester_create"), path("semester/", views.semester_view, name="semester_view"), - path("semester//edit", views.semester_edit, name="semester_edit"), + path("semester//edit", views.SemesterEditView.as_view(), name="semester_edit"), path("semester/make_active", views.semester_make_active, name="semester_make_active"), path("semester/delete", views.semester_delete, name="semester_delete"), path("semester//import", views.semester_import, name="semester_import"), @@ -40,7 +40,7 @@ path("semester//course/create", views.course_create, name="course_create"), path("course/delete", views.course_delete, name="course_delete"), - path("course//edit", views.course_edit, name="course_edit"), + path("course//edit", views.CourseEditView.as_view(), name="course_edit"), path("course//copy", views.course_copy, name="course_copy"), path("semester//singleresult/create", views.single_result_create_for_semester, name="single_result_create_for_semester"), @@ -64,32 +64,32 @@ path("questionnaire/questionnaire_visibility", views.questionnaire_visibility, name="questionnaire_visibility"), path("questionnaire/questionnaire_set_locked", views.questionnaire_set_locked, name="questionnaire_set_locked"), - path("degrees/", views.degree_index, name="degree_index"), + path("degrees/", views.DegreeIndexView.as_view(), name="degree_index"), - path("course_types/", views.course_type_index, name="course_type_index"), + path("course_types/", views.CourseTypeIndexView.as_view(), name="course_type_index"), path("course_types/merge", views.course_type_merge_selection, name="course_type_merge_selection"), path("course_types//merge/", views.course_type_merge, name="course_type_merge"), path("user/", views.user_index, name="user_index"), - path("user/create", views.user_create, name="user_create"), + path("user/create", views.UserCreateView.as_view(), name="user_create"), path("user/import", views.user_import, name="user_import"), path("user//edit", views.user_edit, name="user_edit"), path("user/list", views.user_list, name="user_list"), path("user/delete", views.user_delete, name="user_delete"), path("user/resend_email", views.user_resend_email, name="user_resend_email"), path("user/bulk_update", views.user_bulk_update, name="user_bulk_update"), - path("user/merge", views.user_merge_selection, name="user_merge_selection"), + path("user/merge", views.UserMergeSelectionView.as_view(), name="user_merge_selection"), path("user//merge/", views.user_merge, name="user_merge"), path("template/", RedirectView.as_view(url='/staff/', permanent=True)), - path("template/", views.template_edit, name="template_edit"), + path("template/", views.TemplateEditView.as_view(), name="template_edit"), path("text_answer_warnings/", views.text_answer_warnings_index, name="text_answer_warnings"), - path("faq/", views.faq_index, name="faq_index"), + path("faq/", views.FaqIndexView.as_view(), name="faq_index"), path("faq/", views.faq_section, name="faq_section"), - path("infotexts/", views.infotexts, name="infotexts"), + path("infotexts/", views.InfotextsView.as_view(), name="infotexts"), path("download_sample_file/", views.download_sample_file, name="download_sample_file"), diff --git a/evap/staff/views.py b/evap/staff/views.py index d6f143a7b6..8cd2a63f2a 100644 --- a/evap/staff/views.py +++ b/evap/staff/views.py @@ -9,6 +9,7 @@ import openpyxl from django.conf import settings from django.contrib import messages +from django.contrib.messages.views import SuccessMessageMixin from django.core.exceptions import PermissionDenied, SuspiciousOperation from django.db import IntegrityError, transaction from django.db.models import BooleanField, Case, Count, ExpressionWrapper, IntegerField, Prefetch, Q, Sum, When @@ -17,12 +18,13 @@ from django.forms.models import inlineformset_factory, modelformset_factory from django.http import Http404, HttpRequest, HttpResponse, HttpResponseBadRequest, HttpResponseRedirect from django.shortcuts import get_object_or_404, redirect, render -from django.urls import reverse +from django.urls import reverse, reverse_lazy from django.utils.html import format_html from django.utils.translation import get_language from django.utils.translation import gettext as _ from django.utils.translation import gettext_lazy, ngettext from django.views.decorators.http import require_POST +from django.views.generic import CreateView, FormView, UpdateView from django_stubs_ext import StrOrPromise from evap.contributor.views import export_contributor_results @@ -48,7 +50,9 @@ ) from evap.evaluation.tools import ( AttachmentResponse, + FormsetView, HttpResponseNoContent, + SaveValidFormMixin, get_object_from_dict_pk_entry_or_logged_40x, get_parameter_from_url_or_session, sort_formset, @@ -71,7 +75,6 @@ CourseTypeForm, CourseTypeMergeSelectionForm, DegreeForm, - EmailTemplateForm, EvaluationCopyForm, EvaluationEmailForm, EvaluationForm, @@ -571,15 +574,26 @@ def evaluation_operation(request, semester_id): @manager_required -def semester_create(request): - form = SemesterForm(request.POST or None) +class SemesterCreateView(SuccessMessageMixin, CreateView): + template_name = "staff_semester_form.html" + model = Semester + form_class = SemesterForm + success_message = gettext_lazy("Successfully created semester.") - if form.is_valid(): - semester = form.save() - messages.success(request, _("Successfully created semester.")) - return redirect("staff:semester_view", semester.id) + def get_success_url(self): + return reverse("staff:semester_view", args=[self.object.id]) + + +@manager_required +class SemesterEditView(SuccessMessageMixin, UpdateView): + template_name = "staff_semester_form.html" + model = Semester + form_class = SemesterForm + pk_url_kwarg = "semester_id" + success_message = gettext_lazy("Successfully updated semester.") - return render(request, "staff_semester_form.html", {"form": form}) + def get_success_url(self): + return reverse("staff:semester_view", args=[self.object.id]) @require_POST @@ -595,19 +609,6 @@ def semester_make_active(request): return HttpResponse() -@manager_required -def semester_edit(request, semester_id): - semester = get_object_or_404(Semester, id=semester_id) - form = SemesterForm(request.POST or None, instance=semester) - - if form.is_valid(): - semester = form.save() - messages.success(request, _("Successfully updated semester.")) - return redirect("staff:semester_view", semester.id) - - return render(request, "staff_semester_form.html", {"semester": semester, "form": form}) - - @require_POST @manager_required def semester_delete(request): @@ -615,12 +616,13 @@ def semester_delete(request): if not semester.can_be_deleted_by_manager: raise SuspiciousOperation("Deleting semester not allowed") - RatingAnswerCounter.objects.filter(contribution__evaluation__course__semester=semester).delete() - TextAnswer.objects.filter(contribution__evaluation__course__semester=semester).delete() - Contribution.objects.filter(evaluation__course__semester=semester).delete() - Evaluation.objects.filter(course__semester=semester).delete() - Course.objects.filter(semester=semester).delete() - semester.delete() + with transaction.atomic(): + RatingAnswerCounter.objects.filter(contribution__evaluation__course__semester=semester).delete() + TextAnswer.objects.filter(contribution__evaluation__course__semester=semester).delete() + Contribution.objects.filter(evaluation__course__semester=semester).delete() + Evaluation.objects.filter(course__semester=semester).delete() + Course.objects.filter(semester=semester).delete() + semester.delete() return redirect("staff:index") @@ -1039,41 +1041,47 @@ def course_copy(request, course_id): @manager_required -def course_edit(request, course_id): - course = get_object_or_404(Course, id=course_id) +class CourseEditView(SuccessMessageMixin, UpdateView): + model = Course + pk_url_kwarg = "course_id" + form_class = CourseForm + template_name = "staff_course_form.html" + success_message = gettext_lazy("Successfully updated course.") - course_form = CourseForm(request.POST or None, instance=course) - editable = course.can_be_edited_by_manager + object: Course - if request.method == "POST" and not editable: - raise SuspiciousOperation("Modifying this course is not allowed.") + def get_object(self, *args, **kwargs): + course = super().get_object(*args, **kwargs) + if self.request.method == "POST" and not course.can_be_edited_by_manager: + raise SuspiciousOperation("Modifying this course is not allowed.") + return course - operation = request.POST.get("operation") + def get_context_data(self, **kwargs): + context_data = super().get_context_data(**kwargs) | { + "semester": self.object.semester, + "editable": self.object.can_be_edited_by_manager, + "disable_breadcrumb_course": True, + } + context_data["course_form"] = context_data.pop("form") + return context_data - if course_form.is_valid(): - if operation not in ("save", "save_create_evaluation", "save_create_single_result"): + def form_valid(self, form): + if self.request.POST.get("operation") not in ("save", "save_create_evaluation", "save_create_single_result"): raise SuspiciousOperation("Invalid POST operation") - if course_form.has_changed(): - course = course_form.save() - update_template_cache_of_published_evaluations_in_course(course) + response = super().form_valid(form) + if form.has_changed(): + update_template_cache_of_published_evaluations_in_course(self.object) + return response - messages.success(request, _("Successfully updated course.")) - if operation == "save_create_evaluation": - return redirect("staff:evaluation_create_for_course", course.id) - if operation == "save_create_single_result": - return redirect("staff:single_result_create_for_course", course.id) - - return redirect("staff:semester_view", course.semester.id) - - template_data = { - "course": course, - "semester": course.semester, - "course_form": course_form, - "editable": editable, - "disable_breadcrumb_course": True, - } - return render(request, "staff_course_form.html", template_data) + def get_success_url(self): + match self.request.POST["operation"]: + case "save": + return reverse("staff:semester_view", args=[self.object.semester.id]) + case "save_create_evaluation": + return reverse("staff:evaluation_create_for_course", args=[self.object.id]) + case "save_create_single_result": + return reverse("staff:single_result_create_for_course", args=[self.object.id]) @require_POST @@ -1978,37 +1986,33 @@ def questionnaire_set_locked(request): @manager_required -def degree_index(request): - degrees = Degree.objects.all() - - DegreeFormset = modelformset_factory( - Degree, form=DegreeForm, formset=ModelWithImportNamesFormset, can_delete=True, extra=1 +class DegreeIndexView(SuccessMessageMixin, SaveValidFormMixin, FormsetView): + model = Degree + formset_class = modelformset_factory( + Degree, + form=DegreeForm, + formset=ModelWithImportNamesFormset, + can_delete=True, + extra=1, ) - formset = DegreeFormset(request.POST or None, queryset=degrees) - - if formset.is_valid(): - formset.save() - messages.success(request, _("Successfully updated the degrees.")) - return redirect("staff:degree_index") - - return render(request, "staff_degree_index.html", {"formset": formset}) + template_name = "staff_degree_index.html" + success_url = reverse_lazy("staff:degree_index") + success_message = gettext_lazy("Successfully updated the degrees.") @manager_required -def course_type_index(request): - course_types = CourseType.objects.all() - - CourseTypeFormset = modelformset_factory( - CourseType, form=CourseTypeForm, formset=ModelWithImportNamesFormset, can_delete=True, extra=1 +class CourseTypeIndexView(SuccessMessageMixin, SaveValidFormMixin, FormsetView): + model = CourseType + formset_class = modelformset_factory( + CourseType, + form=CourseTypeForm, + formset=ModelWithImportNamesFormset, + can_delete=True, + extra=1, ) - formset = CourseTypeFormset(request.POST or None, queryset=course_types) - - if formset.is_valid(): - formset.save() - messages.success(request, _("Successfully updated the course types.")) - return redirect("staff:course_type_index") - - return render(request, "staff_course_type_index.html", {"formset": formset}) + template_name = "staff_course_type_index.html" + success_url = reverse_lazy("staff:course_type_index") + success_message = gettext_lazy("Successfully updated the course types.") @manager_required @@ -2117,15 +2121,12 @@ def user_list(request): @manager_required -def user_create(request): - form = UserForm(request.POST or None, instance=UserProfile()) - - if form.is_valid(): - form.save() - messages.success(request, _("Successfully created user.")) - return redirect("staff:user_index") - - return render(request, "staff_user_form.html", {"form": form}) +class UserCreateView(SuccessMessageMixin, CreateView): + model = UserProfile + form_class = UserForm + template_name = "staff_user_form.html" + success_url = reverse_lazy("staff:user_index") + success_message = gettext_lazy("Successfully created user.") @manager_required @@ -2284,15 +2285,16 @@ def user_bulk_update(request): @manager_required -def user_merge_selection(request): - form = UserMergeSelectionForm(request.POST or None) - - if form.is_valid(): - main_user = form.cleaned_data["main_user"] - other_user = form.cleaned_data["other_user"] - return redirect("staff:user_merge", main_user.id, other_user.id) +class UserMergeSelectionView(FormView): + form_class = UserMergeSelectionForm + template_name = "staff_user_merge_selection.html" - return render(request, "staff_user_merge_selection.html", {"form": form}) + def get_success_url(self): + return redirect( + "staff:user_merge", + self.form.cleaned_data["main_user"].id, + self.form.cleaned_data["other_user"].id, + ) @manager_required @@ -2323,61 +2325,59 @@ def user_merge(request, main_user_id, other_user_id): @manager_required -def template_edit(request, template_id): - template = get_object_or_404(EmailTemplate, id=template_id) - form = EmailTemplateForm(request.POST or None, request.FILES or None, instance=template) +class TemplateEditView(SuccessMessageMixin, UpdateView): + model = EmailTemplate + pk_url_kwarg = "template_id" + fields = ("subject", "plain_content", "html_content") + success_message = gettext_lazy("Successfully updated template.") + success_url = reverse_lazy("staff:index") + template_name = "staff_template_form.html" - if form.is_valid(): - form.save() - messages.success(request, _("Successfully updated template.")) - return redirect("staff:index") - - available_variables = [ - "contact_email", - "page_url", - "login_url", # only if they need it - "user", - ] + def get_context_data(self, **kwargs) -> dict: + context = super().get_context_data(**kwargs) + template = context["template"] = context.pop("emailtemplate") - if template.name == EmailTemplate.STUDENT_REMINDER: - available_variables += ["first_due_in_days", "due_evaluations"] - elif template.name in [ - EmailTemplate.EDITOR_REVIEW_NOTICE, - EmailTemplate.EDITOR_REVIEW_REMINDER, - EmailTemplate.PUBLISHING_NOTICE_CONTRIBUTOR, - EmailTemplate.PUBLISHING_NOTICE_PARTICIPANT, - ]: - available_variables += ["evaluations"] - elif template.name == EmailTemplate.TEXT_ANSWER_REVIEW_REMINDER: - available_variables += ["evaluation_url_tuples"] - elif template.name == EmailTemplate.EVALUATION_STARTED: - available_variables += ["evaluations", "due_evaluations"] - elif template.name == EmailTemplate.DIRECT_DELEGATION: - available_variables += ["evaluation", "delegate_user"] - - available_variables = ["{{ " + variable + " }}" for variable in available_variables] - available_variables.sort() + available_variables = [ + "contact_email", + "page_url", + "login_url", # only if they need it + "user", + ] - return render( - request, - "staff_template_form.html", - {"form": form, "template": template, "available_variables": available_variables}, - ) + if template.name == EmailTemplate.STUDENT_REMINDER: + available_variables += ["first_due_in_days", "due_evaluations"] + elif template.name in [ + EmailTemplate.EDITOR_REVIEW_NOTICE, + EmailTemplate.EDITOR_REVIEW_REMINDER, + EmailTemplate.PUBLISHING_NOTICE_CONTRIBUTOR, + EmailTemplate.PUBLISHING_NOTICE_PARTICIPANT, + ]: + available_variables += ["evaluations"] + elif template.name == EmailTemplate.TEXT_ANSWER_REVIEW_REMINDER: + available_variables += ["evaluation_url_tuples"] + elif template.name == EmailTemplate.EVALUATION_STARTED: + available_variables += ["evaluations", "due_evaluations"] + elif template.name == EmailTemplate.DIRECT_DELEGATION: + available_variables += ["evaluation", "delegate_user"] + available_variables = ["{{ " + variable + " }}" for variable in available_variables] + available_variables.sort() -@manager_required -def faq_index(request): - sections = FaqSection.objects.all() + context["available_variables"] = available_variables - SectionFormset = modelformset_factory(FaqSection, form=FaqSectionForm, can_delete=True, extra=1) - formset = SectionFormset(request.POST or None, queryset=sections) + return context - if formset.is_valid(): - formset.save() - messages.success(request, _("Successfully updated the FAQ sections.")) - return redirect("staff:faq_index") - return render(request, "staff_faq_index.html", {"formset": formset, "sections": sections}) +@manager_required +class FaqIndexView(SuccessMessageMixin, SaveValidFormMixin, FormsetView): + model = FaqSection + formset_class = modelformset_factory(FaqSection, form=FaqSectionForm, can_delete=True, extra=1) + template_name = "staff_faq_index.html" + success_url = reverse_lazy("staff:faq_index") + success_message = gettext_lazy("Successfully updated the FAQ sections.") + + def get_context_data(self, **kwargs): + return super().get_context_data(**kwargs) | {"sections": FaqSection.objects.all()} @manager_required @@ -2400,20 +2400,11 @@ def faq_section(request, section_id): @manager_required -def infotexts(request): - InfotextFormSet = modelformset_factory(Infotext, form=InfotextForm, edit_only=True, extra=0) - formset = InfotextFormSet(request.POST or None) - - if formset.is_valid(): - formset.save() - messages.success(request, _("Successfully updated the infotext entries.")) - return redirect("staff:infotexts") - - return render( - request, - "staff_infotexts.html", - {"formset": formset}, - ) +class InfotextsView(SuccessMessageMixin, SaveValidFormMixin, FormsetView): + formset_class = modelformset_factory(Infotext, form=InfotextForm, edit_only=True, extra=0) + template_name = "staff_infotexts.html" + success_url = reverse_lazy("staff:infotexts") + success_message = gettext_lazy("Successfully updated the infotext entries.") @manager_required