Files
Gitea/tests/integration/pull_review_test.go
T
Dominique Hummel 1f6cfe611a feat: Add block on pending codeowner reviews branch protection (#34995)
This commit introduces a new branch protection rule that allows merge
blocking if there are pending reviews from one or more code owners (as
defined in any valid `CODEOWNERS` file). This is determined by
evaluating each rule present in the `CODEOWNERS` file individually. For
every rule, at least one named code owner (or member of a code owner
team) must have given an approving review for merging to be possible.

Closes #32602 

---

This PR does NOT display code owners separately from other reviewers
(#28137)

### Screenshot

<details>
<summary>Pull Request</summary>
<img
src="https://github.com/user-attachments/assets/74560f9c-9f59-477a-b270-63f937b26d6a"
/>
</details>

<details>
<summary>Branch Protection Rule</summary>
<img
src="https://github.com/user-attachments/assets/f0a9ce00-38fe-4eed-b6a3-5290aa404610"
/>
</details>

Doc PR
https://gitea.com/gitea/docs/pulls/245

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: bircni <bircni@icloud.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
2026-07-31 13:00:59 +00:00

417 lines
17 KiB
Go

// Copyright 2019 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package integration
import (
"net/http"
"net/http/httptest"
"net/url"
"path"
"strings"
"testing"
"time"
"gitea.dev/models/db"
git_model "gitea.dev/models/git"
issues_model "gitea.dev/models/issues"
repo_model "gitea.dev/models/repo"
"gitea.dev/models/unittest"
user_model "gitea.dev/models/user"
"gitea.dev/modules/git"
"gitea.dev/modules/test"
issue_service "gitea.dev/services/issue"
pull_service "gitea.dev/services/pull"
repo_service "gitea.dev/services/repository"
files_service "gitea.dev/services/repository/files"
"gitea.dev/tests"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestPullView_ReviewerMissed(t *testing.T) {
defer tests.PrepareTestEnv(t)()
session := loginUser(t, "user1")
req := NewRequest(t, "GET", "/pulls")
resp := session.MakeRequest(t, req, http.StatusOK)
assert.True(t, test.IsNormalPageCompleted(resp.Body.String()))
req = NewRequest(t, "GET", "/user2/repo1/pulls/3")
resp = session.MakeRequest(t, req, http.StatusOK)
assert.True(t, test.IsNormalPageCompleted(resp.Body.String()))
// if some reviews are missing, the page shouldn't fail
err := db.TruncateBeans(t.Context(), &issues_model.Review{})
assert.NoError(t, err)
req = NewRequest(t, "GET", "/user2/repo1/pulls/2")
resp = session.MakeRequest(t, req, http.StatusOK)
assert.True(t, test.IsNormalPageCompleted(resp.Body.String()))
}
func TestPullView_CodeOwner(t *testing.T) {
onGiteaRun(t, func(t *testing.T, u *url.URL) {
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8})
// Create the repo.
repo, err := repo_service.CreateRepositoryDirectly(t.Context(), user2, user2, repo_service.CreateRepoOptions{
Name: "test_codeowner",
Readme: "Default",
AutoInit: true,
ObjectFormatName: git.Sha1ObjectFormat.Name(),
DefaultBranch: "master",
}, true)
assert.NoError(t, err)
// create code owner branch protection
protectBranch := git_model.ProtectedBranch{
BlockOnCodeownerReviews: true,
RepoID: repo.ID,
RuleName: "master",
CanPush: true,
}
err = pull_service.CreateOrUpdateProtectedBranch(t.Context(), repo, &protectBranch, git_model.WhitelistOptions{})
assert.NoError(t, err)
// add CODEOWNERS to default branch
_, err = files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
OldBranch: repo.DefaultBranch,
Files: []*files_service.ChangeRepoFile{
{
Operation: "create",
TreePath: "CODEOWNERS",
ContentReader: strings.NewReader("README.md @user5\nuser8-file.md @user8\n"),
},
},
})
assert.NoError(t, err)
t.Run("First Pull Request", func(t *testing.T) {
// create a new branch to prepare for pull request
_, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
NewBranch: "codeowner-basebranch",
Files: []*files_service.ChangeRepoFile{
{
Operation: "update",
TreePath: "README.md",
ContentReader: strings.NewReader("# This is a new project\n"),
},
},
})
assert.NoError(t, err)
// Create a pull request.
session := loginUser(t, "user2")
testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request")
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: repo.ID, HeadBranch: "codeowner-basebranch"})
unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
assert.NoError(t, pr.LoadIssue(t.Context()))
// capture the current PR head ref so we can wait for the async
// refs/pull/N/head sync triggered by the next push to complete
baseGitRepo, err := git.OpenRepository(t.Context(), repo)
require.NoError(t, err)
defer baseGitRepo.Close()
headRefBefore, err := baseGitRepo.GetRefCommitID(t.Context(), pr.GetGitHeadRefName())
require.NoError(t, err)
// update the file on the pr branch
resp, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
OldBranch: "codeowner-basebranch",
Files: []*files_service.ChangeRepoFile{
{
Operation: "create",
TreePath: "user8-file.md",
ContentReader: strings.NewReader("# This is a new project2\n"),
},
},
})
assert.NoError(t, err)
// refs/pull/N/head is refreshed asynchronously by the push hook; wait for
// it before evaluating code owners, otherwise the changed-file set may not
// yet include user8-file.md and the review request would be missed
require.Eventually(t, func() bool {
headRefAfter, err := baseGitRepo.GetRefCommitID(t.Context(), pr.GetGitHeadRefName())
return err == nil && headRefAfter != headRefBefore
}, 30*time.Second, 100*time.Millisecond)
reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(t.Context(), pr)
require.NoError(t, err)
require.Len(t, reviewNotifiers, 1)
assert.EqualValues(t, 8, reviewNotifiers[0].Reviewer.ID)
err = issue_service.ChangeTitle(t.Context(), pr.Issue, user2, "[WIP] Test Pull Request")
assert.NoError(t, err)
prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
assert.NoError(t, prUpdated1.LoadIssue(t.Context()))
assert.Equal(t, "[WIP] Test Pull Request", prUpdated1.Issue.Title)
err = issue_service.ChangeTitle(t.Context(), prUpdated1.Issue, user2, "Test Pull Request2")
assert.NoError(t, err)
prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
assert.NoError(t, prUpdated2.LoadIssue(t.Context()))
assert.Equal(t, "Test Pull Request2", prUpdated2.Issue.Title)
// ensure it cannot be merged
hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
_, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0))
assert.NoError(t, err)
// should still fail (we also need user8)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
_, _, err = issues_model.SubmitReview(t.Context(), user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0))
assert.NoError(t, err)
// now we should be able to merge
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.True(t, hasCodeownerReviews)
// a later requested-changes review from a required code owner must override
// that user's earlier approval and no longer satisfy the requirement
_, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeReject, "Needs changes", resp.Commit.SHA, false, make([]string, 0))
assert.NoError(t, err)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
})
t.Run("Dismissed Code Owner Review", func(t *testing.T) {
_, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
NewBranch: "codeowner-dismiss-branch",
Files: []*files_service.ChangeRepoFile{
{
Operation: "update",
TreePath: "README.md",
ContentReader: strings.NewReader("# New README content\n"),
},
},
})
assert.NoError(t, err)
session := loginUser(t, "user2")
testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-dismiss-branch", "Test Code Owner Dismissal")
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-dismiss-branch"})
unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
_, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeApprove, " LGTM", "", false, make([]string, 0))
assert.NoError(t, err)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.True(t, hasCodeownerReviews)
review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, ReviewerID: 5, Type: issues_model.ReviewTypeApprove})
assert.False(t, review.Dismissed)
err = issues_model.DismissReview(t.Context(), review, true)
assert.NoError(t, err)
review, err = issues_model.GetReviewByID(t.Context(), review.ID)
assert.NoError(t, err)
assert.True(t, review.Dismissed)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
})
// change the default branch CODEOWNERS file to change README.md's codeowner
_, err = files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
Files: []*files_service.ChangeRepoFile{
{
Operation: "update",
TreePath: "CODEOWNERS",
ContentReader: strings.NewReader("README.md @user8\n"),
},
},
})
assert.NoError(t, err)
t.Run("Second Pull Request", func(t *testing.T) {
// create a new branch to prepare for pull request
resp, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
NewBranch: "codeowner-basebranch2",
Files: []*files_service.ChangeRepoFile{
{
Operation: "update",
TreePath: "README.md",
ContentReader: strings.NewReader("# This is a new project2\n"),
},
},
})
assert.NoError(t, err)
// Create a pull request.
session := loginUser(t, "user2")
testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2")
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"})
unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
// should need user8 approval only now
hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
// exercise the real merge-time gate, not just the helper: the persisted
// protection rule must block the merge on the missing code owner review
err = pull_service.CheckPullBranchProtections(t.Context(), pr, false)
assert.ErrorContains(t, err, "code owner")
_, _, err = issues_model.SubmitReview(t.Context(), user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0))
assert.NoError(t, err)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.True(t, hasCodeownerReviews)
// and once the code owner has approved, the merge gate no longer blocks
assert.NoError(t, pull_service.CheckPullBranchProtections(t.Context(), pr, false))
})
t.Run("Forked Repo Pull Request", func(t *testing.T) {
user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
forkedRepo, err := repo_service.ForkRepository(t.Context(), user2, user5, repo_service.ForkRepoOptions{
BaseRepo: repo,
Name: "test_codeowner",
})
assert.NoError(t, err)
// create a new branch to prepare for pull request
resp, err := files_service.ChangeRepoFiles(t.Context(), forkedRepo, user5, &files_service.ChangeRepoFilesOptions{
NewBranch: "codeowner-basebranch-forked",
Files: []*files_service.ChangeRepoFile{
{
Operation: "update",
TreePath: "README.md",
ContentReader: strings.NewReader("# This is a new forked project\n"),
},
},
})
assert.NoError(t, err)
session := loginUser(t, "user5")
// create a pull request on the forked repository, code reviewers should not be mentioned
testPullCreateDirectly(t, session, createPullRequestOptions{
BaseRepoOwner: "user5",
BaseRepoName: "test_codeowner",
BaseBranch: forkedRepo.DefaultBranch,
HeadRepoOwner: "",
HeadRepoName: "",
HeadBranch: "codeowner-basebranch-forked",
Title: "Test Pull Request on Forked Repository",
})
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"})
unittest.AssertNotExistsBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
// create a pull request to base repository, code reviewers should be mentioned
testPullCreateDirectly(t, session, createPullRequestOptions{
BaseRepoOwner: repo.OwnerName,
BaseRepoName: repo.Name,
BaseBranch: repo.DefaultBranch,
HeadRepoOwner: forkedRepo.OwnerName,
HeadRepoName: forkedRepo.Name,
HeadBranch: "codeowner-basebranch-forked",
Title: "Test Pull Request3",
})
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"})
unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
// will also need user8 for this
hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
_, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0))
assert.NoError(t, err)
// should still fail (user5 is not a code owner for this PR)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
_, _, err = issues_model.SubmitReview(t.Context(), user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0))
assert.NoError(t, err)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.True(t, hasCodeownerReviews)
})
})
}
func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
user1Session := loginUser(t, "user1")
user2Session := loginUser(t, "user2")
// Have user1 create a fork of repo1.
testRepoFork(t, user1Session, "user2", "repo1", "user1", "repo1", "")
t.Run("Submit approve/reject review on merged PR", func(t *testing.T) {
// Create a merged PR (made by user1) in the upstream repo1.
testEditFile(t, user1Session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "master", "This is a pull title")
elem := strings.Split(test.RedirectURL(resp), "/")
assert.Equal(t, "pulls", elem[3])
testPullMerge(t, user1Session, elem[1], elem[2], elem[4], MergeOptions{
Style: repo_model.MergeStyleMerge,
DeleteBranch: false,
})
// Submit an approve review on the PR.
testSubmitReview(t, user2Session, "user2", "repo1", elem[4], "", "approve", http.StatusUnprocessableEntity)
// Submit a reject review on the PR.
testSubmitReview(t, user2Session, "user2", "repo1", elem[4], "", "reject", http.StatusUnprocessableEntity)
})
t.Run("Submit approve/reject review on closed PR", func(t *testing.T) {
// Created a closed PR (made by user1) in the upstream repo1.
testEditFileToNewBranch(t, user1Session, "user1", "repo1", "master", "a-test-branch", "README.md", "Hello, World (Edited...again)\n")
resp := testPullCreate(t, user1Session, "user1", "repo1", false, "master", "a-test-branch", "This is a pull title")
elem := strings.Split(test.RedirectURL(resp), "/")
assert.Equal(t, "pulls", elem[3])
testIssueClose(t, user1Session, elem[1], elem[2], elem[4])
// Submit an approve review on the PR.
testSubmitReview(t, user2Session, "user2", "repo1", elem[4], "", "approve", http.StatusUnprocessableEntity)
// Submit a reject review on the PR.
testSubmitReview(t, user2Session, "user2", "repo1", elem[4], "", "reject", http.StatusUnprocessableEntity)
})
})
}
func testSubmitReview(t *testing.T, session *TestSession, owner, repo, pullNumber, commitID, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder {
options := map[string]string{
"commit_id": commitID,
"content": "test",
"type": reviewType,
}
submitURL := "/" + path.Join(owner, repo, "pulls", pullNumber, "files", "reviews", "submit")
req := NewRequestWithValues(t, "POST", submitURL, options)
return session.MakeRequest(t, req, expectedSubmitStatus)
}
func testIssueClose(t *testing.T, session *TestSession, owner, repo, issueNumber string) *httptest.ResponseRecorder {
closeURL := "/" + path.Join(owner, repo, "issues", issueNumber, "comments")
options := map[string]string{
"status": "close",
}
req := NewRequestWithValues(t, "POST", closeURL, options)
return session.MakeRequest(t, req, http.StatusOK)
}