From 51ff4970ec8b91f5853aadc47e2e93724ff4d6e8 Mon Sep 17 00:00:00 2001
From: 0ko <0ko@noreply.codeberg.org>
Date: Fri, 28 Mar 2025 19:50:43 +0000
Subject: [PATCH] ui: improve error pages (#7274)

* add testing
* make each page accessible via `/devtest/error`
* allow translating the `Page not found` part of the title
* code: improve consistency, remove unused
* devtest: put index page in a container to fix alignment
* 500: make navbar more like the real one, remove fake menu button
* deadcode: remove unused `func NotFound`: it was added in https://codeberg.org/forgejo/forgejo/commit/bdd32f152d73f31beffa854fb7382072043ef235 and the only usage was removed in https://codeberg.org/forgejo/forgejo/commit/1bfb0a24d843e10d6d95c4319a84980485e584ed

Preview:
https://codeberg.org/attachments/1b75afb3-e898-410f-be02-f036a5400143

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7274
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: Beowulf <beowulf@beocode.eu>
---
 .deadcode-out                           |  3 --
 modules/testlogger/testlogger.go        |  3 ++
 options/locale_next/locale_en-US.json   |  5 ++-
 routers/web/devtest/devtest.go          | 13 ++++++
 routers/web/home.go                     |  7 +---
 routers/web/web.go                      |  2 +
 services/context/context_response.go    | 11 +++--
 templates/devtest/list.tmpl             | 21 +++++++---
 templates/status/500.tmpl               | 17 +++-----
 tests/integration/devtest_error_test.go | 53 +++++++++++++++++++++++++
 tests/integration/repo_test.go          |  3 +-
 11 files changed, 106 insertions(+), 32 deletions(-)
 create mode 100644 tests/integration/devtest_error_test.go

diff --git a/.deadcode-out b/.deadcode-out
index e366abee94..31184826ae 100644
--- a/.deadcode-out
+++ b/.deadcode-out
@@ -210,9 +210,6 @@ forgejo.org/modules/zstd
 	Writer.Write
 	Writer.Close
 
-forgejo.org/routers/web
-	NotFound
-
 forgejo.org/routers/web/org
 	MustEnableProjects
 
diff --git a/modules/testlogger/testlogger.go b/modules/testlogger/testlogger.go
index b5f196ad4b..5567ea433e 100644
--- a/modules/testlogger/testlogger.go
+++ b/modules/testlogger/testlogger.go
@@ -363,6 +363,9 @@ var ignoredErrorMessage = []string{
 
 	// TestDatabaseCollation
 	`[E] [Error SQL Query] INSERT INTO test_collation_tbl (txt) VALUES ('main') []`,
+
+	// TestDevtestErrorpages
+	`ErrorPage() [E] Example error: Example error`,
 }
 
 func (w *testLoggerWriterCloser) recordError(msg string) {
diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json
index 4f5d102392..e525598ed5 100644
--- a/options/locale_next/locale_en-US.json
+++ b/options/locale_next/locale_en-US.json
@@ -16,5 +16,6 @@
     "incorrect_root_url": "This Forgejo instance is configured to be served on \"%s\". You are currently viewing Forgejo through a different URL, which may cause parts of the application to break. The canonical URL is controlled by Forgejo admins via the ROOT_URL setting in the app.ini.",
     "themes.names.forgejo-auto": "Forgejo (follow system theme)",
     "themes.names.forgejo-light": "Forgejo light",
-    "themes.names.forgejo-dark": "Forgejo dark"
-}
+    "themes.names.forgejo-dark": "Forgejo dark",
+    "error.not_found.title": "Page not found"
+}
\ No newline at end of file
diff --git a/routers/web/devtest/devtest.go b/routers/web/devtest/devtest.go
index 37496ca117..2ef030cb92 100644
--- a/routers/web/devtest/devtest.go
+++ b/routers/web/devtest/devtest.go
@@ -1,9 +1,11 @@
 // Copyright 2023 The Gitea Authors. All rights reserved.
+// Copyright 2025 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package devtest
 
 import (
+	"errors"
 	"net/http"
 	"path"
 	"strings"
@@ -42,6 +44,17 @@ func FetchActionTest(ctx *context.Context) {
 	ctx.JSONRedirect("")
 }
 
+func ErrorPage(ctx *context.Context) {
+	if ctx.Params("errcode") == "404" {
+		ctx.NotFound("Example error", errors.New("Example error"))
+		return
+	} else if ctx.Params("errcode") == "413" {
+		ctx.HTML(http.StatusRequestEntityTooLarge, base.TplName("status/413"))
+		return
+	}
+	ctx.ServerError("Example error", errors.New("Example error"))
+}
+
 func Tmpl(ctx *context.Context) {
 	now := time.Now()
 	ctx.Data["TimeNow"] = now
diff --git a/routers/web/home.go b/routers/web/home.go
index e0a466a81c..bd9942748a 100644
--- a/routers/web/home.go
+++ b/routers/web/home.go
@@ -1,5 +1,6 @@
 // Copyright 2014 The Gogs Authors. All rights reserved.
 // Copyright 2019 The Gitea Authors. All rights reserved.
+// Copyright 2025 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package web
@@ -112,9 +113,3 @@ func HomeSitemap(ctx *context.Context) {
 		log.Error("Failed writing sitemap: %v", err)
 	}
 }
-
-// NotFound render 404 page
-func NotFound(ctx *context.Context) {
-	ctx.Data["Title"] = "Page Not Found"
-	ctx.NotFound("home.NotFound", nil)
-}
diff --git a/routers/web/web.go b/routers/web/web.go
index 303167a6b9..3643943b5b 100644
--- a/routers/web/web.go
+++ b/routers/web/web.go
@@ -1,4 +1,5 @@
 // Copyright 2017 The Gitea Authors. All rights reserved.
+// Copyright 2023 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package web
@@ -1661,6 +1662,7 @@ func registerRoutes(m *web.Route) {
 		m.Any("/devtest", devtest.List)
 		m.Any("/devtest/fetch-action-test", devtest.FetchActionTest)
 		m.Any("/devtest/{sub}", devtest.Tmpl)
+		m.Get("/devtest/error/{errcode}", devtest.ErrorPage)
 	}
 
 	m.NotFound(func(w http.ResponseWriter, req *http.Request) {
diff --git a/services/context/context_response.go b/services/context/context_response.go
index e20e7dd852..e64f478420 100644
--- a/services/context/context_response.go
+++ b/services/context/context_response.go
@@ -1,4 +1,5 @@
 // Copyright 2023 The Gitea Authors. All rights reserved.
+// Copyright 2024 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package context
@@ -66,7 +67,10 @@ func (ctx *Context) RedirectToFirst(location ...string) string {
 	return setting.AppSubURL + "/"
 }
 
-const tplStatus500 base.TplName = "status/500"
+const (
+	tplStatus404 base.TplName = "status/404"
+	tplStatus500 base.TplName = "status/500"
+)
 
 // HTML calls Context.HTML and renders the template to HTTP response
 func (ctx *Context) HTML(status int, name base.TplName) {
@@ -153,8 +157,8 @@ func (ctx *Context) notFoundInternal(logMsg string, logErr error) {
 	}
 
 	ctx.Data["IsRepo"] = ctx.Repo.Repository != nil
-	ctx.Data["Title"] = "Page Not Found"
-	ctx.HTML(http.StatusNotFound, base.TplName("status/404"))
+	ctx.Data["Title"] = ctx.Locale.TrString("error.not_found.title")
+	ctx.HTML(http.StatusNotFound, tplStatus404)
 }
 
 // ServerError displays a 500 (Internal Server Error) page and prints the given error, if any.
@@ -177,7 +181,6 @@ func (ctx *Context) serverErrorInternal(logMsg string, logErr error) {
 		}
 	}
 
-	ctx.Data["Title"] = "Internal Server Error"
 	ctx.HTML(http.StatusInternalServerError, tplStatus500)
 }
 
diff --git a/templates/devtest/list.tmpl b/templates/devtest/list.tmpl
index 90b1fcc9d0..e169311222 100644
--- a/templates/devtest/list.tmpl
+++ b/templates/devtest/list.tmpl
@@ -1,10 +1,21 @@
 {{template "base/head" .}}
 
-<ul>
-	{{range .SubNames}}
-	<li><a href="{{AppSubUrl}}/devtest/{{.}}">{{.}}</a></li>
-	{{end}}
-</ul>
+<div role="main" class="page-content ui container">
+	<ul>
+		{{range .SubNames}}
+		<li><a href="{{AppSubUrl}}/devtest/{{.}}">{{.}}</a></li>
+		{{end}}
+	</ul>
+
+	<article>
+		<h2>Error pages</h2>
+		<ul>
+			<li><a href="./error/404">Not found</a></li>
+			<li><a href="./error/413">Quota exhaustion</a></li>
+			<li><a href="./error/500">Server error</a></li>
+		</ul>
+	</article>
+</div>
 
 <style>
 ul {
diff --git a/templates/status/500.tmpl b/templates/status/500.tmpl
index 59f7c25688..4e7e64d9c7 100644
--- a/templates/status/500.tmpl
+++ b/templates/status/500.tmpl
@@ -16,19 +16,14 @@
 </head>
 <body>
 	<div class="full height">
-		<nav class="ui secondary menu">
-			<div class="ui container tw-flex">
-				<div class="item tw-flex-1">
-					<a href="{{AppSubUrl}}/" aria-label="{{ctx.Locale.Tr "home"}}">
-						<img width="30" height="30" src="{{AssetUrlPrefix}}/img/logo.svg" alt="{{ctx.Locale.Tr "logo"}}" aria-hidden="true">
-					</a>
-				</div>
-				<div class="item">
-					<button class="ui icon button disabled">{{svg "octicon-three-bars"}}</button>{{/* a fake button to make the UI looks better*/}}
-				</div>
+		<nav id="navbar" aria-label="{{ctx.Locale.Tr "aria.navbar"}}">
+			<div class="navbar-left ui secondary menu">
+				<a class="item" id="navbar-logo" href="{{AppSubUrl}}/" aria-label="{{ctx.Locale.Tr "home"}}">
+					<img width="30" height="30" src="{{AssetUrlPrefix}}/img/logo.svg" alt="{{ctx.Locale.Tr "logo"}}" aria-hidden="true">
+				</a>
 			</div>
 		</nav>
-		<div class="divider tw-my-0"></div>
+
 		<div role="main" class="page-content status-page-500">
 			<div class="ui container" >
 				<style> .ui.message.flash-message { text-align: left; } </style>
diff --git a/tests/integration/devtest_error_test.go b/tests/integration/devtest_error_test.go
new file mode 100644
index 0000000000..8b86f1c552
--- /dev/null
+++ b/tests/integration/devtest_error_test.go
@@ -0,0 +1,53 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package integration
+
+import (
+	"net/http"
+	"testing"
+
+	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
+	"forgejo.org/routers"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// `/devtest/error/{errcode}` provides a convenient way of testing various
+// error pages sometimes which can be hard to reach otherwise.
+// This file is a test of various attributes on those pages.
+
+func TestDevtestErrorpages(t *testing.T) {
+	defer test.MockVariableValue(&setting.IsProd, false)()
+	defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())()
+
+	t.Run("Server error", func(t *testing.T) {
+		// `/devtest/error/x` returns 500 for any x by default.
+		// `/500` is simply for good look here
+		req := NewRequest(t, "GET", "/devtest/error/500")
+		resp := MakeRequest(t, req, http.StatusInternalServerError)
+		doc := NewHTMLParser(t, resp.Body)
+		assert.EqualValues(t, "500", doc.Find(".error-code").Text())
+		assert.Contains(t, doc.Find("head title").Text(), "Internal server error")
+	})
+
+	t.Run("Page not found",
+		func(t *testing.T) {
+			req := NewRequest(t, "GET", "/devtest/error/404").
+				// Without this header `notFoundInternal` returns plaintext error message
+				SetHeader("Accept", "text/html")
+			resp := MakeRequest(t, req, http.StatusNotFound)
+			doc := NewHTMLParser(t, resp.Body)
+			assert.EqualValues(t, "404", doc.Find(".error-code").Text())
+			assert.Contains(t, doc.Find("head title").Text(), "Page not found")
+		})
+
+	t.Run("Quota exhaustion",
+		func(t *testing.T) {
+			req := NewRequest(t, "GET", "/devtest/error/413")
+			resp := MakeRequest(t, req, http.StatusRequestEntityTooLarge)
+			doc := NewHTMLParser(t, resp.Body)
+			assert.EqualValues(t, "413", doc.Find(".error-code").Text())
+		})
+}
diff --git a/tests/integration/repo_test.go b/tests/integration/repo_test.go
index f8961443c5..35cab81975 100644
--- a/tests/integration/repo_test.go
+++ b/tests/integration/repo_test.go
@@ -1,4 +1,5 @@
 // Copyright 2017 The Gitea Authors. All rights reserved.
+// Copyright 2023 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package integration
@@ -691,7 +692,7 @@ func TestCommitView(t *testing.T) {
 
 		// Really ensure that 404 is being sent back.
 		doc := NewHTMLParser(t, resp.Body)
-		doc.AssertElement(t, `[aria-label="Page Not Found"]`, true)
+		doc.AssertElement(t, `[aria-label="Page not found"]`, true)
 	})
 
 	t.Run("Too short commit ID", func(t *testing.T) {