From d30950d803bd74a8b446266145f27c8656d75ef0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 15 Mar 2024 13:13:09 +0800 Subject: [PATCH 1/6] Fix codeowner detected diff base branch to mergebase (#29783) Fix #29763 This PR fixes 2 problems with CodeOwner in the pull request. - Don't use the pull request base branch but merge-base as a diff base to detect the code owner. - CodeOwner detection in fork repositories will be disabled because almost all the fork repositories will not change CODEOWNERS files but it should not be used on fork repositories' pull requests. --------- Co-authored-by: wxiaoguang --- models/issues/pull.go | 84 +---------------- routers/web/repo/issue.go | 2 +- routers/web/repo/pull.go | 2 +- services/convert/pull.go | 2 +- services/issue/issue.go | 2 +- services/issue/pull.go | 121 +++++++++++++++++++++++++ services/mailer/mail_issue.go | 2 +- services/mailer/notify.go | 2 +- services/pull/check.go | 2 +- services/pull/pull.go | 4 +- services/uinotification/notify.go | 2 +- tests/integration/pull_review_test.go | 124 ++++++++++++++++++++++++++ 12 files changed, 259 insertions(+), 90 deletions(-) create mode 100644 services/issue/pull.go diff --git a/models/issues/pull.go b/models/issues/pull.go index 3ca0847652f34..b6ac47aad209e 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -706,8 +706,8 @@ func (pr *PullRequest) UpdateColsIfNotMerged(ctx context.Context, cols ...string // IsWorkInProgress determine if the Pull Request is a Work In Progress by its title // Issue must be set before this method can be called. -func (pr *PullRequest) IsWorkInProgress() bool { - if err := pr.LoadIssue(db.DefaultContext); err != nil { +func (pr *PullRequest) IsWorkInProgress(ctx context.Context) bool { + if err := pr.LoadIssue(ctx); err != nil { log.Error("LoadIssue: %v", err) return false } @@ -810,14 +810,14 @@ func UpdateAllowEdits(ctx context.Context, pr *PullRequest) error { } // Mergeable returns if the pullrequest is mergeable. -func (pr *PullRequest) Mergeable() bool { +func (pr *PullRequest) Mergeable(ctx context.Context) bool { // If a pull request isn't mergable if it's: // - Being conflict checked. // - Has a conflict. // - Received a error while being conflict checked. // - Is a work-in-progress pull request. return pr.Status != PullRequestStatusChecking && pr.Status != PullRequestStatusConflict && - pr.Status != PullRequestStatusError && !pr.IsWorkInProgress() + pr.Status != PullRequestStatusError && !pr.IsWorkInProgress(ctx) } // HasEnoughApprovals returns true if pr has enough granted approvals. @@ -887,82 +887,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr * return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0 } -func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullRequest) error { - files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - - if pr.IsWorkInProgress() { - return nil - } - - if err := pr.LoadBaseRepo(ctx); err != nil { - return err - } - - repo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) - if err != nil { - return err - } - defer repo.Close() - - branch, err := repo.GetDefaultBranch() - if err != nil { - return err - } - - commit, err := repo.GetBranchCommit(branch) - if err != nil { - return err - } - - var data string - for _, file := range files { - if blob, err := commit.GetBlobByPath(file); err == nil { - data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) - if err == nil { - break - } - } - } - - rules, _ := GetCodeOwnersFromContent(ctx, data) - changedFiles, err := repo.GetFilesChangedBetween(git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) - if err != nil { - return err - } - - uniqUsers := make(map[int64]*user_model.User) - uniqTeams := make(map[string]*org_model.Team) - for _, rule := range rules { - for _, f := range changedFiles { - if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) { - for _, u := range rule.Users { - uniqUsers[u.ID] = u - } - for _, t := range rule.Teams { - uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t - } - } - } - } - - for _, u := range uniqUsers { - if u.ID != pull.Poster.ID { - if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil { - log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) - return err - } - } - } - for _, t := range uniqTeams { - if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil { - log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) - return err - } - } - - return nil -} - // GetCodeOwnersFromContent returns the code owners configuration // Return empty slice if files missing // Return warning messages on parsing errors diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 4d70b485fe1b8..88f745f91233c 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1910,7 +1910,7 @@ func ViewIssue(ctx *context.Context) { if pull.HasMerged || issue.IsClosed || !ctx.IsSigned { return false } - if pull.CanAutoMerge() || pull.IsWorkInProgress() || pull.IsChecking() { + if pull.CanAutoMerge() || pull.IsWorkInProgress(ctx) || pull.IsChecking() { return false } if (ctx.Doer.IsAdmin || ctx.Repo.IsAdmin()) && prConfig.AllowManualMerge { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 308841b172340..5cf9967598c48 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -706,7 +706,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C ctx.Data["IsNothingToCompare"] = true } - if pull.IsWorkInProgress() { + if pull.IsWorkInProgress(ctx) { ctx.Data["IsPullWorkInProgress"] = true ctx.Data["WorkInProgressPrefix"] = pull.GetWorkInProgressPrefix(ctx) } diff --git a/services/convert/pull.go b/services/convert/pull.go index e4e3097056a2b..7eebe20426c31 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -68,7 +68,7 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u PatchURL: pr.Issue.PatchURL(), HasMerged: pr.HasMerged, MergeBase: pr.MergeBase, - Mergeable: pr.Mergeable(), + Mergeable: pr.Mergeable(ctx), Deadline: apiIssue.Deadline, Created: pr.Issue.CreatedUnix.AsTimePtr(), Updated: pr.Issue.UpdatedUnix.AsTimePtr(), diff --git a/services/issue/issue.go b/services/issue/issue.go index 8a08baf2ba4ab..10aec1288f101 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -71,7 +71,7 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode } if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { - if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil { + if err := PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil { return err } } diff --git a/services/issue/pull.go b/services/issue/pull.go new file mode 100644 index 0000000000000..94020de42da6d --- /dev/null +++ b/services/issue/pull.go @@ -0,0 +1,121 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issue + +import ( + "context" + "fmt" + "time" + + issues_model "code.gitea.io/gitea/models/issues" + org_model "code.gitea.io/gitea/models/organization" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" +) + +func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) { + // Add a temporary remote + tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano()) + if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil { + return "", fmt.Errorf("AddRemote: %w", err) + } + defer func() { + if err := repo.RemoveRemote(tmpRemote); err != nil { + log.Error("getMergeBase: RemoveRemote: %v", err) + } + }() + + mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch) + return mergeBase, err +} + +func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, pr *issues_model.PullRequest) error { + files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} + + if pr.IsWorkInProgress(ctx) { + return nil + } + + if err := pr.LoadHeadRepo(ctx); err != nil { + return err + } + + if pr.HeadRepo.IsFork { + return nil + } + + if err := pr.LoadBaseRepo(ctx); err != nil { + return err + } + + repo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) + if err != nil { + return err + } + defer repo.Close() + + commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch) + if err != nil { + return err + } + + var data string + for _, file := range files { + if blob, err := commit.GetBlobByPath(file); err == nil { + data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) + if err == nil { + break + } + } + } + + rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) + + // get the mergebase + mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) + if err != nil { + return err + } + + // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed + // between the merge base and the head commit but not the base branch and the head commit + changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.HeadCommitID) + if err != nil { + return err + } + + uniqUsers := make(map[int64]*user_model.User) + uniqTeams := make(map[string]*org_model.Team) + for _, rule := range rules { + for _, f := range changedFiles { + if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) { + for _, u := range rule.Users { + uniqUsers[u.ID] = u + } + for _, t := range rule.Teams { + uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t + } + } + } + } + + for _, u := range uniqUsers { + if u.ID != pull.Poster.ID { + if _, err := issues_model.AddReviewRequest(ctx, pull, u, pull.Poster); err != nil { + log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) + return err + } + } + } + for _, t := range uniqTeams { + if _, err := issues_model.AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil { + log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) + return err + } + } + + return nil +} diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index 8a00be6926a87..fab3315be216e 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -82,7 +82,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_mo // =========== Repo watchers =========== // Make repo watchers last, since it's likely the list with the most users - if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress() && ctx.ActionType != activities_model.ActionCreatePullRequest) { + if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress(ctx) && ctx.ActionType != activities_model.ActionCreatePullRequest) { ids, err = repo_model.GetRepoWatchersIDs(ctx, ctx.Issue.RepoID) if err != nil { return fmt.Errorf("GetRepoWatchersIDs(%d): %w", ctx.Issue.RepoID, err) diff --git a/services/mailer/notify.go b/services/mailer/notify.go index 9eaf268d0ac89..cc4e6baf0b3a2 100644 --- a/services/mailer/notify.go +++ b/services/mailer/notify.go @@ -79,7 +79,7 @@ func (m *mailNotifier) IssueChangeTitle(ctx context.Context, doer *user_model.Us log.Error("issue.LoadPullRequest: %v", err) return } - if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { + if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress(ctx) { if err := MailParticipants(ctx, issue, doer, activities_model.ActionPullRequestReadyForReview, nil); err != nil { log.Error("MailParticipants: %v", err) } diff --git a/services/pull/check.go b/services/pull/check.go index e4a0f6b27bb79..21ec40a0c7041 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -91,7 +91,7 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce return nil } - if pr.IsWorkInProgress() { + if pr.IsWorkInProgress(ctx) { return ErrIsWorkInProgress } diff --git a/services/pull/pull.go b/services/pull/pull.go index 63f39b3dca8ce..020c80add36a7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -126,8 +126,8 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss return err } - if !pr.IsWorkInProgress() { - if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil { + if !pr.IsWorkInProgress(ctx) { + if err := issue_service.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil { return err } } diff --git a/services/uinotification/notify.go b/services/uinotification/notify.go index 9230d1ee6cee7..be5f7019a2e11 100644 --- a/services/uinotification/notify.go +++ b/services/uinotification/notify.go @@ -114,7 +114,7 @@ func (ns *notificationService) IssueChangeTitle(ctx context.Context, doer *user_ log.Error("issue.LoadPullRequest: %v", err) return } - if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { + if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress(ctx) { _ = ns.issueQueue.Push(issueNotificationOpts{ IssueID: issue.ID, NotificationAuthorID: doer.ID, diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 68d80a102157d..7166a740ab148 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -5,9 +5,20 @@ package integration import ( "net/http" + "net/url" + "strings" "testing" + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + repo_service "code.gitea.io/gitea/services/repository" + files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" ) func TestPullView_ReviewerMissed(t *testing.T) { @@ -20,3 +31,116 @@ func TestPullView_ReviewerMissed(t *testing.T) { req = NewRequest(t, "GET", "/user2/repo1/pulls/3") session.MakeRequest(t, req, http.StatusOK) } + +func TestPullView_CodeOwner(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // Create the repo. + repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ + Name: "test_codeowner", + Readme: "Default", + AutoInit: true, + ObjectFormatName: git.Sha1ObjectFormat.Name(), + DefaultBranch: "master", + }) + assert.NoError(t, err) + + // add CODEOWNERS to default branch + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + OldBranch: repo.DefaultBranch, + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "CODEOWNERS", + ContentReader: strings.NewReader("README.md @user5\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(db.DefaultContext, 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, HeadBranch: "codeowner-basebranch"}) + unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + }) + + // change the default branch CODEOWNERS file to change README.md's codeowner + _, err = files_service.ChangeRepoFiles(db.DefaultContext, 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 + _, err = files_service.ChangeRepoFiles(db.DefaultContext, 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.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + }) + + t.Run("Forked Repo Pull Request", func(t *testing.T) { + user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + forkedRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{ + BaseRepo: repo, + Name: "test_codeowner_fork", + }) + assert.NoError(t, err) + + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, 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") + testPullCreate(t, session, "user5", "test_codeowner_fork", false, forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2") + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch-forked"}) + unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + }) + }) +} From dbeb2d0bd9963682ec7cd90790b14043991ff496 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 15 Mar 2024 18:14:39 +0800 Subject: [PATCH 2/6] Fix lint --- models/issues/pull_test.go | 6 +++--- tests/integration/pull_merge_test.go | 2 +- tests/integration/pull_review_test.go | 16 +++++++--------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 3636263c46d4b..7307de9002a0f 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -260,13 +260,13 @@ func TestPullRequest_IsWorkInProgress(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) pr.LoadIssue(db.DefaultContext) - assert.False(t, pr.IsWorkInProgress()) + assert.False(t, pr.IsWorkInProgress(db.DefaultContext)) pr.Issue.Title = "WIP: " + pr.Issue.Title - assert.True(t, pr.IsWorkInProgress()) + assert.True(t, pr.IsWorkInProgress(db.DefaultContext)) pr.Issue.Title = "[wip]: " + pr.Issue.Title - assert.True(t, pr.IsWorkInProgress()) + assert.True(t, pr.IsWorkInProgress(db.DefaultContext)) } func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) { diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 42bf584f58429..2853b9f899568 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -432,6 +432,6 @@ func TestConflictChecking(t *testing.T) { // Check if status is correct. assert.Equal(t, issues_model.PullRequestStatusConflict, conflictingPR.Status) // Ensure that mergeable returns false - assert.False(t, conflictingPR.Mergeable()) + assert.False(t, conflictingPR.Mergeable(db.DefaultContext)) }) } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 7166a740ab148..af95cb15a04cc 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -13,7 +13,6 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" @@ -38,11 +37,10 @@ func TestPullView_CodeOwner(t *testing.T) { // Create the repo. repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ - Name: "test_codeowner", - Readme: "Default", - AutoInit: true, - ObjectFormatName: git.Sha1ObjectFormat.Name(), - DefaultBranch: "master", + Name: "test_codeowner", + Readme: "Default", + AutoInit: true, + DefaultBranch: "master", }) assert.NoError(t, err) @@ -75,7 +73,7 @@ func TestPullView_CodeOwner(t *testing.T) { // Create a pull request. session := loginUser(t, "user2") - testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request") + testPullCreate(t, session, "user2", "test_codeowner", "codeowner-basebranch", "Test Pull Request") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) @@ -109,7 +107,7 @@ func TestPullView_CodeOwner(t *testing.T) { // Create a pull request. session := loginUser(t, "user2") - testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2") + testPullCreate(t, session, "user2", "test_codeowner", "codeowner-basebranch2", "Test Pull Request2") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"}) unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) @@ -137,7 +135,7 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, err) session := loginUser(t, "user5") - testPullCreate(t, session, "user5", "test_codeowner_fork", false, forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2") + testPullCreate(t, session, "user5", "test_codeowner_fork", "codeowner-basebranch-forked", "Test Pull Request2") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) From 27c15e22e50c421e6f57186452c111868823790e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 15 Mar 2024 18:16:34 +0800 Subject: [PATCH 3/6] Fix test --- templates/shared/issueicon.tmpl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/shared/issueicon.tmpl b/templates/shared/issueicon.tmpl index 2a2e70085a79f..b8c03c6cb6894 100644 --- a/templates/shared/issueicon.tmpl +++ b/templates/shared/issueicon.tmpl @@ -7,9 +7,9 @@ {{if .IsClosed}} {{svg "octicon-git-pull-request" 16 "text red"}} {{else}} - {{if and .PullRequest .PullRequest.IsWorkInProgress}} + {{if and .PullRequest .PullRequest.IsWorkInProgress ctx}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}} - {{else if and .GetPullRequest .GetPullRequest.IsWorkInProgress}} + {{else if and .GetPullRequest .GetPullRequest.IsWorkInProgress ctx}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}} {{else}} {{svg "octicon-git-pull-request" 16 "text green"}} From 9278e3cbe2ba9364b064d6c715d84f394a077422 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 16 Mar 2024 17:57:36 +0800 Subject: [PATCH 4/6] Fix test --- tests/integration/pull_create_test.go | 8 +++----- tests/integration/pull_review_test.go | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index a6ee0d9dfa1ef..f36ca804958f1 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -4,11 +4,11 @@ package integration import ( + "fmt" "net/http" "net/http/httptest" "net/url" "path" - "strings" "testing" "code.gitea.io/gitea/modules/test" @@ -23,12 +23,10 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch, titl // Click the PR button to create a pull htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find("#new-pull-request").Attr("href") + _, exists := htmlDoc.doc.Find("#new-pull-request").Attr("href") assert.True(t, exists, "The template has changed") - if branch != "master" { - link = strings.Replace(link, ":master", ":"+branch, 1) - } + link := fmt.Sprintf("/%s/%s/compare/master...%s", user, repo, branch) req = NewRequest(t, "GET", link) resp = session.MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index af95cb15a04cc..457641cedcb4e 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -137,7 +137,7 @@ func TestPullView_CodeOwner(t *testing.T) { session := loginUser(t, "user5") testPullCreate(t, session, "user5", "test_codeowner_fork", "codeowner-basebranch-forked", "Test Pull Request2") - pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch-forked"}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) }) }) From c96d09b9d4561d646ccb02652335c642e5e1a99d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 16 Mar 2024 19:43:12 +0800 Subject: [PATCH 5/6] Fix test --- tests/integration/pull_review_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 457641cedcb4e..7fafa06eb0d3c 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -117,7 +117,7 @@ func TestPullView_CodeOwner(t *testing.T) { user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) forkedRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{ BaseRepo: repo, - Name: "test_codeowner_fork", + Name: "test_codeowner", }) assert.NoError(t, err) @@ -135,7 +135,7 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, err) session := loginUser(t, "user5") - testPullCreate(t, session, "user5", "test_codeowner_fork", "codeowner-basebranch-forked", "Test Pull Request2") + testPullCreate(t, session, "user5", "test_codeowner", "codeowner-basebranch-forked", "Test Pull Request2") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) From a44bfd9b09fbc80819e38d94990c5e5654e60c9a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 16 Mar 2024 21:34:00 +0800 Subject: [PATCH 6/6] Fix test --- tests/integration/pull_create_test.go | 24 ++++++++++++++++++++++-- tests/integration/pull_review_test.go | 6 +++--- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index f36ca804958f1..4e09872610ce2 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "net/url" "path" + "strings" "testing" "code.gitea.io/gitea/modules/test" @@ -17,16 +18,35 @@ import ( "github.com/stretchr/testify/assert" ) +func createPullRequest(t *testing.T, session *TestSession, user, repo, baseBranch, headBranch, title string) *httptest.ResponseRecorder { + link := fmt.Sprintf("/%s/%s/compare/%s...%s", user, repo, baseBranch, headBranch) + req := NewRequest(t, "GET", link) + resp := session.MakeRequest(t, req, http.StatusOK) + + // Submit the form for creating the pull + htmlDoc := NewHTMLParser(t, resp.Body) + link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action") + assert.True(t, exists, "The template has changed") + req = NewRequestWithValues(t, "POST", link, map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "title": title, + }) + resp = session.MakeRequest(t, req, http.StatusOK) + return resp +} + func testPullCreate(t *testing.T, session *TestSession, user, repo, branch, title string) *httptest.ResponseRecorder { req := NewRequest(t, "GET", path.Join(user, repo)) resp := session.MakeRequest(t, req, http.StatusOK) // Click the PR button to create a pull htmlDoc := NewHTMLParser(t, resp.Body) - _, exists := htmlDoc.doc.Find("#new-pull-request").Attr("href") + link, exists := htmlDoc.doc.Find("#new-pull-request").Attr("href") assert.True(t, exists, "The template has changed") + if branch != "master" { + link = strings.Replace(link, ":master", ":"+branch, 1) + } - link := fmt.Sprintf("/%s/%s/compare/master...%s", user, repo, branch) req = NewRequest(t, "GET", link) resp = session.MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 7fafa06eb0d3c..8987ad304b28c 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -73,7 +73,7 @@ func TestPullView_CodeOwner(t *testing.T) { // Create a pull request. session := loginUser(t, "user2") - testPullCreate(t, session, "user2", "test_codeowner", "codeowner-basebranch", "Test Pull Request") + createPullRequest(t, session, "user2", "test_codeowner", repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) @@ -107,7 +107,7 @@ func TestPullView_CodeOwner(t *testing.T) { // Create a pull request. session := loginUser(t, "user2") - testPullCreate(t, session, "user2", "test_codeowner", "codeowner-basebranch2", "Test Pull Request2") + createPullRequest(t, session, "user2", "test_codeowner", repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"}) unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) @@ -135,7 +135,7 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, err) session := loginUser(t, "user5") - testPullCreate(t, session, "user5", "test_codeowner", "codeowner-basebranch-forked", "Test Pull Request2") + createPullRequest(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2") pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})