From f68bc0ec6ac437b8c2339719b6063068fc6bbdfb Mon Sep 17 00:00:00 2001
From: Gusted <postmaster@gusted.xyz>
Date: Thu, 22 Feb 2024 22:25:19 +0100
Subject: [PATCH] [REFACTOR] Simplify converting struct to map in admin stats

- Instead of relying on JSON to convert the struct to map, use
`reflect` to do this conversion. Also simplify it a bit by only passing
one variable to the template.
- This avoids issues where the conversion to JSON causes changes in
the value, for example huge numbers are converted to its scientific
notation but are consequently not converted back when being displayed.
- Adds unit tests.
- Resolves an issue where the amount of comments is being displayed in
scientific notation on Codeberg.
---
 routers/web/admin/admin.go      | 35 +++++++++++-------------
 routers/web/admin/admin_test.go | 48 +++++++++++++++++++++++++++++++++
 templates/admin/stats.tmpl      |  4 +--
 3 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/routers/web/admin/admin.go b/routers/web/admin/admin.go
index 9fbd429f74..58bb281731 100644
--- a/routers/web/admin/admin.go
+++ b/routers/web/admin/admin.go
@@ -7,8 +7,8 @@ package admin
 import (
 	"fmt"
 	"net/http"
+	"reflect"
 	"runtime"
-	"sort"
 	"time"
 
 	activities_model "code.gitea.io/gitea/models/activities"
@@ -16,7 +16,6 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/graceful"
-	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/updatechecker"
@@ -225,26 +224,22 @@ func CronTasks(ctx *context.Context) {
 func MonitorStats(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("admin.monitor.stats")
 	ctx.Data["PageIsAdminMonitorStats"] = true
-	bs, err := json.Marshal(activities_model.GetStatistic(ctx).Counter)
-	if err != nil {
-		ctx.ServerError("MonitorStats", err)
-		return
-	}
-	statsCounter := map[string]any{}
-	err = json.Unmarshal(bs, &statsCounter)
-	if err != nil {
-		ctx.ServerError("MonitorStats", err)
-		return
-	}
-	statsKeys := make([]string, 0, len(statsCounter))
-	for k := range statsCounter {
-		if statsCounter[k] == nil {
+	modelStats := activities_model.GetStatistic(ctx).Counter
+	stats := map[string]any{}
+
+	// To avoid manually converting the values of the stats struct to an map,
+	// and to avoid using JSON to do this for us (JSON encoder converts numbers to
+	// scientific notation). Use reflect to convert the struct to an map.
+	rv := reflect.ValueOf(modelStats)
+	for i := 0; i < rv.NumField(); i++ {
+		field := rv.Field(i)
+		// Preserve old behavior, do not show arrays that are empty.
+		if field.Kind() == reflect.Slice && field.Len() == 0 {
 			continue
 		}
-		statsKeys = append(statsKeys, k)
+		stats[rv.Type().Field(i).Name] = field.Interface()
 	}
-	sort.Strings(statsKeys)
-	ctx.Data["StatsKeys"] = statsKeys
-	ctx.Data["StatsCounter"] = statsCounter
+
+	ctx.Data["Stats"] = stats
 	ctx.HTML(http.StatusOK, tplStats)
 }
diff --git a/routers/web/admin/admin_test.go b/routers/web/admin/admin_test.go
index 2b65ab3ea3..452291e179 100644
--- a/routers/web/admin/admin_test.go
+++ b/routers/web/admin/admin_test.go
@@ -6,6 +6,11 @@ package admin
 import (
 	"testing"
 
+	activities_model "code.gitea.io/gitea/models/activities"
+	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/contexttest"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -66,3 +71,46 @@ func TestShadowPassword(t *testing.T) {
 		assert.EqualValues(t, k.Result, shadowPassword(k.Provider, k.CfgItem))
 	}
 }
+
+func TestMonitorStats(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	t.Run("Normal", func(t *testing.T) {
+		defer test.MockVariableValue(&setting.Metrics.EnabledIssueByLabel, false)()
+		defer test.MockVariableValue(&setting.Metrics.EnabledIssueByRepository, false)()
+
+		ctx, _ := contexttest.MockContext(t, "admin/stats")
+		MonitorStats(ctx)
+
+		// Test some of the stats manually.
+		mappedStats := ctx.Data["Stats"].(map[string]any)
+		stats := activities_model.GetStatistic(ctx).Counter
+
+		assert.EqualValues(t, stats.Comment, mappedStats["Comment"])
+		assert.EqualValues(t, stats.Issue, mappedStats["Issue"])
+		assert.EqualValues(t, stats.User, mappedStats["User"])
+		assert.EqualValues(t, stats.Milestone, mappedStats["Milestone"])
+
+		// Ensure that these aren't set.
+		assert.Empty(t, stats.IssueByLabel)
+		assert.Empty(t, stats.IssueByRepository)
+		assert.Nil(t, mappedStats["IssueByLabel"])
+		assert.Nil(t, mappedStats["IssueByRepository"])
+	})
+
+	t.Run("IssueByX", func(t *testing.T) {
+		defer test.MockVariableValue(&setting.Metrics.EnabledIssueByLabel, true)()
+		defer test.MockVariableValue(&setting.Metrics.EnabledIssueByRepository, true)()
+
+		ctx, _ := contexttest.MockContext(t, "admin/stats")
+		MonitorStats(ctx)
+
+		mappedStats := ctx.Data["Stats"].(map[string]any)
+		stats := activities_model.GetStatistic(ctx).Counter
+
+		assert.NotEmpty(t, stats.IssueByLabel)
+		assert.NotEmpty(t, stats.IssueByRepository)
+		assert.EqualValues(t, stats.IssueByLabel, mappedStats["IssueByLabel"])
+		assert.EqualValues(t, stats.IssueByRepository, mappedStats["IssueByRepository"])
+	})
+}
diff --git a/templates/admin/stats.tmpl b/templates/admin/stats.tmpl
index 04fa862a85..70f2aa7fb4 100644
--- a/templates/admin/stats.tmpl
+++ b/templates/admin/stats.tmpl
@@ -5,10 +5,10 @@
 	</h4>
 	<div class="ui attached table segment">
 		<table class="ui very basic striped table unstackable">
-			{{range $statsKey := .StatsKeys}}
+			{{range $statsKey, $statsValue := .Stats}}
 			<tr>
 				<td width="200">{{$statsKey}}</td>
-				<td>{{index $.StatsCounter $statsKey}}</td>
+				<td>{{$statsValue}}</td>
 			</tr>
 			{{end}}
 		</table>