From 02eab930c9f7b481a62f3232f6e5b154a6a922f8 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 25 Feb 2024 22:20:23 +0100 Subject: [PATCH 1/3] routers/web: Drop an unused branch from repo.Action The "desc" action has not been used since at least 2016, probably much earlier. It's an ancient Gogs artifact - drop it. Signed-off-by: Gergely Nagy --- routers/web/repo/repo.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index bede21be1..c6f4304cc 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -323,15 +323,6 @@ func Action(ctx *context.Context) { 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 { From 38e30f2a8f047cb11594b393d9fab8644c6735ea Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 25 Feb 2024 22:45:57 +0100 Subject: [PATCH 2/3] 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 --- 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() { From 7e6fe41389cd04a1e4c5688560a9c67ccbdf5d67 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Mon, 26 Feb 2024 09:14:11 +0100 Subject: [PATCH 3/3] Add tests for the star/unstar & watch/unwatch UI Signed-off-by: Gergely Nagy --- tests/integration/repo_starwatch_test.go | 82 ++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 tests/integration/repo_starwatch_test.go diff --git a/tests/integration/repo_starwatch_test.go b/tests/integration/repo_starwatch_test.go new file mode 100644 index 000000000..4b4f96772 --- /dev/null +++ b/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") +}