Fix: 404 on hyperlinks to Sector and AP objects (#636)

* Fix: 404 on hyperlinks to Sector and AP objects

* Typing fixes

* Add test for invalid device id

src/meshapi/admin/models/device.py

@@ -1,10 +1,13 @@
 import os
+from typing import Optional
 
 from django import forms
 from django.contrib import admin
+from django.contrib.admin.utils import unquote
 from django.contrib.postgres.search import SearchVector
 from django.db.models import QuerySet
-from django.http import HttpRequest
+from django.http import HttpRequest, HttpResponse, HttpResponseRedirect
+from django.shortcuts import redirect
 from import_export.admin import ExportActionMixin, ImportExportMixin
 from simple_history.admin import SimpleHistoryAdmin
 
@@ -13,6 +16,7 @@
 
 from ..inlines import DeviceLinkInline
 from ..ranked_search import RankedSearchMixin
+from ..utils import downclass_device, get_admin_url
 
 UISP_URL = os.environ.get("UISP_URL", "https://uisp.mesh.nycmesh.net/nms")
 
@@ -87,3 +91,28 @@ def get_queryset(self, request: HttpRequest) -> QuerySet[Device]:
             queryset = queryset.exclude(sector__isnull=False).exclude(accesspoint__isnull=False)
 
         return queryset
+
+    def _get_subtype_redirect(self, request: HttpRequest, object_id: str) -> Optional[HttpResponseRedirect]:
+        """Create a redirect for an AP or sector device by its object ID (if such a subtype exists)"""
+        device = Device.objects.filter(pk=object_id).first()
+        if not device:
+            return None
+
+        downclassed_model_obj = downclass_device(device)
+        target_url = get_admin_url(downclassed_model_obj, site_base_url=f"{request.scheme}://{request.get_host()}")
+        return redirect(target_url)
+
+    def _changeform_view(
+        self, request: HttpRequest, object_id: str, form_url: str, extra_context: dict
+    ) -> HttpResponse:
+        if object_id and not self.get_object(request, unquote(object_id), None):
+            # If the built-in object lookup logic doesn't find this device,
+            # it's probably because it's excluded in get_queryset() above
+            # (as a Sector or AP). However, there are some direct links to device objects,
+            # and if the user has hit one of those, we try to redirect them to the more
+            # specific page so they don't get 404ed
+            redirect = self._get_subtype_redirect(request, object_id)
+            if redirect:
+                return redirect
+
+        return super()._changeform_view(request, object_id, form_url, extra_context)

src/meshapi/admin/utils.py

@@ -0,0 +1,44 @@
+from typing import Union
+from urllib.parse import urljoin
+
+from django.contrib.contenttypes.models import ContentType
+from django.db.models import Model
+from django.urls import reverse
+
+from meshapi.models import AccessPoint, Device, Sector
+
+
+def get_admin_url(model: Model, site_base_url: str) -> str:
+    """
+    Get the admin URL corresponding to the given model
+    """
+    content_type = ContentType.objects.get_for_model(model.__class__)
+    return urljoin(
+        site_base_url,
+        reverse(
+            f"admin:{content_type.app_label}_{content_type.model}_change",
+            args=(model.pk,),
+        ),
+    )
+
+
+def downclass_device(device: Device) -> Union[Device, Sector, AccessPoint]:
+    """
+    Every sector and AP is also a device because we are using model inheritance. This function takes
+    a device object and returns the leaf object that this device corresponds to.
+
+    That is, it looks up the device ID in the sector and AP tables, and returns the appropriate
+    object type if it exists. Returns the input device if no such leaf exists
+    :param device: the device to use to search for leaf objects
+    :return: the leaf object corresponding to this device
+    """
+    sector = Sector.objects.filter(device_ptr=device).first()
+    access_point = AccessPoint.objects.filter(device_ptr=device).first()
+
+    db_object = device
+    if sector:
+        db_object = sector
+    elif access_point:
+        db_object = access_point
+
+    return db_object

src/meshapi/tests/test_admin_change_view.py

@@ -285,6 +285,21 @@ def test_change_device(self):
         response = self._call(change_url, 200)
         self._submit_form(change_url, bs4.BeautifulSoup(response.content.decode()).find(id="device_form"), 302)
 
+    def test_change_sector_accessed_via_device_redirect(self):
+        change_url = f"/admin/meshapi/device/{self.sector.id}/change/"
+        response = self._call(change_url, 302)
+        self.assertEqual(response.url, f"http://testserver/admin/meshapi/sector/{self.sector.id}/change/")
+
+    def test_change_ap_accessed_via_device_redirect(self):
+        change_url = f"/admin/meshapi/device/{self.access_point.id}/change/"
+        response = self._call(change_url, 302)
+        self.assertEqual(response.url, f"http://testserver/admin/meshapi/accesspoint/{self.access_point.id}/change/")
+
+    def test_change_device_bad_id(self):
+        change_url = "/admin/meshapi/device/00000000-0000-0000-0000-000000000000/change/"
+        response = self._call(change_url, 302)
+        self.assertEqual(response.url, "/admin/")
+
     def test_change_sector(self):
         change_url = f"/admin/meshapi/sector/{self.sector.id}/change/"
         response = self._call(change_url, 200)

src/meshapi/util/admin_notifications.py

@@ -2,30 +2,18 @@
 import logging
 import os
 from typing import Optional, Sequence, Type
-from urllib.parse import urljoin
 
 import requests
-from django.contrib.contenttypes.models import ContentType
 from django.db.models import Model
 from django.http import HttpRequest
-from django.urls import reverse
 from rest_framework.serializers import Serializer
 
+from meshapi.admin.utils import get_admin_url
+
 SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL = os.environ.get("SLACK_ADMIN_NOTIFICATIONS_WEBHOOK_URL")
 SITE_BASE_URL = os.environ.get("SITE_BASE_URL")
 
 
-def get_admin_url(model: Model, site_base_url: str) -> str:
-    content_type = ContentType.objects.get_for_model(model.__class__)
-    return urljoin(
-        site_base_url,
-        reverse(
-            f"admin:{content_type.app_label}_{content_type.model}_change",
-            args=(model.pk,),
-        ),
-    )
-
-
 def notify_administrators_of_data_issue(
     model_instances: Sequence[Model],
     serializer_class: Type[Serializer],

src/meshapi/util/uisp_import/sync_handlers.py

@@ -6,6 +6,7 @@
 from django.db import transaction
 from django.db.models import Q
 
+from meshapi.admin import downclass_device
 from meshapi.models import LOS, Device, Link, Node, Sector
 from meshapi.serializers import DeviceSerializer, LinkSerializer
 from meshapi.types.uisp_api.data_links import DataLink as UISPDataLink
@@ -23,7 +24,6 @@
 from meshapi.util.uisp_import.fetch_uisp import get_uisp_session
 from meshapi.util.uisp_import.update_objects import update_device_from_uisp_data, update_link_from_uisp_data
 from meshapi.util.uisp_import.utils import (
-    downclass_device,
     get_building_from_network_number,
     get_link_type,
     guess_compass_heading_from_device_name,

src/meshapi/util/uisp_import/utils.py

@@ -6,6 +6,7 @@
 import requests
 from rest_framework.serializers import Serializer
 
+from meshapi.admin import downclass_device
 from meshapi.models import AccessPoint, Building, Device, Link, Node, Sector
 from meshapi.serializers import AccessPointSerializer, DeviceSerializer, LinkSerializer, SectorSerializer
 from meshapi.types.uisp_api.data_links import DataLink as USIPDataLink
@@ -84,31 +85,15 @@ def get_serializer(db_object: Union[Device, Link, Sector, AccessPoint]) -> Type[
     return serializer_lookup[type(db_object)]
 
 
-def downclass_device(device: Device) -> Union[Device, Sector, AccessPoint]:
-    """
-    Down-class this device, so that the logging messages make sense.
-    We hide the model inheritance from admins, so they'd be confused if
-    we called a Sector a "device" in a notification message
-    (also the admin UI link would be wrong from their perspective)
-    """
-    sector = Sector.objects.filter(device_ptr=device).first()
-    access_point = AccessPoint.objects.filter(device_ptr=device).first()
-
-    db_object = device
-    if sector:
-        db_object = sector
-    elif access_point:
-        db_object = access_point
-
-    return db_object
-
-
 def notify_admins_of_changes(
     db_object: Union[Device, Link, Sector, AccessPoint],
     change_list: List[str],
     created: bool = False,
 ) -> None:
     # Attempt to downclass if needed (so admin links and such make sense)
+    # We hide the model inheritance from admins, so they'd be confused if
+    # we called a Sector a "device" in a notification message
+    # (also the admin UI link would be wrong from their perspective)
     if type(db_object) is Device:
         db_object = downclass_device(db_object)