Merge pull request #1256 from django-helpdesk/fix_ticket_update_page_craash

Fix ticket update page craash when uploading files with invalid extension

helpdesk/forms.py

@@ -424,7 +424,7 @@ class AbstractTicketForm(CustomFieldMixin, forms.Form):
 
 class TicketForm(AbstractTicketForm):
     """
-    Ticket Form creation for registered users.
+    Ticket Form for registered users.
     """
 
     submitter_email = forms.EmailField(
@@ -451,15 +451,20 @@ class TicketForm(AbstractTicketForm):
         choices=(),
     )
 
-    def __init__(self, *args, **kwargs):
+    def __init__(
+        self, instance=None, queue_choices=None, body_reqd=True, *args, **kwargs
+    ):
         """
         Add any custom fields that are defined to the form.
+        The view will have injected extra kwargs into the form init
+        by calling the views get_form_kwargs() which must be removed before
+        calling super() because the django.forms.forms.BaseForm only
+        supports specific kwargs and so will crash and burn if they are left in
         """
-        queue_choices = kwargs.pop("queue_choices")
-
         super().__init__(*args, **kwargs)
-
-        self.fields["queue"].choices = queue_choices
+        if queue_choices:
+            self.fields["queue"].choices = queue_choices
+        self.fields["body"].required = body_reqd
         if helpdesk_settings.HELPDESK_STAFF_ONLY_TICKET_OWNERS:
             assignable_users = User.objects.filter(
                 is_active=True, is_staff=True

helpdesk/templates/helpdesk/ticket.html

@@ -18,6 +18,9 @@
 {% endblock %}
 
 {% block helpdesk_body %}
+	{% if form.errors %}
+	    {% include 'helpdesk/include/alert_form_errors.html' %}
+	{% endif %}
     {% if helpdesk_settings.HELPDESK_TRANSLATE_TICKET_COMMENTS %}
         <div id="google_translate_element"></div>
         <script src="//translate.google.com/translate_a/element.js?cb=googleTranslateElementInit"></script>
@@ -95,7 +98,7 @@
             <div class="card-header">{% trans "Respond to this ticket" %}</div>
             <div class="card-body">
 
-        <form method='post' action='update/' enctype='multipart/form-data'>
+         <form method="post" action="{% url 'helpdesk:update' ticket.id %}" enctype="multipart/form-data">
 
         <fieldset>
             <dl>
@@ -105,8 +108,10 @@
                 <dd class='form_help_text'>{% trans "Selecting a pre-set reply will over-write your comment below. You can then modify the pre-set reply to your liking before saving this update." %}</dd>
                 {% endif %}
 
-                <dt><label for='commentBox'>{% trans "Comment / Resolution" %}</label></dt>
-                <dd><textarea rows='8' cols='70' name='comment' id='commentBox'></textarea></dd>
+                <dt>
+                    <label for='commentBox'>{% trans "Comment / Resolution" %}</label>
+                </dt>
+                <dd><textarea rows='8' cols='70' name='comment' id='commentBox'>{% if form.errors %}{{ xform.comment }}{% endif %}</textarea></dd>
                 {% url "helpdesk:help_context" as context_help_url %}
                 {% blocktrans %}
                 <dd class='form_help_text'>You can insert ticket and queue details in your message. For more information, see the <a href='{{ context_help_url }}'>context help page</a>.</dd>
@@ -135,7 +140,7 @@
                 <dt>
                     <label for='id_time_spent'>{% trans "Time spent" %}</label> <span class='form_optional'>{% trans "(Optional)" %}</span>
                 </dt>
-                <dd><input name='time_spent' type="time" /></dd>
+                <dd><input name='time_spent' type="time" value="{% if form.errors %}{{ xform.time_spent }}{% endif %}"/></dd>
                 {% endif %}
                 {% endif %}
             </dl>
@@ -204,6 +209,9 @@
         {% endif %}
 
         {% if helpdesk_settings.HELPDESK_ENABLE_ATTACHMENTS %}
+            {% if form.errors.attachment %}
+                    <small class='error'>{{ form.errors.attachment }}</small>
+            {% endif %}
         <p id='ShowFileUploadPara'><button type="button" class="btn btn-warning btn-sm" id='ShowFileUpload'>{% trans "Attach File(s) &raquo;" %}</button></p>
         {% endif %}
 
@@ -211,7 +219,7 @@
 
             <dl>
                 <dt><label for='id_file'>{% trans "Attach a File" %}</label></dt>
-                <dd>
+               <dd>
                     <div class="add_file_fields_wrap">
                         <button class="add_file_field_button btn btn-success btn-xs">{% trans "Add Another File" %}</button>
                         <div><label class='btn btn-primary btn-sm btn-file'>

helpdesk/tests/test_attachments.py

@@ -10,6 +10,7 @@ import shutil
 from tempfile import gettempdir
 from unittest import mock
 from unittest.case import skip
+from django.contrib.auth import get_user_model
 
 
 MEDIA_DIR = os.path.join(gettempdir(), "helpdesk_test_media")
@@ -92,6 +93,89 @@ class AttachmentIntegrationTests(TestCase):
         self.assertEqual(disk_content, "โจ")
 
 
+@override_settings(MEDIA_ROOT=MEDIA_DIR)
+class AttachmentIntegrationStaffTests(TestCase):
+    def setUp(self):
+        self.ticket = models.Ticket.objects.create(
+            queue=models.Queue.objects.create(),
+            title="Test attachments via ticket update",
+        )
+        self.default_update_post_data = {
+            "queue": self.ticket.queue_id,
+            "title": self.ticket.title,
+            "priority": self.ticket.priority,
+        }
+
+    def loginUser(self, is_staff=True):
+        """Create a staff user and login"""
+        User = get_user_model()
+        self.user = User.objects.create(
+            username="User_1",
+            is_staff=is_staff,
+        )
+        self.user.set_password("pass")
+        self.user.save()
+        self.client.login(username="User_1", password="pass")
+
+    def test_update_ticket_with_attachment_valid_extension(self):
+        self.loginUser(is_staff=True)
+        file_content = "staff attached file content"
+        test_file = SimpleUploadedFile(
+            "test_staff_att.txt", bytes(file_content, "utf-8"), "text/plain"
+        )
+        post_data = {
+            "attachment": test_file,
+            **self.default_update_post_data,
+        }
+        # Ensure ticket form submits with attachment successfully
+        self.client.post(
+            reverse(
+                "helpdesk:update",
+                kwargs={"ticket_id": self.ticket.id},
+            ),
+            post_data,
+            follow=True,
+        )
+        # Ensure attachment is available with correct content
+        att = models.FollowUpAttachment.objects.get(followup__ticket=self.ticket)
+        with open(os.path.join(MEDIA_DIR, att.file.name)) as file_on_disk:
+            disk_content = file_on_disk.read()
+        self.assertEqual(disk_content, file_content)
+
+    def test_update_ticket_with_attachment_invalid_extension(self):
+        self.loginUser(is_staff=True)
+        file_content = "staff attached file content with invalid extension"
+        file_extension = ".crash"
+        test_file = SimpleUploadedFile(
+            f"test_staff_att{file_extension}",
+            bytes(file_content, "utf-8"),
+            "text/plain",
+        )
+        post_data = {
+            "attachment": test_file,
+            **self.default_update_post_data,
+        }
+        # Ensure ticket form submits with attachment successfully
+        response = self.client.post(
+            reverse(
+                "helpdesk:update",
+                kwargs={"ticket_id": self.ticket.id},
+            ),
+            post_data,
+            follow=True,
+        )
+        error_msg = response.context_data["form"].errors["attachment"][0]
+        self.assertTrue(
+            file_extension in error_msg,
+            "Response indicates there were no errors attaching illegal file extension",
+        )
+        # Ensure attachment is not uploaded
+        has_att = models.FollowUpAttachment.objects.filter(
+            followup__ticket=self.ticket
+        ).exists()
+        self.assertFalse(has_att, "File was attached with invalid extension")
+
+
 @mock.patch.object(models.FollowUp, "save", autospec=True)
 @mock.patch.object(models.FollowUpAttachment, "save", autospec=True)
 @mock.patch.object(models.Ticket, "save", autospec=True)

helpdesk/tests/test_checklist.py

@@ -17,8 +17,13 @@ class TicketChecklistTestCase(TestCase):
         self.client.login(username="User", password="pass")
 
         self.ticket = Ticket.objects.create(
-            queue=Queue.objects.create(title="Queue", slug="queue")
+            queue=Queue.objects.create(title="Queue", slug="queue"), title="Test Queue"
         )
+        self.default_update_post_data = {
+            "queue": self.ticket.queue_id,
+            "title": self.ticket.title,
+            "priority": self.ticket.priority,
+        }
 
     def test_create_checklist(self):
         self.assertEqual(self.ticket.checklists.count(), 0)
@@ -141,7 +146,10 @@ class TicketChecklistTestCase(TestCase):
 
         response = self.client.post(
             reverse("helpdesk:update", kwargs={"ticket_id": self.ticket.id}),
-            data={f"checklist-{checklist.id}": task.id},
+            data={
+                f"checklist-{checklist.id}": task.id,
+                **self.default_update_post_data,
+            },
             follow=True,
         )
         self.assertEqual(response.status_code, 200)
@@ -167,6 +175,7 @@ class TicketChecklistTestCase(TestCase):
 
         response = self.client.post(
             reverse("helpdesk:update", kwargs={"ticket_id": self.ticket.id}),
+            data={**self.default_update_post_data},
             follow=True,
         )
         self.assertEqual(response.status_code, 200)

helpdesk/tests/test_navigation.py

@@ -279,7 +279,7 @@ class ReturnToTicketTestCase(TestCase):
 
         user = get_staff_user()
         ticket = create_ticket()
-        response = return_to_ticket(user, helpdesk_settings, ticket)
+        response = return_to_ticket(user, ticket)
         self.assertEqual(response["location"], ticket.get_absolute_url())
 
     def test_non_staff_user(self):
@@ -291,5 +291,5 @@ class ReturnToTicketTestCase(TestCase):
             email="wensleydale@example.com",
         )
         ticket = create_ticket()
-        response = return_to_ticket(user, helpdesk_settings, ticket)
+        response = return_to_ticket(user, ticket)
         self.assertEqual(response["location"], ticket.ticket_url)

helpdesk/tests/test_ticket_actions.py

@@ -119,9 +119,15 @@ class TicketActionsTestCase(TestCase):
         ticket = Ticket.objects.create(**initial_data)
         ticket_id = ticket.id
 
+        default_post_data = {
+            "title": ticket.title,
+            "priority": ticket.priority,
+            "queue": ticket.queue_id,
+        }
         # assign new owner
         post_data = {
             "owner": self.user2.id,
+            **default_post_data,
         }
         response = self.client.post(
             reverse("helpdesk:update", kwargs={"ticket_id": ticket_id}),
@@ -139,7 +145,11 @@ class TicketActionsTestCase(TestCase):
         self.user2.save()
         self.user.email = "user1@test.com"
         self.user.save()
-        post_data = {"new_status": Ticket.CLOSED_STATUS, "public": True}
+        post_data = {
+            "new_status": Ticket.CLOSED_STATUS,
+            "public": True,
+            **default_post_data,
+        }
 
         # do this also to a newly assigned user (different from logged in one)
         ticket.assigned_to = self.user
@@ -153,6 +163,7 @@ class TicketActionsTestCase(TestCase):
             "new_status": Ticket.OPEN_STATUS,
             "owner": self.user2.id,
             "public": True,
+            **default_post_data,
         }
         response = self.client.post(
             reverse("helpdesk:update", kwargs={"ticket_id": ticket_id}),
@@ -363,6 +374,8 @@ class TicketActionsTestCase(TestCase):
             slug="newqueue",
         )
         post_data = {
+            "title": ticket.title,
+            "priority": ticket.priority,
             "comment": "first follow-up in new queue",
             "queue": str(new_queue.id),
         }

helpdesk/tests/test_time_spent_auto.py

@@ -416,6 +416,7 @@ class TimeSpentAutoTestCase(TestCase):
             "created": datetime.strptime(
                 "2024-04-09T08:00:00+00:00", "%Y-%m-%dT%H:%M:%S%z"
             ),
+            "description": "Followup time spent auto exclude queues",
         }
         ticket = Ticket.objects.create(**initial_data)
 
@@ -427,6 +428,8 @@ class TimeSpentAutoTestCase(TestCase):
             post_data = {
                 "comment": "ticket in queue {}".format(queue),
                 "queue": queues[queue].id,
+                "title": ticket.title,
+                "priority": ticket.priority,
             }
             self.client.post(
                 reverse("helpdesk:update", kwargs={"ticket_id": ticket.id}), post_data

helpdesk/views/staff.py

@@ -11,7 +11,7 @@ from ..lib import format_time_spent
 from ..templated_email import send_templated_mail
 from collections import defaultdict
 from copy import deepcopy
-from datetime import datetime, time, timedelta
+from datetime import datetime, timedelta
 from django.conf import settings
 from django.contrib.auth import get_user_model
 from django.contrib.auth.decorators import user_passes_test
@@ -21,14 +21,13 @@ from django.core.exceptions import PermissionDenied
 from django.core.handlers.wsgi import WSGIRequest
 from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
 from django.db.models import Q, Case, When
+from django.db.models.query import QuerySet
 from django.forms import HiddenInput, inlineformset_factory, TextInput
 from django.http import Http404, HttpResponse, HttpResponseRedirect, JsonResponse
 from django.shortcuts import get_object_or_404, redirect, render
 from django.urls import reverse, reverse_lazy
 from django.utils import timezone
-from django.utils.dateparse import parse_date, parse_datetime
 from django.utils.html import escape
-from django.utils.timezone import make_aware
 from django.utils.translation import gettext as _
 from django.views.decorators.csrf import requires_csrf_token
 from django.views.generic.edit import FormView, UpdateView
@@ -119,7 +118,6 @@ def _get_queue_choices(queues):
     idea is to return only one choice if there is only one queue or add empty
     choice at the beginning of the list, if there are more queues
     """
-
     queue_choices = []
     if len(queues) > 1:
         queue_choices = [("", "--------")]
@@ -127,6 +125,29 @@ def _get_queue_choices(queues):
     return queue_choices
 
 
+def get_active_users() -> QuerySet:
+    if helpdesk_settings.HELPDESK_STAFF_ONLY_TICKET_OWNERS:
+        users = User.objects.filter(is_active=True, is_staff=True).order_by(
+            User.USERNAME_FIELD
+        )
+    else:
+        users = User.objects.filter(is_active=True).order_by(User.USERNAME_FIELD)
+    return users
+
+
+def get_user_queues(user) -> dict[str, str]:
+    queues = HelpdeskUser(user).get_queues()
+    return _get_queue_choices(queues)
+
+
+def get_form_extra_kwargs(user) -> dict[str, object]:
+    return {
+        "active_users": get_active_users(),
+        "queues": get_user_queues(user),
+        "priorities": Ticket.PRIORITY_CHOICES,
+    }
+
+
 @helpdesk_staff_member_required
 def dashboard(request):
     """
@@ -381,7 +402,7 @@ def view_ticket(request, ticket_id):
 
     if "take" in request.GET:
         update_ticket(request.user, ticket, owner=request.user.id)
-        return return_to_ticket(request.user, helpdesk_settings, ticket)
+        return return_to_ticket(request.user, ticket)
 
     if "subscribe" in request.GET:
         # Allow the user to subscribe him/herself to the ticket whilst viewing
@@ -406,22 +427,13 @@ def view_ticket(request, ticket_id):
             owner=owner,
             comment=_("Accepted resolution and closed ticket"),
         )
-        return return_to_ticket(request.user, helpdesk_settings, ticket)
+        return return_to_ticket(request.user, ticket)
 
-    if helpdesk_settings.HELPDESK_STAFF_ONLY_TICKET_OWNERS:
-        users = User.objects.filter(is_active=True, is_staff=True).order_by(
-            User.USERNAME_FIELD
-        )
-    else:
-        users = User.objects.filter(is_active=True).order_by(User.USERNAME_FIELD)
-
-    queues = HelpdeskUser(request.user).get_queues()
-    queue_choices = _get_queue_choices(queues)
-    # TODO: shouldn't this template get a form to begin with?
+    extra_context_kwargs = get_form_extra_kwargs(request.user)
     form = TicketForm(
-        initial={"due_date": ticket.due_date}, queue_choices=queue_choices
+        initial={"due_date": ticket.due_date},
+        queue_choices=extra_context_kwargs["queues"],
     )
-
     ticketcc_string, show_subscribe = return_ticketccstring_and_show_subscribe(
         request.user, ticket
     )
@@ -467,9 +479,6 @@ def view_ticket(request, ticket_id):
             "dependencies": dependencies,
             "submitter_userprofile_url": submitter_userprofile_url,
             "form": form,
-            "active_users": users,
-            "priorities": Ticket.PRIORITY_CHOICES,
-            "queues": queue_choices,
             "preset_replies": PreSetReply.objects.filter(
                 Q(queues=ticket.queue) | Q(queues__isnull=True)
             ),
@@ -477,6 +486,7 @@ def view_ticket(request, ticket_id):
             "SHOW_SUBSCRIBE": show_subscribe,
             "checklist_form": checklist_form,
             "customfields_form": customfields_form,
+            **extra_context_kwargs,
         },
     )
 
@@ -571,23 +581,17 @@ def get_ticket_from_request_with_authorisation(
     return get_object_or_404(Ticket, id=ticket_id)
 
 
-def get_due_date_from_request_or_ticket(
-    request: WSGIRequest, ticket: Ticket
+def get_due_date_from_form_or_ticket(
+    form, ticket: Ticket
 ) -> typing.Optional[datetime.date]:
-    """Tries to locate the due date for a ticket from the `request.POST`
+    """Tries to locate the due date for a ticket from the form
     'due_date' parameter or the `due_date_*` paramaters.
     """
-    due_date = request.POST.get("due_date", None) or None
-
-    if due_date is not None:
-        parsed_date = parse_datetime(due_date) or datetime.combine(
-            parse_date(due_date), time()
-        )
-        due_date = make_aware(parsed_date)
-    else:
-        due_date_year = int(request.POST.get("due_date_year", 0))
-        due_date_month = int(request.POST.get("due_date_month", 0))
-        due_date_day = int(request.POST.get("due_date_day", 0))
+    due_date = form.cleaned_data.get("due_date") or None
+    if due_date is None:
+        due_date_year = int(form.cleaned_data.get("due_date_year", 0))
+        due_date_month = int(form.cleaned_data.get("due_date_month", 0))
+        due_date_day = int(form.cleaned_data.get("due_date_day", 0))
         # old way, probably deprecated?
         if not (due_date_year and due_date_month and due_date_day):
             due_date = ticket.due_date
@@ -602,36 +606,38 @@ def get_due_date_from_request_or_ticket(
     return due_date
 
 
-def get_time_spent_from_request(request: WSGIRequest) -> typing.Optional[timedelta]:
-    if request.POST.get("time_spent"):
-        (hours, minutes) = [int(f) for f in request.POST.get("time_spent").split(":")]
+def get_time_spent_from_form(form: dict) -> typing.Optional[timedelta]:
+    if form.data.get("time_spent"):
+        (hours, minutes) = [int(f) for f in form.data.get("time_spent").split(":")]
         return timedelta(hours=hours, minutes=minutes)
     return None
 
 
-def update_ticket_view(request, ticket_id, public=False):
-    try:
-        ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public)
-    except PermissionDenied:
-        return redirect_to_login(request.path, "helpdesk:login")
+def update_ticket_view(request, ticket_id, *args, **kwargs):
+    return UpdateTicketView.as_view()(request, *args, ticket_id=ticket_id, **kwargs)
 
-    comment = request.POST.get("comment", "")
-    new_status = int(request.POST.get("new_status", ticket.status))
-    title = request.POST.get("title", ticket.title)
-    owner = int(request.POST.get("owner", -1))
-    priority = int(request.POST.get("priority", ticket.priority))
-    queue = int(request.POST.get("queue", ticket.queue.id))
+
+def save_ticket_update(form, ticket, user):
+    comment = form.data.get("comment", "")
+    new_status = int(form.data.get("new_status", ticket.status))
+    title = form.cleaned_data.get("title", ticket.title)
+    owner = int(form.data.get("owner", -1))
+    priority = int(form.cleaned_data.get("priority", ticket.priority))
+    queue = int(form.cleaned_data.get("queue", ticket.queue.id))
 
     # custom fields
-    customfields_form = EditTicketCustomFieldForm(request.POST or None, instance=ticket)
+    customfields_form = EditTicketCustomFieldForm(
+        form.cleaned_data or None, instance=ticket
+    )
 
     # Check if a change happened on checklists
     new_checklists = {}
     changes_in_checklists = False
     for checklist in ticket.checklists.all():
         old_completed = set(checklist.tasks.completed().values_list("id", flat=True))
+        # Checklists will not be in the cleaned_data so access the submitted data
         new_checklist = set(
-            map(int, request.POST.getlist(f"checklist-{checklist.id}", []))
+            map(int, form.data.getlist(f"checklist-{checklist.id}", []))
         )
         new_checklists[checklist.id] = new_checklist
         if new_checklist != old_completed:
@@ -640,10 +646,10 @@ def update_ticket_view(request, ticket_id, public=False):
     # NOTE: jQuery's default for dates is mm/dd/yy
     # very US-centric but for now that's the only format supported
     # until we clean up code to internationalize a little more
-    due_date = get_due_date_from_request_or_ticket(request, ticket)
+    due_date = get_due_date_from_form_or_ticket(form, ticket)
     no_changes = all(
         [
-            not request.FILES,
+            not form.files,
             not comment,
             not changes_in_checklists,
             new_status == ticket.status,
@@ -658,29 +664,29 @@ def update_ticket_view(request, ticket_id, public=False):
         ]
     )
     if no_changes:
-        return return_to_ticket(request.user, helpdesk_settings, ticket)
+        return ticket
 
     update_ticket(
-        request.user,
+        user,
         ticket,
         title=title,
         comment=comment,
-        files=request.FILES.getlist("attachment"),
-        public=request.POST.get("public", False),
-        owner=int(request.POST.get("owner", -1)),
-        priority=int(request.POST.get("priority", -1)),
-        queue=int(request.POST.get("queue", -1)),
+        files=form.files.getlist("attachment"),
+        public=form.data.get("public", False),
+        owner=owner or -1,
+        priority=priority or -1,
+        queue=queue or -1,
         new_status=new_status,
-        time_spent=get_time_spent_from_request(request),
-        due_date=get_due_date_from_request_or_ticket(request, ticket),
+        time_spent=get_time_spent_from_form(form),
+        due_date=due_date,
         new_checklists=new_checklists,
         customfields_form=customfields_form,
     )
 
-    return return_to_ticket(request.user, helpdesk_settings, ticket)
+    return ticket
 
 
-def return_to_ticket(user, helpdesk_settings, ticket):
+def return_to_ticket(user, ticket):
     """Helper function for update_ticket"""
 
     if is_helpdesk_staff(user):
@@ -1277,8 +1283,7 @@ class CreateTicketView(
 
     def get_form_kwargs(self):
         kwargs = super().get_form_kwargs()
-        queues = HelpdeskUser(self.request.user).get_queues()
-        kwargs["queue_choices"] = _get_queue_choices(queues)
+        kwargs["queue_choices"] = get_user_queues(self.request.user)
         return kwargs
 
     def form_valid(self, form):
@@ -1295,6 +1300,56 @@ class CreateTicketView(
             return reverse("helpdesk:dashboard")
 
 
+class UpdateTicketView(
+    MustBeStaffMixin, abstract_views.AbstractCreateTicketMixin, UpdateView
+):
+    template_name = "helpdesk/ticket.html"
+    form_class = TicketForm
+
+    def get_initial(self):
+        initial_data = super().get_initial()
+        return initial_data
+
+    def get_context_data(self, **kwargs):
+        """Insert view context that would be lost after a POST."""
+        extra = get_form_extra_kwargs(self.request.user)
+        kwargs.update(extra)
+        # Copy all data submitted that is not in the forms defined fields
+        form_fields = kwargs["form"].base_fields
+        all_fields = kwargs["form"].data
+        self.extra_context = {
+            "xform": {
+                k: v
+                for k, v in all_fields.items()
+                if k != "csrfmiddlewaretoken" and k not in form_fields
+            }
+        }
+        return super().get_context_data(**kwargs)
+
+    def get_form_kwargs(self):
+        kwargs = super().get_form_kwargs()
+        # The ModelFormMixin adds "instance" which is then a problem in the
+        kwargs["queue_choices"] = get_user_queues(self.request.user)
+        kwargs["body_reqd"] = False
+        return kwargs
+
+    def get_object(self, queryset=None):
+        ticket_id = self.kwargs["ticket_id"]
+        return Ticket.objects.get(id=ticket_id)
+
+    def form_valid(self, form):
+        ticket_id = self.kwargs["ticket_id"]
+        try:
+            self.ticket = get_ticket_from_request_with_authorisation(
+                self.request, ticket_id, False
+            )
+        except PermissionDenied:
+            return redirect_to_login(self.request.path, "helpdesk:login")
+        # Avoid calling super as it will call the save() method on the form
+        save_ticket_update(form, self.ticket, self.request.user)
+        return return_to_ticket(self.request.user, self.ticket)
+
+
 @helpdesk_staff_member_required
 def raw_details(request, type_):
     # TODO: This currently only supports spewing out 'PreSetReply' objects,