Remove functionally dependent ids from URLs (#1836)

For example, we had something like `/semester/42/course/1337` before,
but the `/course/1337` already implies that the semester should be - the
course's semester. The motivation for this change is a (only staff) view
I found in another PR that checked some condition for a course _on the
semester_, but never ensured that the semester from the URL matched the
course's semester.

I reverted the change for `evaluation_operation`, because it is more
tricky there: There could be no evaluation sent so we wouldn't know
where to redirect the client to if the semester wasn't there anymore. It
would be great if we could just respond with some HTTP code that says
"you made a mistake, please reload your current URL", but I couldn't
find anything (`HTTP 205: Reset Content` only clears forms).

evap/evaluation/management/commands/send_reminders.py

@@ -20,7 +20,7 @@ def get_sorted_evaluation_url_tuples_with_urgent_review() -> List[Tuple[Evaluati
             settings.PAGE_URL
             + reverse(
                 "staff:evaluation_textanswers",
-                kwargs={"semester_id": evaluation.course.semester.id, "evaluation_id": evaluation.id},
+                kwargs={"evaluation_id": evaluation.id},
             ),
         )
         for evaluation in Evaluation.objects.filter(state=Evaluation.State.EVALUATED)

evap/evaluation/tests/test_commands.py

@@ -353,7 +353,7 @@ def test_send_text_answer_review_reminder(self):
             [
                 (
                     evaluation,
-                    f"{settings.PAGE_URL}/staff/semester/{evaluation.course.semester.id}/evaluation/{evaluation.id}/textanswers",
+                    f"{settings.PAGE_URL}/staff/evaluation/{evaluation.id}/textanswers",
                 )
             ],
         )

evap/grades/templates/grades_course_base.html

@@ -8,7 +8,7 @@
         {% if disable_breadcrumb_course %}
             <li class="breadcrumb-item">{{ course.name }}</li>
         {% else %}
-            <li class="breadcrumb-item"><a href="{% url 'grades:course_view' semester.id course.id %}">{{ course.name }}</a></li>
+            <li class="breadcrumb-item"><a href="{% url 'grades:course_view' course.id %}">{{ course.name }}</a></li>
         {% endif %}
     {% endif %}
 {% endblock %}

evap/grades/templates/grades_course_view.html

@@ -29,7 +29,7 @@ <h3 class="mb-3">{{ course.name }} ({{ semester.name }})</h3>
                                 <td>
                                     <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' semester.id course.id 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>
+                                        <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' %}">
                                             <span class="fas fa-trash"></span>
                                         </button>
@@ -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' semester.id course.id %}" class="btn btn-dark {{ disable_if_archived }}">{% trans 'Upload new midterm grades' %}</a>
-        <a href="{% url 'grades:upload_grades' semester.id 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 {{ 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>
     {% endif %}
 {% endblock %}
 

evap/grades/templates/grades_semester_view.html

@@ -40,7 +40,7 @@ <h3 class="col-9 mb-0">
                         {% for course, num_midterm_grades, num_final_grades in courses %}
                             <tr>
                                 <td data-col="name" data-order="{{ course.name|lower }}">
-                                    <a href="{% url 'grades:course_view' semester.id course.id %}">{{ course.name }}</a>
+                                    <a href="{% url 'grades:course_view' course.id %}">{{ course.name }}</a>
                                     <br />
                                     {% include 'course_badges.html' %}
                                 </td>
@@ -73,12 +73,12 @@ <h3 class="col-9 mb-0">
                                                     <span class="fas fa-upload"></span>
                                                 </button>
                                                 <div class="dropdown-menu" aria-labelledby="btnUpload{{ course.id }}">
-                                                    <a class="dropdown-item" href="{% url 'grades:upload_grades' semester.id course.id %}">{% trans 'Midterm grades' %}</a>
-                                                    <a class="dropdown-item" href="{% url 'grades:upload_grades' semester.id course.id %}?final=true">{% trans 'Final grades' %}</a>
+                                                    <a class="dropdown-item" href="{% url 'grades:upload_grades' course.id %}">{% trans 'Midterm grades' %}</a>
+                                                    <a class="dropdown-item" href="{% url 'grades:upload_grades' course.id %}?final=true">{% trans 'Final grades' %}</a>
                                                 </div>
                                             </div>
                                         {% else %}
-                                            <a href="{% url 'grades:course_view' semester.id 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 {{ disable_if_archived }}" data-bs-toggle="tooltip" data-bs-placement="top" title="{% trans 'Change grade document' %}"><span class="fas fa-pencil"></span></a>
                                         {% endif %}
                                     {% endif %}
                                 </td>

evap/grades/tests.py

@@ -62,7 +62,7 @@ def helper_upload_grades(self, course, final_grades):
 
         final = "?final=true" if final_grades else ""
         response = self.app.post(
-            f"/grades/semester/{course.semester.id}/course/{course.id}/upload{final}",
+            f"{reverse('grades:upload_grades', args=[course.id])}{final}",
             params={"description_en": "Grades", "description_de": "Grades"},
             user=self.grade_publisher,
             content_type="multipart/form-data",
@@ -221,7 +221,7 @@ def setUpTestData(cls):
         cls.grade_publisher = make_grade_publisher()
 
         cls.test_users = [cls.grade_publisher]
-        cls.url = f"/grades/semester/{cls.semester.pk}/course/{cls.evaluation.course.pk}"
+        cls.url = reverse("grades:course_view", args=[cls.evaluation.course.pk])
 
     def test_403_on_archived_semester(self):
         self.semester.grade_documents_are_deleted = True
@@ -230,19 +230,33 @@ def test_403_on_archived_semester(self):
 
 
 class GradeEditTest(WebTest):
-    def test_grades_headlines(self):
-        grade_publisher = make_grade_publisher()
-        grade_document = baker.make(GradeDocument)
-
-        url = f"/grades/semester/{grade_document.course.semester.pk}/course/{grade_document.course.pk}/edit/{grade_document.pk}"
+    csrf_checks = False
 
-        response = self.app.get(url, user=grade_publisher)
+    @classmethod
+    def setUpTestData(cls) -> None:
+        cls.grade_publisher = make_grade_publisher()
+        cls.grade_document = baker.make(GradeDocument)
+        cls.url = reverse("grades:edit_grades", args=[cls.grade_document.pk])
+
+    def test_edit_grades(self) -> None:
+        previous_modifying_user = self.grade_document.last_modified_user
+        self.assertNotEqual(previous_modifying_user, self.grade_publisher)
+        response = self.app.get(self.url, user=self.grade_publisher)
+        form = response.forms[3]
+        form["description_en"] = "Absolutely final grades"
+        form["file"] = ("grades.txt", b"You did great!")
+        form.submit()
+        self.grade_document.refresh_from_db()
+        self.assertEqual(self.grade_document.last_modified_user, self.grade_publisher)
+
+    def test_grades_headlines(self) -> None:
+        response = self.app.get(self.url, user=self.grade_publisher)
         self.assertContains(response, "Upload midterm grades")
         self.assertNotContains(response, "Upload final grades")
 
-        grade_document.type = GradeDocument.Type.FINAL_GRADES
-        grade_document.save()
-        response = self.app.get(url, user=grade_publisher)
+        self.grade_document.type = GradeDocument.Type.FINAL_GRADES
+        self.grade_document.save()
+        response = self.app.get(self.url, user=self.grade_publisher)
         self.assertContains(response, "Upload final grades")
         self.assertNotContains(response, "Upload midterm grades")
 

evap/grades/urls.py

@@ -9,9 +9,9 @@
 
     path("download/<int:grade_document_id>", views.download_grades, name="download_grades"),
     path("semester/<int:semester_id>", views.semester_view, name="semester_view"),
-    path("semester/<int:semester_id>/course/<int:course_id>", views.course_view, name="course_view"),
-    path("semester/<int:semester_id>/course/<int:course_id>/upload", views.upload_grades, name="upload_grades"),
-    path("semester/<int:semester_id>/course/<int:course_id>/edit/<int:grade_document_id>", views.edit_grades, name="edit_grades"),
+    path("course/<int:course_id>", views.course_view, name="course_view"),
+    path("course/<int:course_id>/upload", views.upload_grades, name="upload_grades"),
+    path("grade_document/<int:grade_document_id>/edit", views.edit_grades, name="edit_grades"),
 
     path("delete_grades", views.delete_grades, name="delete_grades"),
     path("toggle_no_grades", views.toggle_no_grades, name="toggle_no_grades"),

evap/grades/views.py

@@ -65,11 +65,11 @@ def semester_view(request, semester_id):
 
 
 @grade_publisher_or_manager_required
-def course_view(request, semester_id, course_id):
-    semester = get_object_or_404(Semester, id=semester_id)
+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
-    course = get_object_or_404(Course, id=course_id, semester=semester)
 
     template_data = {
         "semester": semester,
@@ -95,11 +95,11 @@ def on_grading_process_finished(course):
 
 
 @grade_publisher_required
-def upload_grades(request, semester_id, course_id):
-    semester = get_object_or_404(Semester, id=semester_id)
+def upload_grades(request, course_id):
+    course = get_object_or_404(Course, id=course_id)
+    semester = course.semester
     if semester.grade_documents_are_deleted:
         raise PermissionDenied
-    course = get_object_or_404(Course, id=course_id, semester=semester)
 
     final_grades = request.GET.get("final") == "true"  # if parameter is not given, assume midterm grades
 
@@ -122,7 +122,7 @@ def upload_grades(request, semester_id, course_id):
             on_grading_process_finished(course)
 
         messages.success(request, _("Successfully uploaded grades."))
-        return redirect("grades:course_view", semester.id, course.id)
+        return redirect("grades:course_view", course.id)
 
     template_data = {
         "semester": semester,
@@ -161,12 +161,12 @@ def download_grades(request, grade_document_id):
 
 
 @grade_publisher_required
-def edit_grades(request, semester_id, course_id, grade_document_id):
-    semester = get_object_or_404(Semester, id=semester_id)
+def edit_grades(request, grade_document_id):
+    grade_document = get_object_or_404(GradeDocument, id=grade_document_id)
+    course = grade_document.course
+    semester = course.semester
     if semester.grade_documents_are_deleted:
         raise PermissionDenied
-    course = get_object_or_404(Course, id=course_id, semester=semester)
-    grade_document = get_object_or_404(GradeDocument, id=grade_document_id, course=course)
 
     form = GradeDocumentForm(request.POST or None, request.FILES or None, instance=grade_document)
 
@@ -177,7 +177,7 @@ def edit_grades(request, semester_id, course_id, grade_document_id):
     if form.is_valid():
         form.save(modifying_user=request.user)
         messages.success(request, _("Successfully updated grades."))
-        return redirect("grades:course_view", semester.id, course.id)
+        return redirect("grades:course_view", course.id)
 
     template_data = {
         "semester": semester,

evap/staff/templates/staff_course_base.html

@@ -15,9 +15,9 @@
         {% if disable_breadcrumb_course %}
             <li class="breadcrumb-item">{{ course.name }}</li>
         {% else %}
-            <li class="breadcrumb-item"><a href="{% url 'staff:course_edit' semester.id course.id %}">{{ course.name }}</a></li>
+            <li class="breadcrumb-item"><a href="{% url 'staff:course_edit' course.id %}">{{ course.name }}</a></li>
         {% endif %}
     {% elif evaluation.course %}
-        <li class="breadcrumb-item"><a href="{% url 'staff:course_edit' semester.id evaluation.course.id %}">{{ evaluation.course.name }}</a></li>
+        <li class="breadcrumb-item"><a href="{% url 'staff:course_edit' evaluation.course.id %}">{{ evaluation.course.name }}</a></li>
     {% endif %}
 {% endblock %}

evap/staff/templates/staff_course_form.html

@@ -26,7 +26,7 @@ <h5 class="card-title">{% trans 'Evaluations' %}</h5>
                         <ul>
                             {% for evaluation in course.evaluations.all %}
                                 <li>
-                                    <a href="{% url 'staff:evaluation_edit' semester.id evaluation.id %}">
+                                    <a href="{% url 'staff:evaluation_edit' evaluation.id %}">
                                         {{ evaluation.full_name }}
                                     </a>
                                 </li>

evap/staff/templates/staff_evaluation_person_management.html

@@ -106,7 +106,7 @@ <h6 class="card-subtitle mb-2 text-muted">{% trans 'From other evaluation' %}</h
 
     <div class="row">
         <div class="col">
-            <form id="login-key-export-form" method="POST" class="form-vertical" action="{% url 'staff:evaluation_login_key_export' semester.id evaluation.id %}">
+            <form id="login-key-export-form" method="POST" class="form-vertical" action="{% url 'staff:evaluation_login_key_export' evaluation.id %}">
                 {% csrf_token %}
                 <div class="card">
                     <div class="card-body">

evap/staff/templates/staff_evaluation_textanswer_edit.html

@@ -2,7 +2,7 @@
 
 {% block breadcrumb %}
     {{ block.super }}
-    <li class="breadcrumb-item"><a href="{% url 'staff:evaluation_textanswers' semester.id evaluation.id %}#{{ textanswer.id }}">{% trans 'Text answers' %}</a></li>
+    <li class="breadcrumb-item"><a href="{% url 'staff:evaluation_textanswers' evaluation.id %}#{{ textanswer.id }}">{% trans 'Text answers' %}</a></li>
     <li class="breadcrumb-item">{{ textanswer.id }}</li>
 {% endblock %}
 

evap/staff/templates/staff_evaluation_textanswers.html

@@ -15,13 +15,13 @@ <h3 class="me-auto">{{ evaluation.full_name }} ({{ evaluation.course.semester.na
         <div class="btn-switch btn-switch-light my-auto d-print-none">
             <div class="btn-switch-label">{% trans 'View' %}</div>
             <div class="btn-switch btn-group">
-                <a href="{% url 'staff:evaluation_textanswers' semester.id evaluation.id %}" role="button" class="btn btn-sm btn-light{% if view == 'quick' %} active{% endif %}">
+                <a href="{% url 'staff:evaluation_textanswers' evaluation.id %}" role="button" class="btn btn-sm btn-light{% if view == 'quick' %} active{% endif %}">
                     {% trans 'Quick' %}
                 </a>
-                <a href="{% url 'staff:evaluation_textanswers' semester.id evaluation.id %}?view=full" role="button" class="btn btn-sm btn-light{% if view == 'full' %} active{% endif %}">
+                <a href="{% url 'staff:evaluation_textanswers' evaluation.id %}?view=full" role="button" class="btn btn-sm btn-light{% if view == 'full' %} active{% endif %}">
                     {% trans 'Full' %}
                 </a>
-                <a href="{% url 'staff:evaluation_textanswers' semester.id evaluation.id %}?view=undecided" role="button" class="btn btn-sm btn-light{% if view == 'undecided' %} active{% endif %}">
+                <a href="{% url 'staff:evaluation_textanswers' evaluation.id %}?view=undecided" role="button" class="btn btn-sm btn-light{% if view == 'undecided' %} active{% endif %}">
                     {% trans 'Undecided' %}
                 </a>
             </div>

evap/staff/templates/staff_evaluation_textanswers_quick.html

@@ -137,7 +137,7 @@ <h5 class="modal-title" id="hotkeys-modal-title">Hotkeys</h5>
                 <div class="lcr-center d-none" data-action-set="summary">
                     {% if next_evaluations %}
                         {% for next_evaluation in next_evaluations %}
-                            <a href="{% url 'staff:evaluation_textanswers' next_evaluation.course.semester.id next_evaluation.id %}" data-next-evaluation-index="{{ forloop.counter0 }}" data-evaluation="{{ next_evaluation.pk }}" data-url="next-evaluation" class="btn btn-sm btn-primary">
+                            <a href="{% url 'staff:evaluation_textanswers' next_evaluation.id %}" data-next-evaluation-index="{{ forloop.counter0 }}" data-evaluation="{{ next_evaluation.pk }}" data-url="next-evaluation" class="btn btn-sm btn-primary">
                                 {% trans 'Review next evaluation' %}
                             </a>
                         {% endfor %}

evap/staff/templates/staff_semester_preparation_reminder.html

@@ -49,7 +49,7 @@
                                     {% with approval_state_values=evaluation.state|approval_state_values %}
                                         <span class="{{ approval_state_values.icon }}" data-bs-toggle="tooltip" data-bs-placement="top" title="{{ approval_state_values.description }}"></span>
                                     {% endwith %}
-                                    <a href="{% url 'staff:evaluation_edit' semester.id evaluation.id %}">{{ evaluation.full_name }}</a><br />
+                                    <a href="{% url 'staff:evaluation_edit' evaluation.id %}">{{ evaluation.full_name }}</a><br />
                                     {% include 'evaluation_badges.html' with mode='manager' %}
                                 </th>
                                 <td>{% include 'staff_evaluation_evaluation_period.html' with state=evaluation.state start_only=True %}</td>