From 5df3166510d3aecb44493633f6b691306bb77926 Mon Sep 17 00:00:00 2001 From: Naxdy Date: Fri, 11 Jul 2025 22:17:34 +0200 Subject: [PATCH] Amend code owner test to include branch protection rule --- services/issue/pull.go | 9 ++-- tests/integration/pull_review_test.go | 61 +++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/services/issue/pull.go b/services/issue/pull.go index b5bc8a800e..3096fce5f3 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -61,6 +61,10 @@ func HasAllRequiredCodeownerReviews(ctx context.Context, pb *git_model.Protected return false } + if err := pr.LoadIssue(ctx); err != nil { + return false + } + pr.Issue.Repo = pr.BaseRepo if pr.BaseRepo.IsFork { @@ -114,9 +118,8 @@ func HasAllRequiredCodeownerReviews(ctx context.Context, pb *git_model.Protected } reviews, err := issues_model.FindLatestReviews(ctx, issues_model.FindReviewOptions{ - Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove}, - IssueID: pr.IssueID, - OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly, + Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove}, + IssueID: pr.IssueID, }) if err != nil { return false diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 1121751c88..fdbcf0daa8 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -12,6 +12,7 @@ import ( "testing" "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" @@ -19,6 +20,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/test" issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" @@ -49,6 +51,8 @@ func TestPullView_ReviewerMissed(t *testing.T) { 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(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ @@ -60,6 +64,17 @@ func TestPullView_CodeOwner(t *testing.T) { }, 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(db.DefaultContext, repo, &protectBranch, git_model.WhitelistOptions{}) + assert.NoError(t, err) + // add CODEOWNERS to default branch _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ OldBranch: repo.DefaultBranch, @@ -96,7 +111,7 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, pr.LoadIssue(db.DefaultContext)) // update the file on the pr branch - _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + resp, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ OldBranch: "codeowner-basebranch", Files: []*files_service.ChangeRepoFile{ { @@ -124,6 +139,22 @@ func TestPullView_CodeOwner(t *testing.T) { prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.NoError(t, prUpdated2.LoadIssue(db.DefaultContext)) assert.Equal(t, "Test Pull Request2", prUpdated2.Issue.Title) + + // ensure it cannot be merged + hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(db.DefaultContext, &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + issues_model.SubmitReview(db.DefaultContext, user5, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + + // should still fail (we also need user8) + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(db.DefaultContext, &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + issues_model.SubmitReview(db.DefaultContext, user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + + // now we should be able to merge + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(db.DefaultContext, &protectBranch, pr) + assert.True(t, hasCodeownerReviews) }) // change the default branch CODEOWNERS file to change README.md's codeowner @@ -140,7 +171,7 @@ func TestPullView_CodeOwner(t *testing.T) { t.Run("Second Pull Request", func(t *testing.T) { // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + resp, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch2", Files: []*files_service.ChangeRepoFile{ { @@ -158,6 +189,15 @@ func TestPullView_CodeOwner(t *testing.T) { 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(db.DefaultContext, &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + issues_model.SubmitReview(db.DefaultContext, user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(db.DefaultContext, &protectBranch, pr) + assert.True(t, hasCodeownerReviews) }) t.Run("Forked Repo Pull Request", func(t *testing.T) { @@ -169,7 +209,7 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, err) // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ + resp, err := files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch-forked", Files: []*files_service.ChangeRepoFile{ { @@ -194,6 +234,21 @@ func TestPullView_CodeOwner(t *testing.T) { 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(db.DefaultContext, &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + issues_model.SubmitReview(db.DefaultContext, user5, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + + // should still fail (user5 is not a code owner for this PR) + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(db.DefaultContext, &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + issues_model.SubmitReview(db.DefaultContext, user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(db.DefaultContext, &protectBranch, pr) + assert.True(t, hasCodeownerReviews) }) }) }