Merge pull request 'Split up repo.Action in routers/web into smaller functions.' (#2476) from algernon/forgejo:refactor/routes/web/repo-action into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2476
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

routers/web/repo/repo.go

@@ -307,66 +307,66 @@ const (
 	tplStarUnstar   base.TplName = "repo/star_unstar"
 )
 
-// Action response for actions to a repository
+func ActionWatch(watch bool) func(ctx *context.Context) {
-func Action(ctx *context.Context) {
+	return func(ctx *context.Context) {
-	var err error
+		err := repo_model.WatchRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, watch)
-	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)
-	case "desc": // FIXME: this is not used
-		if !ctx.Repo.IsOwner() {
-			ctx.Error(http.StatusNotFound)
-			return
-		}
-
-		ctx.Repo.Repository.Description = ctx.FormString("desc")
-		ctx.Repo.Repository.Website = ctx.FormString("site")
-		err = repo_service.UpdateRepository(ctx, ctx.Repo.Repository, false)
-	}
-
 		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.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
 		}
+
+		ctx.HTML(http.StatusOK, tplWatchUnwatch)
+	}
 }
 
-	switch ctx.Params(":action") {
+func ActionStar(star bool) func(ctx *context.Context) {
-	case "watch", "unwatch":
+	return func(ctx *context.Context) {
-		ctx.HTML(http.StatusOK, tplWatchUnwatch)
+		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
-	case "star", "unstar":
+		}
+
+		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 {
 	repoTransfer, err := models.GetPendingRepositoryTransfer(ctx, ctx.Repo.Repository)

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() {

tests/integration/repo_starwatch_test.go

@@ -0,0 +1,82 @@
+// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+	"testing"
+
+	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func testRepoStarringOrWatching(t *testing.T, action, listURI string) {
+	t.Helper()
+
+	defer tests.PrepareTestEnv(t)()
+
+	oppositeAction := "un" + action
+	session := loginUser(t, "user5")
+
+	// Star/Watch the repo as user5
+	req := NewRequestWithValues(t, "POST", fmt.Sprintf("/user2/repo1/action/%s", action), map[string]string{
+		"_csrf": GetCSRF(t, session, "/user2/repo1"),
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+
+	// Load the repo home as user5
+	req = NewRequest(t, "GET", "/user2/repo1")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+
+	// Verify that the star/watch button is now the opposite
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	actionButton := htmlDoc.Find(fmt.Sprintf("form[action='/user2/repo1/action/%s']", oppositeAction))
+	assert.True(t, actionButton.Length() == 1)
+	text := strings.ToLower(actionButton.Find("button span.text").Text())
+	assert.Equal(t, oppositeAction, text)
+
+	// Load stargazers/watchers as user5
+	req = NewRequestf(t, "GET", "/user2/repo1/%s", listURI)
+	resp = session.MakeRequest(t, req, http.StatusOK)
+
+	// Verify that "user5" is among the stargazers/watchers
+	htmlDoc = NewHTMLParser(t, resp.Body)
+	htmlDoc.AssertElement(t, ".user-cards .list .item.ui.segment > a[href='/user5']", true)
+
+	// Unstar/unwatch the repo as user5
+	req = NewRequestWithValues(t, "POST", fmt.Sprintf("/user2/repo1/action/%s", oppositeAction), map[string]string{
+		"_csrf": GetCSRF(t, session, "/user2/repo1"),
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+
+	// Load the repo home as user5
+	req = NewRequest(t, "GET", "/user2/repo1")
+	resp = session.MakeRequest(t, req, http.StatusOK)
+
+	// Verify that the star/watch button is now back to its default
+	htmlDoc = NewHTMLParser(t, resp.Body)
+	actionButton = htmlDoc.Find(fmt.Sprintf("form[action='/user2/repo1/action/%s']", action))
+	assert.True(t, actionButton.Length() == 1)
+	text = strings.ToLower(actionButton.Find("button span.text").Text())
+	assert.Equal(t, action, text)
+
+	// Load stargazers/watchers as user5
+	req = NewRequestf(t, "GET", "/user2/repo1/%s", listURI)
+	resp = session.MakeRequest(t, req, http.StatusOK)
+
+	// Verify that "user5" is not among the stargazers/watchers
+	htmlDoc = NewHTMLParser(t, resp.Body)
+	htmlDoc.AssertElement(t, ".user-cards .list .item.ui.segment > a[href='/user5']", false)
+}
+
+func TestRepoStarUnstarUI(t *testing.T) {
+	testRepoStarringOrWatching(t, "star", "stars")
+}
+
+func TestRepoWatchUnwatchUI(t *testing.T) {
+	testRepoStarringOrWatching(t, "watch", "watchers")
+}