Clean up ticket url handling

* Move saving ticket url and making change event to incident method
* Update bulk ticket url to make change events
* Refactor autoticket plugin-loading and client creation into utility functions

changelog.d/+improve-ticket-url-handling.changed.md

@@ -0,0 +1,6 @@
+Refactored ticket creation code so the actual changing of the incident happens
+only in one place. Also moved the actual autocreation magic to utility
+functions (sans error-handling since that is response-type dependent). Made
+bulk changes of tickets actually create the ChangeEvents so that it behaves
+like other bulk actions and make it possible to get notified of changed ticket
+urls.

src/argus/dev/management/commands/bulk_incidents.py

@@ -123,16 +123,20 @@ def handle(self, *args, **options):
 
         incident_qs = Incident.objects.filter(pk__in=incident_pks)
 
+        timestamp = options.get("timestamp") or None
+        description = options.get("description") or ""
+
         if action == "ticket_url":
             url = options.get("url") or None
             if not url:
                 self.stderr.write(self.style.WARNING("A ticket url is needed."))
                 return
 
-            incident_qs.update_ticket_url(url=url)
-
-        timestamp = options.get("timestamp") or None
-        description = options.get("description") or ""
+            incident_qs.update_ticket_url(
+                actor=actor,
+                url=url,
+                timestamp=timestamp,
+            )
 
         if action == "ack":
             expiration = options.get("expiration") or None

src/argus/incident/models.py

@@ -349,9 +349,12 @@ def reopen(self, actor: User, timestamp=None, description=""):
         events = qs.create_events(actor, event_type, timestamp, description)
         return events
 
-    def update_ticket_url(self, url: str):
-        self.update(ticket_url=url)
-        return self.all()  # Return updated qs
+    def update_ticket_url(self, actor: User, url: str, timestamp=None):
+        events = set()
+        for incident in self:
+            event = incident.change_ticket_url(actor, url, timestamp)
+            events.add(event.pk)
+        return self.all()
 
 
 # TODO: review whether fields should be nullable, and on_delete modes
@@ -555,6 +558,14 @@ def change_level(self, actor, new_level, timestamp=None):
         event = ChangeEvent.change_level(self, actor, new_level, timestamp)
         return event
 
+    # @transaction.atomic
+    def change_ticket_url(self, actor, url="", timestamp=None):
+        old_ticket_url = self.ticket_url
+        self.ticket_url = url
+        self.save(update_fields=["ticket_url"])
+        event = ChangeEvent.change_ticket_url(self, actor, old_ticket_url, url, timestamp)
+        return event
+
     def pp_details_url(self):
         "Merge Incident.details_url with Source.base_url"
         path = self.details_url.strip()
@@ -618,6 +629,14 @@ def change_level(cls, incident, actor, new_level, timestamp=None):
         event.save()
         return event
 
+    @classmethod
+    def change_ticket_url(cls, incident, actor, old_ticket="", new_ticket="", timestamp=None):
+        timestamp = timestamp if timestamp else timezone.now()
+        description = cls.format_description("ticket_url", old_ticket, new_ticket)
+        event = cls(incident=incident, actor=actor, timestamp=timestamp, description=description)
+        event.save()
+        return event
+
 
 class AcknowledgementQuerySet(models.QuerySet):
     def expired(self, timestamp=None):

src/argus/incident/ticket/utils.py

@@ -0,0 +1,59 @@
+from __future__ import annotations
+
+import logging
+from typing import TYPE_CHECKING
+from urllib.parse import urljoin
+
+from django.conf import settings
+
+from argus.incident.ticket.base import (
+    TicketSettingsException,
+)
+from argus.util.utils import import_class_from_dotted_path
+
+from ..serializers import IncidentSerializer
+
+if TYPE_CHECKING:
+    from argus.incident.models import Incident
+    from django.contrib.auth import get_user_model
+
+    User = get_user_model()
+
+
+LOG = logging.getLogger(__name__)
+SETTING_NAME = "TICKET_PLUGIN"
+
+
+__all__ = [
+    "SETTING_NAME",
+    "get_autocreate_ticket_plugin",
+    "serialize_incident_for_ticket_autocreation",
+]
+
+
+def get_autocreate_ticket_plugin():
+    plugin = getattr(settings, SETTING_NAME, None)
+
+    if not plugin:
+        raise TicketSettingsException(
+            f'No path to ticket plugin can be found in the settings. Please update the setting "{SETTING_NAME}".'
+        )
+
+    try:
+        ticket_class = import_class_from_dotted_path(plugin)
+    except Exception as e:
+        LOG.exception("Could not import ticket plugin from path %s", plugin)
+        raise TicketSettingsException(f"Ticket plugin is incorrectly configured: {e}")
+    else:
+        return ticket_class
+
+
+def serialize_incident_for_ticket_autocreation(incident: Incident, actor: User):
+    serialized_incident = IncidentSerializer(incident).data
+    # TODO: ensure argus_url ends with "/" on HTMx frontend
+    serialized_incident["argus_url"] = urljoin(
+        getattr(settings, "FRONTEND_URL", ""),
+        f"incidents/{incident.pk}",
+    )
+    serialized_incident["user"] = actor.get_full_name()
+    return serialized_incident

src/argus/incident/views.py

@@ -1,6 +1,5 @@
 import logging
 import secrets
-from urllib.parse import urljoin
 
 from django.conf import settings
 from django.db import IntegrityError
@@ -30,6 +29,10 @@
     TicketPluginException,
     TicketSettingsException,
 )
+from argus.incident.ticket.utils import (
+    get_autocreate_ticket_plugin,
+    serialize_incident_for_ticket_autocreation,
+)
 from argus.notificationprofile.media import (
     send_notifications_to_users,
     background_send_notification,
@@ -285,38 +288,23 @@ class TicketPluginViewSet(viewsets.ViewSet):
     def update(self, request, incident_pk=None):
         incident = get_object_or_404(self.queryset, pk=incident_pk)
 
+        # never overwrite existing url
         if incident.ticket_url:
             serializer = self.serializer_class(data={"ticket_url": incident.ticket_url})
             if serializer.is_valid():
                 return Response(serializer.data, status=status.HTTP_200_OK)
             return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
 
-        plugin = getattr(settings, "TICKET_PLUGIN", None)
-
-        if not plugin:
-            return Response(
-                data="No path to ticket plugin can be found in the settings. Please update the setting 'TICKET_PLUGIN'.",
-                status=status.HTTP_400_BAD_REQUEST,
-            )
-
         try:
-            ticket_class = import_class_from_dotted_path(plugin)
-        except Exception:
-            LOG.exception("Could not import ticket plugin from path %s", plugin)
-            return Response(
-                data="Ticket plugins are incorrectly configured.",
-                status=status.HTTP_400_BAD_REQUEST,
-            )
+            ticket_plugin = get_autocreate_ticket_plugin()
+        except TicketSettingsException as e:
+            # shouldn't this be a 500 Server Error?
+            return Response(data=str(e), status=status.HTTP_400_BAD_REQUEST)
 
-        serialized_incident = IncidentSerializer(incident).data
-        serialized_incident["argus_url"] = urljoin(
-            getattr(settings, "FRONTEND_URL", ""),
-            f"incidents/{incident_pk}",
-        )
-        serialized_incident["user"] = request.user.get_full_name()
+        serialized_incident = serialize_incident_for_ticket_autocreation(incident, request.user)
 
         try:
-            url = ticket_class.create_ticket(serialized_incident)
+            url = ticket_plugin.create_ticket(serialized_incident)
         except TicketSettingsException as e:
             return Response(
                 data=str(e),
@@ -339,12 +327,7 @@ def update(self, request, incident_pk=None):
             )
 
         if url:
-            description = ChangeEvent.format_description("ticket_url", "", url)
-            ChangeEvent.objects.create(
-                incident=incident, actor=request.user, timestamp=timezone.now(), description=description
-            )
-            incident.ticket_url = url
-            incident.save(update_fields=["ticket_url"])
+            incident.change_ticket_url(request.user, url, timezone.now())
             serializer = self.serializer_class(data={"ticket_url": incident.ticket_url})
             if serializer.is_valid():
                 return Response(serializer.data, status=status.HTTP_200_OK)
@@ -727,7 +710,7 @@ def create(self, request):
 
         qs, changes, status_codes_seen = self.bulk_setup(incident_ids)
 
-        incidents = qs.update_ticket_url(ticket_url)
+        incidents = qs.update_ticket_url(request.user, ticket_url, timestamp=timezone.now())
         for incident in incidents:
             changes[str(incident.id)] = {
                 "ticket_url": ticket_url,

tests/incident/test_queryset.py

@@ -105,7 +105,7 @@ def test_create_events(self):
     @tag("bulk")
     def test_update_ticket_url(self):
         qs = Incident.objects.filter(id__in=(self.incident1.id, self.incident2.id))
-        qs.update_ticket_url("http://vg.no")
+        qs.update_ticket_url(self.user, "http://vg.no")
         result = qs.has_ticket()
         self.assertEqual(set(result), set(qs.all()))