From 38e30f2a8f047cb11594b393d9fab8644c6735ea Mon Sep 17 00:00:00 2001
From: Gergely Nagy <forgejo@gergo.csillger.hu>
Date: Sun, 25 Feb 2024 22:45:57 +0100
Subject: [PATCH] Refactor routers/web/repo.Action

Split up `repo.Action` in `routers/web` into smaller functions.

While some of the functionality was very similar (starring / watching),
they are ultimately separate actions. Rather than collecting all of them
under a single handler (`repo.Action`), split them up into smaller,
independent functions.

This does result in a little bit of code duplication, but the
independent functions should be easier to follow and understand.

Signed-off-by: Gergely Nagy <forgejo@gergo.csillger.hu>
---
 routers/web/repo/repo.go | 85 ++++++++++++++++++++++------------------
 routers/web/web.go       |  9 ++++-
 2 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go
index c6f4304cc..88c73f65f 100644
--- a/routers/web/repo/repo.go
+++ b/routers/web/repo/repo.go
@@ -307,56 +307,65 @@ const (
 	tplStarUnstar   base.TplName = "repo/star_unstar"
 )
 
-// Action response for actions to a repository
-func Action(ctx *context.Context) {
-	var err error
-	switch ctx.Params(":action") {
-	case "watch":
-		err = repo_model.WatchRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, true)
-	case "unwatch":
-		err = repo_model.WatchRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, false)
-	case "star":
-		err = repo_model.StarRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, true)
-	case "unstar":
-		err = repo_model.StarRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, false)
-	case "accept_transfer":
-		err = acceptOrRejectRepoTransfer(ctx, true)
-	case "reject_transfer":
-		err = acceptOrRejectRepoTransfer(ctx, false)
-	}
+func ActionWatch(watch bool) func(ctx *context.Context) {
+	return func(ctx *context.Context) {
+		err := repo_model.WatchRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, watch)
+		if err != nil {
+			ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watch), err)
+			return
+		}
 
-	if err != nil {
-		ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.Params(":action")), err)
-		return
-	}
-
-	switch ctx.Params(":action") {
-	case "watch", "unwatch":
 		ctx.Data["IsWatchingRepo"] = repo_model.IsWatching(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID)
-	case "star", "unstar":
-		ctx.Data["IsStaringRepo"] = repo_model.IsStaring(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID)
-	}
 
-	switch ctx.Params(":action") {
-	case "watch", "unwatch", "star", "unstar":
 		// we have to reload the repository because NumStars or NumWatching (used in the templates) has just changed
 		ctx.Data["Repository"], err = repo_model.GetRepositoryByName(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.Name)
 		if err != nil {
-			ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.Params(":action")), err)
+			ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watch), err)
 			return
 		}
-	}
 
-	switch ctx.Params(":action") {
-	case "watch", "unwatch":
 		ctx.HTML(http.StatusOK, tplWatchUnwatch)
-		return
-	case "star", "unstar":
-		ctx.HTML(http.StatusOK, tplStarUnstar)
-		return
 	}
+}
 
-	ctx.RedirectToFirst(ctx.FormString("redirect_to"), ctx.Repo.RepoLink)
+func ActionStar(star bool) func(ctx *context.Context) {
+	return func(ctx *context.Context) {
+		err := repo_model.StarRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, star)
+		if err != nil {
+			ctx.ServerError(fmt.Sprintf("Action (star, %t)", star), err)
+			return
+		}
+
+		ctx.Data["IsStaringRepo"] = repo_model.IsStaring(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID)
+
+		// we have to reload the repository because NumStars or NumWatching (used in the templates) has just changed
+		ctx.Data["Repository"], err = repo_model.GetRepositoryByName(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.Name)
+		if err != nil {
+			ctx.ServerError(fmt.Sprintf("Action (star, %t)", star), err)
+			return
+		}
+
+		ctx.HTML(http.StatusOK, tplStarUnstar)
+	}
+}
+
+func ActionTransfer(accept bool) func(ctx *context.Context) {
+	return func(ctx *context.Context) {
+		var action string
+		if accept {
+			action = "accept_transfer"
+		} else {
+			action = "reject_transfer"
+		}
+
+		err := acceptOrRejectRepoTransfer(ctx, accept)
+		if err != nil {
+			ctx.ServerError(fmt.Sprintf("Action (%s)", action), err)
+			return
+		}
+
+		ctx.RedirectToFirst(ctx.FormString("redirect_to"), ctx.Repo.RepoLink)
+	}
 }
 
 func acceptOrRejectRepoTransfer(ctx *context.Context, accept bool) error {
diff --git a/routers/web/web.go b/routers/web/web.go
index 602a233c9..38cf5e92b 100644
--- a/routers/web/web.go
+++ b/routers/web/web.go
@@ -1124,7 +1124,14 @@ func registerRoutes(m *web.Route) {
 		}, ctxDataSet("PageIsRepoSettings", true, "LFSStartServer", setting.LFS.StartServer))
 	}, reqSignIn, context.RepoAssignment, context.UnitTypes(), reqRepoAdmin, context.RepoRef())
 
-	m.Post("/{username}/{reponame}/action/{action}", reqSignIn, context.RepoAssignment, context.UnitTypes(), repo.Action)
+	m.Group("/{username}/{reponame}/action", func() {
+		m.Post("/watch", repo.ActionWatch(true))
+		m.Post("/unwatch", repo.ActionWatch(false))
+		m.Post("/accept_transfer", repo.ActionTransfer(true))
+		m.Post("/reject_transfer", repo.ActionTransfer(false))
+		m.Post("/star", repo.ActionStar(true))
+		m.Post("/unstar", repo.ActionStar(false))
+	}, reqSignIn, context.RepoAssignment, context.UnitTypes())
 
 	// Grouping for those endpoints not requiring authentication (but should respect ignSignIn)
 	m.Group("/{username}/{reponame}", func() {