fix: show "not found" for deleted servers, nodes and users

CHANGELOG.md

@@ -50,6 +50,10 @@ follows [Semantic Versioning](https://semver.org) guidelines.
 - Fixed failing jobs retrying with no pause. A job whose request to Proxmox failed was retried at once, again and
   again, until it ran out of time -- thousands of requests a minute against a node that was already struggling. Jobs
   now wait a few seconds, then longer, between retries.
+- Fixed opening a server that has been deleted showing "Something went wrong" with "No query results for model
+  [Convoy\Models\Server]". It now shows "Server not found" with a way back to the server list, and the API names a
+  missing record in plain words ("This server could not be found. It may have been deleted."). Deleted nodes and users
+  in the admin area, which used to break the page outright, now get the same screen.
 
 
 ## v4.7.0-rc.2

app/Exceptions/Handler.php

@@ -4,8 +4,12 @@
 
 use Exception;
 use Throwable;
+use Illuminate\Http\Request;
+use Illuminate\Support\Str;
 use Illuminate\Container\Container;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 
 class Handler extends ExceptionHandler
 {
@@ -28,6 +32,25 @@ public function register(): void
         $this->reportable(function (Throwable $e) {
             //
         });
+
+        // A missing route-bound model came back with Laravel's own message --
+        // "No query results for model [Convoy\Models\Server]" -- which the panel
+        // showed as-is. Name the record in plain words instead. Laravel has
+        // already turned it into a NotFoundHttpException by the time this runs,
+        // so the model is on the original exception.
+        $this->renderable(function (NotFoundHttpException $e, Request $request) {
+            $missing = $e->getPrevious();
+
+            if (! $missing instanceof ModelNotFoundException || ! $request->expectsJson()) {
+                return null;
+            }
+
+            $model = Str::of(class_basename($missing->getModel()))->headline()->lower();
+
+            return response()->json([
+                'message' => "This {$model} could not be found. It may have been deleted.",
+            ], 404);
+        });
     }
 
     /**

resources/scripts/api/http.ts

@@ -64,6 +64,13 @@ export default http
  * Converts an error into a human readable response. Mostly just a generic helper to
  * make sure we display the message from the server back to the user if we can.
  */
+/**
+ * Whether a request failed because the record it asked for doesn't exist --
+ * typically one deleted since the page was opened.
+ */
+export const isNotFoundError = (error: any): boolean =>
+    error?.response?.status === 404
+
 export function httpErrorToHuman(error: any): string {
     if (error.response && error.response.data) {
         let {data} = error.response

resources/scripts/components/elements/ScreenBlock.tsx

@@ -118,6 +118,7 @@ export const NotFound = ({
             "The link is either broken or doesn't exist on the server."
         }
         icon={LinkIcon}
+        center
         full={full}
         onBack={onBack}
     />

resources/scripts/routers/AdminNodeRouter.tsx

@@ -1,8 +1,14 @@
-import { lazyLoad, query } from '@/routers/helpers'
+import {
+    LoadFailureScreen,
+    isLoadFailure,
+    lazyLoad,
+    loadFailure,
+    query,
+} from '@/routers/helpers'
 import { Route } from '@/routers/router'
 import { lazy, useContext, useEffect } from 'react'
 import { useTranslation } from 'react-i18next'
-import { Outlet } from 'react-router-dom'
+import { Outlet, useLoaderData } from 'react-router-dom'
 
 import getNode from '@/api/admin/nodes/getNode'
 import useNodeSWR, { getKey as getPoolKey } from '@/api/admin/nodes/useNodeSWR'
@@ -31,7 +37,7 @@ export const routes: Route[] = [
                         const node = await query(getPoolKey(nodeId), () => getNode(nodeId));
                         return node;
                     } catch (error) {
-                        return null;
+                        return loadFailure(error)
                     }
                 },
                 element: lazyLoad(
@@ -126,6 +132,7 @@ export const routes: Route[] = [
 const AdminNodeRouter = () => {
     const { t: tStrings } = useTranslation('strings')
     const { data: node } = useNodeSWR()
+    const loaded = useLoaderData()
 
     const routes = [
         {
@@ -164,9 +171,19 @@ const AdminNodeRouter = () => {
     }, [])
 
     useEffect(() => {
-        setBreadcrumb(node.name)
+        setBreadcrumb(node?.name)
     }, [node])
 
+    if (isLoadFailure(loaded)) {
+        return (
+            <LoadFailureScreen
+                failure={loaded}
+                record='Node'
+                backTo='/admin/nodes'
+            />
+        )
+    }
+
     return <>{!node ? <Spinner /> : <Outlet />}</>
 }
 

resources/scripts/routers/AdminServerRouter.tsx

@@ -4,11 +4,14 @@ import { AdminServerContext } from '@/state/admin/server'
 import { ExclamationCircleIcon } from '@heroicons/react/24/outline'
 import { lazy, useContext, useEffect, useState } from 'react'
 import { useTranslation } from 'react-i18next'
-import { Outlet, useMatch } from 'react-router-dom'
+import { Outlet, useMatch, useNavigate } from 'react-router-dom'
 
-import { httpErrorToHuman } from '@/api/http'
+import { httpErrorToHuman, isNotFoundError } from '@/api/http'
 
-import ScreenBlock, { ErrorMessage } from '@/components/elements/ScreenBlock'
+import ScreenBlock, {
+    ErrorMessage,
+    NotFound,
+} from '@/components/elements/ScreenBlock'
 import Spinner from '@/components/elements/Spinner'
 import { NavigationBarContext } from '@/components/elements/navigation/NavigationBar'
 
@@ -95,6 +98,8 @@ export const routes: Route[] = [
 const AdminServerRouter = () => {
     const match = useMatch('/admin/servers/:id/*')
     const [error, setError] = useState<string>()
+    const [notFound, setNotFound] = useState(false)
+    const navigate = useNavigate()
     const server = AdminServerContext.useStoreState(state => state.server.data)
     const getServer = AdminServerContext.useStoreActions(
         actions => actions.server.getServer
@@ -118,8 +123,10 @@ const AdminServerRouter = () => {
 
     useEffect(() => {
         setError(undefined)
+        setNotFound(false)
 
         getServer(match!.params.id as string).catch((error: any) => {
+            setNotFound(isNotFoundError(error))
             setError(httpErrorToHuman(error))
         })
 
@@ -143,7 +150,13 @@ const AdminServerRouter = () => {
     return (
         <>
             {!server ? (
-                error ? (
+                notFound ? (
+                    <NotFound
+                        title='Server not found'
+                        message="This server doesn't exist, or it has been deleted."
+                        onBack={() => navigate('/admin/servers')}
+                    />
+                ) : error ? (
                     <ErrorMessage message={error} />
                 ) : (
                     <Spinner />

resources/scripts/routers/AdminUserRouter.tsx

@@ -1,7 +1,13 @@
-import { lazyLoad, query } from '@/routers/helpers'
+import {
+    LoadFailureScreen,
+    isLoadFailure,
+    lazyLoad,
+    loadFailure,
+    query,
+} from '@/routers/helpers'
 import { Route } from '@/routers/router'
 import { lazy, useContext, useEffect } from 'react'
-import { Outlet } from 'react-router-dom'
+import { Outlet, useLoaderData } from 'react-router-dom'
 
 import getUser from '@/api/admin/users/getUser'
 import useUserSWR, { getKey } from '@/api/admin/users/useUserSWR'
@@ -30,7 +36,7 @@ export const routes: Route[] = [
                         const user = await query(getKey(id), () => getUser(id));
                         return user;
                     } catch (error) {
-                        return null;
+                        return loadFailure(error)
                     }
                 },
                 children: [
@@ -89,6 +95,7 @@ const visibleRoutes = [
 const AdminUserRouter = () => {
     const { setRoutes, setBreadcrumb } = useContext(NavigationBarContext)
     const { data: user } = useUserSWR()
+    const loaded = useLoaderData()
 
     useEffect(() => {
         setRoutes(visibleRoutes)
@@ -97,8 +104,18 @@ const AdminUserRouter = () => {
     }, [])
 
     useEffect(() => {
-        setBreadcrumb(user.name)
-    }, [user.name])
+        setBreadcrumb(user?.name)
+    }, [user?.name])
+
+    if (isLoadFailure(loaded)) {
+        return (
+            <LoadFailureScreen
+                failure={loaded}
+                record='User'
+                backTo='/admin/users'
+            />
+        )
+    }
 
     return <Outlet />
 }

resources/scripts/routers/ServerRouter.tsx

@@ -8,12 +8,15 @@ import {
 } from '@heroicons/react/24/outline'
 import { lazy, useContext, useEffect, useMemo, useState } from 'react'
 import { useTranslation } from 'react-i18next'
-import { Outlet, useMatches } from 'react-router-dom'
+import { Outlet, useMatches, useNavigate } from 'react-router-dom'
 
-import { httpErrorToHuman } from '@/api/http'
+import { httpErrorToHuman, isNotFoundError } from '@/api/http'
 import { EloquentStatus } from '@/api/server/types'
 
-import ScreenBlock, { ErrorMessage } from '@/components/elements/ScreenBlock'
+import ScreenBlock, {
+    ErrorMessage,
+    NotFound,
+} from '@/components/elements/ScreenBlock'
 import Spinner from '@/components/elements/Spinner'
 import { NavigationBarContext } from '@/components/elements/navigation/NavigationBar'
 
@@ -130,6 +133,8 @@ const ServerRouter = () => {
     const matches = useMatches()
     const id = matches[0].params.id
     const [error, setError] = useState<string>()
+    const [notFound, setNotFound] = useState(false)
+    const navigate = useNavigate()
     const server = ServerContext.useStoreState(state => state.server.data)
     const getServer = ServerContext.useStoreActions(
         actions => actions.server.getServer
@@ -159,8 +164,10 @@ const ServerRouter = () => {
 
     useEffect(() => {
         setError(undefined)
+        setNotFound(false)
 
         getServer(id as string).catch((error: any) => {
+            setNotFound(isNotFoundError(error))
             setError(httpErrorToHuman(error))
         })
 
@@ -261,7 +268,13 @@ const ServerRouter = () => {
     return (
         <>
             {!server ? (
-                error ? (
+                notFound ? (
+                    <NotFound
+                        title='Server not found'
+                        message="This server doesn't exist, or it has been deleted."
+                        onBack={() => navigate('/')}
+                    />
+                ) : error ? (
                     <ErrorMessage message={error} />
                 ) : (
                     <Spinner />

resources/scripts/routers/helpers.tsx

@@ -1,6 +1,10 @@
 import { LazyExoticComponent } from 'react'
+import { useNavigate } from 'react-router-dom'
 import { BareFetcher, Key, MutatorOptions, mutate } from 'swr'
 
+import { httpErrorToHuman, isNotFoundError } from '@/api/http'
+
+import { ErrorMessage, NotFound } from '@/components/elements/ScreenBlock'
 import Spinner from '@/components/elements/Spinner'
 
 export const lazyLoad = (
@@ -23,4 +27,46 @@ export const query = async <T,>(
     await mutate(key, data, options)
 
     return data
-}
+}
+
+/**
+ * What a record page's loader returns when it couldn't load the record, so the
+ * page can say so instead of rendering without it.
+ */
+export interface LoadFailure {
+    loadFailed: true
+    notFound: boolean
+    message: string
+}
+
+export const loadFailure = (error: any): LoadFailure => ({
+    loadFailed: true,
+    notFound: isNotFoundError(error),
+    message: httpErrorToHuman(error),
+})
+
+export const isLoadFailure = (data: unknown): data is LoadFailure =>
+    typeof data === 'object' && data !== null && 'loadFailed' in data
+
+export const LoadFailureScreen = ({
+    failure,
+    record,
+    backTo,
+}: {
+    failure: LoadFailure
+    /** What the page shows, as in "Node not found". */
+    record: string
+    backTo: string
+}) => {
+    const navigate = useNavigate()
+
+    return failure.notFound ? (
+        <NotFound
+            title={`${record} not found`}
+            message={`This ${record.toLowerCase()} doesn't exist, or it has been deleted.`}
+            onBack={() => navigate(backTo)}
+        />
+    ) : (
+        <ErrorMessage message={failure.message} />
+    )
+}

tests/Feature/NotFoundResponseTest.php

@@ -0,0 +1,19 @@
+<?php
+
+use Convoy\Models\User;
+
+// A deleted record used to come back as "No query results for model
+// [Convoy\Models\Server]", which the panel showed to users as-is.
+it('names a missing server in plain words', function () {
+    $this->actingAs(User::factory()->create())
+         ->getJson('/api/client/servers/deadbeef')
+         ->assertNotFound()
+         ->assertExactJson(['message' => 'This server could not be found. It may have been deleted.']);
+});
+
+it('names a missing node in plain words', function () {
+    $this->actingAs(User::factory()->create(['root_admin' => true]))
+         ->getJson('/api/admin/nodes/999999999')
+         ->assertNotFound()
+         ->assertJsonPath('message', 'This node could not be found. It may have been deleted.');
+});

tests/e2e/admin/not-found.spec.ts

@@ -0,0 +1,24 @@
+import { expect, test } from '@playwright/test'
+
+import { signInAsAdmin } from '../support/auth'
+
+test.beforeEach(async ({ page }) => {
+    await signInAsAdmin(page)
+})
+
+// Opening a deleted record used to show "Something went wrong" with Laravel's
+// "No query results for model [...]" (servers), or crash outright (nodes, users).
+for (const { path, record } of [
+    { path: '/servers/deadbeef', record: 'Server' },
+    { path: '/admin/servers/00000000-0000-4000-8000-00000000dead', record: 'Server' },
+    { path: '/admin/nodes/999999999', record: 'Node' },
+    { path: '/admin/users/999999999', record: 'User' },
+]) {
+    test(`says a missing ${record.toLowerCase()} is gone (${path})`, async ({ page }) => {
+        await page.goto(path)
+
+        await expect(page.getByRole('heading', { name: `${record} not found` })).toBeVisible()
+        await expect(page.getByText('No query results')).toHaveCount(0)
+        await expect(page.getByRole('button', { name: 'Go Back' })).toBeVisible()
+    })
+}