From bc7e448d4940e854dc6d43ac4d4670c5d0588e8c Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 21 Jan 2024 15:18:24 +0100 Subject: [PATCH] [GITEA] Rework when recently pushed branches are displayed With this change, the "You pushed on branch xyz" banner will be displayed when either the viewed repository or its base repo (if the current one's a fork) has pull requests enabled. Previously it only displayed if the viewed repo had PRs enabled. Furthermore, if the viewed repository is an original repository that the viewing user has a fork of, if the forked repository has recently pushed branches, then the banner will appear for the original repository too. In this case, the notification will include branches from the viewing user's fork, and branches they pushed to the base repo, too. Refs: https://codeberg.org/forgejo/forgejo/pulls/2195 Signed-off-by: Gergely Nagy (cherry picked from commit a29f10661d59f6c33c5cfbee723f03f981aa6b72) (cherry picked from commit 70c5e2021d7c385b9285622f0b2d878d3807d33c) (cherry picked from commit 48b25be67a94f739601fbfea951ade6cdfe30691) --- models/git/branch.go | 4 + routers/web/repo/view.go | 41 ++++- .../code/recently_pushed_new_branches.tmpl | 11 +- tests/integration/pull_create_test.go | 171 ++++++++++++++++++ 4 files changed, 215 insertions(+), 12 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index db02fc9b2..a96d7ff8c 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -128,6 +128,10 @@ func (b *Branch) LoadDeletedBy(ctx context.Context) (err error) { return err } +func (b *Branch) GetRepo(ctx context.Context) (*repo_model.Repository, error) { + return repo_model.GetRepositoryByID(ctx, b.RepoID) +} + func (b *Branch) LoadPusher(ctx context.Context) (err error) { if b.Pusher == nil && b.PusherID > 0 { b.Pusher, err = user_model.GetUserByID(ctx, b.PusherID) diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index 114822844..9c606c3bc 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -1035,20 +1035,43 @@ func renderCode(ctx *context.Context) { return } - showRecentlyPushedNewBranches := true - if ctx.Repo.Repository.IsMirror || - !ctx.Repo.Repository.UnitEnabled(ctx, unit_model.TypePullRequests) { - showRecentlyPushedNewBranches = false + // If the repo is a mirror, don't display recently pushed branches. + if ctx.Repo.Repository.IsMirror { + goto PostRecentBranchCheck } - if showRecentlyPushedNewBranches { - ctx.Data["RecentlyPushedNewBranches"], err = git_model.FindRecentlyPushedNewBranches(ctx, ctx.Repo.Repository.ID, ctx.Doer.ID, ctx.Repo.Repository.DefaultBranch) - if err != nil { - ctx.ServerError("GetRecentlyPushedBranches", err) - return + + // If pull requests aren't enabled for either the current repo, or its + // base, don't display recently pushed branches. + if !(ctx.Repo.Repository.AllowsPulls(ctx) || + (ctx.Repo.Repository.BaseRepo != nil && ctx.Repo.Repository.BaseRepo.AllowsPulls(ctx))) { + goto PostRecentBranchCheck + } + + // Find recently pushed new branches to *this* repo. + branches, err := git_model.FindRecentlyPushedNewBranches(ctx, ctx.Repo.Repository.ID, ctx.Doer.ID, ctx.Repo.Repository.DefaultBranch) + if err != nil { + ctx.ServerError("FindRecentlyPushedBranches", err) + return + } + + // If this is not a fork, check if the signed in user has a fork, and + // check branches there. + if !ctx.Repo.Repository.IsFork { + repo := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) + if repo != nil { + baseBranches, err := git_model.FindRecentlyPushedNewBranches(ctx, repo.ID, ctx.Doer.ID, repo.DefaultBranch) + if err != nil { + ctx.ServerError("FindRecentlyPushedBranches", err) + return + } + branches = append(branches, baseBranches...) } } + + ctx.Data["RecentlyPushedNewBranches"] = branches } +PostRecentBranchCheck: var treeNames []string paths := make([]string, 0, 5) if len(ctx.Repo.TreePath) > 0 { diff --git a/templates/repo/code/recently_pushed_new_branches.tmpl b/templates/repo/code/recently_pushed_new_branches.tmpl index 2b9948d21..176ae3df0 100644 --- a/templates/repo/code/recently_pushed_new_branches.tmpl +++ b/templates/repo/code/recently_pushed_new_branches.tmpl @@ -2,10 +2,15 @@
{{$timeSince := TimeSince .CommitTime.AsTime ctx.Locale}} - {{$branchLink := (print $.RepoLink "/src/branch/" (PathEscapeSegments .Name))}} - {{ctx.Locale.Tr "repo.pulls.recently_pushed_new_branches" (Escape .Name) $timeSince $branchLink | Safe}} + {{$repo := .GetRepo $.Context}} + {{$name := .Name}} + {{if ne $repo.ID $.Repository.ID}} + {{$name = (print $repo.FullName ":" .Name)}} + {{end}} + {{$branchLink := (print ($repo.Link) "/src/branch/" (PathEscapeSegments .Name))}} + {{ctx.Locale.Tr "repo.pulls.recently_pushed_new_branches" (Escape $name) $timeSince $branchLink | Safe}}
- + {{ctx.Locale.Tr "repo.pulls.compare_changes"}}
diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index 01965ecbe..ba52b5a8d 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -1,4 +1,5 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved. // SPDX-License-Identifier: MIT package integration @@ -12,7 +13,11 @@ import ( "strings" "testing" + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/test" + repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -175,3 +180,169 @@ func TestPullBranchDelete(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) }) } + +func TestRecentlyPushed(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + + testCreateBranch(t, session, "user1", "repo1", "branch/master", "recent-push", http.StatusSeeOther) + testEditFile(t, session, "user1", "repo1", "recent-push", "README.md", "Hello recently!\n") + + testCreateBranch(t, session, "user2", "repo1", "branch/master", "recent-push-base", http.StatusSeeOther) + testEditFile(t, session, "user2", "repo1", "recent-push-base", "README.md", "Hello, recently, from base!\n") + + baseRepo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, "user2", "repo1") + assert.NoError(t, err) + repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, "user1", "repo1") + assert.NoError(t, err) + + enablePRs := func(t *testing.T, repo *repo_model.Repository) { + t.Helper() + + err := repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, + []repo_model.RepoUnit{{ + RepoID: repo.ID, + Type: unit_model.TypePullRequests, + }}, + nil) + assert.NoError(t, err) + } + + disablePRs := func(t *testing.T, repo *repo_model.Repository) { + t.Helper() + + err := repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, nil, + []unit_model.Type{unit_model.TypePullRequests}) + assert.NoError(t, err) + } + + testBanner := func(t *testing.T) { + t.Helper() + + req := NewRequest(t, "GET", "/user1/repo1") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + message := strings.TrimSpace(htmlDoc.Find(".ui.message").Text()) + link, _ := htmlDoc.Find(".ui.message a").Attr("href") + expectedMessage := "You pushed on branch recent-push" + + assert.Contains(t, message, expectedMessage) + assert.Equal(t, "/user1/repo1/src/branch/recent-push", link) + } + + // Test that there's a recently pushed branches banner, and it contains + // a link to the branch. + t.Run("recently-pushed-banner", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + testBanner(t) + }) + + // Test that it is still there if the fork has PRs disabled, but the + // base repo still has them enabled. + t.Run("with-fork-prs-disabled", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + defer func() { + enablePRs(t, repo) + }() + + disablePRs(t, repo) + testBanner(t) + }) + + // Test that it is still there if the fork has PRs enabled, but the base + // repo does not. + t.Run("with-base-prs-disabled", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + defer func() { + enablePRs(t, baseRepo) + }() + + disablePRs(t, baseRepo) + testBanner(t) + }) + + // Test that the banner is not present if both the base and current + // repo have PRs disabled. + t.Run("with-prs-disabled", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + defer func() { + enablePRs(t, baseRepo) + enablePRs(t, repo) + }() + + disablePRs(t, repo) + disablePRs(t, baseRepo) + + req := NewRequest(t, "GET", "/user1/repo1") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, ".ui.message", false) + }) + + // Test that visiting the base repo has the banner too, and includes + // recent push notifications from both the fork, and the base repo. + t.Run("on the base repo", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Count recently pushed branches on the fork + req := NewRequest(t, "GET", "/user1/repo1") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, ".ui.message", true) + + // Count recently pushed branches on the base repo + req = NewRequest(t, "GET", "/user2/repo1") + resp = session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + messageCountOnBase := htmlDoc.Find(".ui.message").Length() + + // We have two messages on the base: one from the fork, one on the + // base itself. + assert.Equal(t, 2, messageCountOnBase) + }) + + // Test that the banner's links point to the right repos + t.Run("link validity", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // We're testing against the origin repo, because that has both + // local branches, and another from a fork, so we can test both in + // one test! + + req := NewRequest(t, "GET", "/user2/repo1") + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + messages := htmlDoc.Find(".ui.message") + + prButtons := messages.Find("a[role='button']") + branchLinks := messages.Find("a[href*='/src/branch/']") + + // ** base repo tests ** + basePRLink, _ := prButtons.First().Attr("href") + baseBranchLink, _ := branchLinks.First().Attr("href") + baseBranchName := branchLinks.First().Text() + + // branch in the same repo does not have a `user/repo:` qualifier. + assert.Equal(t, "recent-push-base", baseBranchName) + // branch link points to the same repo + assert.Equal(t, "/user2/repo1/src/branch/recent-push-base", baseBranchLink) + // PR link compares against the correct rep, and unqualified branch name + assert.Equal(t, "/user2/repo1/compare/master...recent-push-base", basePRLink) + + // ** forked repo tests ** + forkPRLink, _ := prButtons.Last().Attr("href") + forkBranchLink, _ := branchLinks.Last().Attr("href") + forkBranchName := branchLinks.Last().Text() + + // branch in the forked repo has a `user/repo:` qualifier. + assert.Equal(t, "user1/repo1:recent-push", forkBranchName) + // branch link points to the forked repo + assert.Equal(t, "/user1/repo1/src/branch/recent-push", forkBranchLink) + // PR link compares against the correct rep, and qualified branch name + assert.Equal(t, "/user2/repo1/compare/master...user1/repo1:recent-push", forkPRLink) + }) + }) +}