From e2dd7e8e08487d3b59a097787a2447646bc63616 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Aug 2025 17:47:01 -0700 Subject: [PATCH 1/6] Move request review functions to pull service package --- models/issues/pull.go | 144 -------- models/issues/pull_test.go | 22 -- modules/repository/codeowner.go | 168 ++++++++++ modules/repository/codeowner_test.go | 32 ++ routers/api/v1/repo/collaborators.go | 3 +- routers/api/v1/repo/pull_review.go | 5 +- routers/web/repo/issue_page_meta.go | 3 +- routers/web/repo/pull_review.go | 5 +- routers/web/repo/view_file.go | 6 +- services/issue/assignee.go | 271 --------------- services/issue/issue.go | 15 - services/pull/pull.go | 65 +++- services/pull/review.go | 3 +- services/pull/review_request.go | 316 ++++++++++++++++++ .../review_request_code_owner.go} | 26 +- tests/integration/actions_trigger_test.go | 4 +- tests/integration/api_pull_review_test.go | 4 +- tests/integration/pull_review_test.go | 3 +- 18 files changed, 587 insertions(+), 508 deletions(-) create mode 100644 modules/repository/codeowner.go create mode 100644 modules/repository/codeowner_test.go create mode 100644 services/pull/review_request.go rename services/{issue/pull.go => pull/review_request_code_owner.go} (87%) diff --git a/models/issues/pull.go b/models/issues/pull.go index 7a37b627e1bd0..8597858babc5e 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "io" - "regexp" "strings" "code.gitea.io/gitea/models/db" @@ -822,149 +821,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr * return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0 } -// GetCodeOwnersFromContent returns the code owners configuration -// Return empty slice if files missing -// Return warning messages on parsing errors -// We're trying to do the best we can when parsing a file. -// Invalid lines are skipped. Non-existent users and teams too. -func GetCodeOwnersFromContent(ctx context.Context, data string) ([]*CodeOwnerRule, []string) { - if len(data) == 0 { - return nil, nil - } - - rules := make([]*CodeOwnerRule, 0) - lines := strings.Split(data, "\n") - warnings := make([]string, 0) - - for i, line := range lines { - tokens := TokenizeCodeOwnersLine(line) - if len(tokens) == 0 { - continue - } else if len(tokens) < 2 { - warnings = append(warnings, fmt.Sprintf("Line: %d: incorrect format", i+1)) - continue - } - rule, wr := ParseCodeOwnersLine(ctx, tokens) - for _, w := range wr { - warnings = append(warnings, fmt.Sprintf("Line: %d: %s", i+1, w)) - } - if rule == nil { - continue - } - - rules = append(rules, rule) - } - - return rules, warnings -} - -type CodeOwnerRule struct { - Rule *regexp.Regexp - Negative bool - Users []*user_model.User - Teams []*org_model.Team -} - -func ParseCodeOwnersLine(ctx context.Context, tokens []string) (*CodeOwnerRule, []string) { - var err error - rule := &CodeOwnerRule{ - Users: make([]*user_model.User, 0), - Teams: make([]*org_model.Team, 0), - Negative: strings.HasPrefix(tokens[0], "!"), - } - - warnings := make([]string, 0) - - rule.Rule, err = regexp.Compile(fmt.Sprintf("^%s$", strings.TrimPrefix(tokens[0], "!"))) - if err != nil { - warnings = append(warnings, fmt.Sprintf("incorrect codeowner regexp: %s", err)) - return nil, warnings - } - - for _, user := range tokens[1:] { - user = strings.TrimPrefix(user, "@") - - // Only @org/team can contain slashes - if strings.Contains(user, "/") { - s := strings.Split(user, "/") - if len(s) != 2 { - warnings = append(warnings, "incorrect codeowner group: "+user) - continue - } - orgName := s[0] - teamName := s[1] - - org, err := org_model.GetOrgByName(ctx, orgName) - if err != nil { - warnings = append(warnings, "incorrect codeowner organization: "+user) - continue - } - teams, err := org.LoadTeams(ctx) - if err != nil { - warnings = append(warnings, "incorrect codeowner team: "+user) - continue - } - - for _, team := range teams { - if team.Name == teamName { - rule.Teams = append(rule.Teams, team) - } - } - } else { - u, err := user_model.GetUserByName(ctx, user) - if err != nil { - warnings = append(warnings, "incorrect codeowner user: "+user) - continue - } - rule.Users = append(rule.Users, u) - } - } - - if (len(rule.Users) == 0) && (len(rule.Teams) == 0) { - warnings = append(warnings, "no users/groups matched") - return nil, warnings - } - - return rule, warnings -} - -func TokenizeCodeOwnersLine(line string) []string { - if len(line) == 0 { - return nil - } - - line = strings.TrimSpace(line) - line = strings.ReplaceAll(line, "\t", " ") - - tokens := make([]string, 0) - - escape := false - token := "" - for _, char := range line { - if escape { - token += string(char) - escape = false - } else if string(char) == "\\" { - escape = true - } else if string(char) == "#" { - break - } else if string(char) == " " { - if len(token) > 0 { - tokens = append(tokens, token) - token = "" - } - } else { - token += string(char) - } - } - - if len(token) > 0 { - tokens = append(tokens, token) - } - - return tokens -} - // InsertPullRequests inserted pull requests func InsertPullRequests(ctx context.Context, prs ...*PullRequest) error { return db.WithTx(ctx, func(ctx context.Context) error { diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 39efaa5792ffe..cceda965f652a 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -317,28 +317,6 @@ func TestDeleteOrphanedObjects(t *testing.T) { assert.Equal(t, countBefore, countAfter) } -func TestParseCodeOwnersLine(t *testing.T) { - type CodeOwnerTest struct { - Line string - Tokens []string - } - - given := []CodeOwnerTest{ - {Line: "", Tokens: nil}, - {Line: "# comment", Tokens: []string{}}, - {Line: "!.* @user1 @org1/team1", Tokens: []string{"!.*", "@user1", "@org1/team1"}}, - {Line: `.*\\.js @user2 #comment`, Tokens: []string{`.*\.js`, "@user2"}}, - {Line: `docs/(aws|google|azure)/[^/]*\\.(md|txt) @org3 @org2/team2`, Tokens: []string{`docs/(aws|google|azure)/[^/]*\.(md|txt)`, "@org3", "@org2/team2"}}, - {Line: `\#path @org3`, Tokens: []string{`#path`, "@org3"}}, - {Line: `path\ with\ spaces/ @org3`, Tokens: []string{`path with spaces/`, "@org3"}}, - } - - for _, g := range given { - tokens := issues_model.TokenizeCodeOwnersLine(g.Line) - assert.Equal(t, g.Tokens, tokens, "Codeowners tokenizer failed") - } -} - func TestGetApprovers(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5}) diff --git a/modules/repository/codeowner.go b/modules/repository/codeowner.go new file mode 100644 index 0000000000000..e0b3ff5845a7a --- /dev/null +++ b/modules/repository/codeowner.go @@ -0,0 +1,168 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repository + +import ( + "context" + "fmt" + "regexp" + "slices" + "strings" + + org_model "code.gitea.io/gitea/models/organization" + user_model "code.gitea.io/gitea/models/user" +) + +var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} + +func IsCodeOwnerFile(f string) bool { + return slices.Contains(codeOwnerFiles, f) +} + +func GetCodeOwnerFiles() []string { + return codeOwnerFiles +} + +// GetCodeOwnersFromContent returns the code owners configuration +// Return empty slice if files missing +// Return warning messages on parsing errors +// We're trying to do the best we can when parsing a file. +// Invalid lines are skipped. Non-existent users and teams too. +func GetCodeOwnersFromContent(ctx context.Context, data string) ([]*CodeOwnerRule, []string) { + if len(data) == 0 { + return nil, nil + } + + rules := make([]*CodeOwnerRule, 0) + lines := strings.Split(data, "\n") + warnings := make([]string, 0) + + for i, line := range lines { + tokens := TokenizeCodeOwnersLine(line) + if len(tokens) == 0 { + continue + } else if len(tokens) < 2 { + warnings = append(warnings, fmt.Sprintf("Line: %d: incorrect format", i+1)) + continue + } + rule, wr := ParseCodeOwnersLine(ctx, tokens) + for _, w := range wr { + warnings = append(warnings, fmt.Sprintf("Line: %d: %s", i+1, w)) + } + if rule == nil { + continue + } + + rules = append(rules, rule) + } + + return rules, warnings +} + +type CodeOwnerRule struct { + Rule *regexp.Regexp + Negative bool + Users []*user_model.User + Teams []*org_model.Team +} + +func ParseCodeOwnersLine(ctx context.Context, tokens []string) (*CodeOwnerRule, []string) { + var err error + rule := &CodeOwnerRule{ + Users: make([]*user_model.User, 0), + Teams: make([]*org_model.Team, 0), + Negative: strings.HasPrefix(tokens[0], "!"), + } + + warnings := make([]string, 0) + + rule.Rule, err = regexp.Compile(fmt.Sprintf("^%s$", strings.TrimPrefix(tokens[0], "!"))) + if err != nil { + warnings = append(warnings, fmt.Sprintf("incorrect codeowner regexp: %s", err)) + return nil, warnings + } + + for _, user := range tokens[1:] { + user = strings.TrimPrefix(user, "@") + + // Only @org/team can contain slashes + if strings.Contains(user, "/") { + s := strings.Split(user, "/") + if len(s) != 2 { + warnings = append(warnings, "incorrect codeowner group: "+user) + continue + } + orgName := s[0] + teamName := s[1] + + org, err := org_model.GetOrgByName(ctx, orgName) + if err != nil { + warnings = append(warnings, "incorrect codeowner organization: "+user) + continue + } + teams, err := org.LoadTeams(ctx) + if err != nil { + warnings = append(warnings, "incorrect codeowner team: "+user) + continue + } + + for _, team := range teams { + if team.Name == teamName { + rule.Teams = append(rule.Teams, team) + } + } + } else { + u, err := user_model.GetUserByName(ctx, user) + if err != nil { + warnings = append(warnings, "incorrect codeowner user: "+user) + continue + } + rule.Users = append(rule.Users, u) + } + } + + if (len(rule.Users) == 0) && (len(rule.Teams) == 0) { + warnings = append(warnings, "no users/groups matched") + return nil, warnings + } + + return rule, warnings +} + +func TokenizeCodeOwnersLine(line string) []string { + if len(line) == 0 { + return nil + } + + line = strings.TrimSpace(line) + line = strings.ReplaceAll(line, "\t", " ") + + tokens := make([]string, 0) + + escape := false + token := "" + for _, char := range line { + if escape { + token += string(char) + escape = false + } else if string(char) == "\\" { + escape = true + } else if string(char) == "#" { + break + } else if string(char) == " " { + if len(token) > 0 { + tokens = append(tokens, token) + token = "" + } + } else { + token += string(char) + } + } + + if len(token) > 0 { + tokens = append(tokens, token) + } + + return tokens +} diff --git a/modules/repository/codeowner_test.go b/modules/repository/codeowner_test.go new file mode 100644 index 0000000000000..4dbce10bcdbca --- /dev/null +++ b/modules/repository/codeowner_test.go @@ -0,0 +1,32 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repository + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseCodeOwnersLine(t *testing.T) { + type CodeOwnerTest struct { + Line string + Tokens []string + } + + given := []CodeOwnerTest{ + {Line: "", Tokens: nil}, + {Line: "# comment", Tokens: []string{}}, + {Line: "!.* @user1 @org1/team1", Tokens: []string{"!.*", "@user1", "@org1/team1"}}, + {Line: `.*\\.js @user2 #comment`, Tokens: []string{`.*\.js`, "@user2"}}, + {Line: `docs/(aws|google|azure)/[^/]*\\.(md|txt) @org3 @org2/team2`, Tokens: []string{`docs/(aws|google|azure)/[^/]*\.(md|txt)`, "@org3", "@org2/team2"}}, + {Line: `\#path @org3`, Tokens: []string{`#path`, "@org3"}}, + {Line: `path\ with\ spaces/ @org3`, Tokens: []string{`path with spaces/`, "@org3"}}, + } + + for _, g := range given { + tokens := TokenizeCodeOwnersLine(g.Line) + assert.Equal(t, g.Tokens, tokens, "Codeowners tokenizer failed") + } +} diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index eed9c19fe1493..e721ce5313ad8 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" ) @@ -324,7 +323,7 @@ func GetReviewers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0) + canChooseReviewer := pull_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0) if !canChooseReviewer { ctx.APIError(http.StatusForbidden, errors.New("doer has no permission to get reviewers")) return diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 3c00193fac1d9..f5ba571bd864d 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -19,7 +19,6 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" ) @@ -730,7 +729,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions } for _, reviewer := range reviewers { - comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd) + comment, err := pull_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.APIError(http.StatusForbidden, err) @@ -755,7 +754,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { for _, teamReviewer := range teamReviewers { - comment, err := issue_service.TeamReviewRequest(ctx, pr.Issue, ctx.Doer, teamReviewer, isAdd) + comment, err := pull_service.TeamReviewRequest(ctx, pr.Issue, ctx.Doer, teamReviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.APIError(http.StatusForbidden, err) diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index 93cc38bffa1cf..c3dc7067aa9f4 100644 --- a/routers/web/repo/issue_page_meta.go +++ b/routers/web/repo/issue_page_meta.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/modules/optional" shared_user "code.gitea.io/gitea/routers/web/shared/user" "code.gitea.io/gitea/services/context" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" ) @@ -194,7 +193,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { if d.Issue == nil { data.CanChooseReviewer = true } else { - data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID) + data.CanChooseReviewer = pull_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID) } } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 18e14e9b224c4..f0005a2f9673e 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -20,7 +20,6 @@ import ( "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/forms" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" user_service "code.gitea.io/gitea/services/user" ) @@ -396,7 +395,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") + _, err = pull_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -428,7 +427,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") + _, err = pull_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( diff --git a/routers/web/repo/view_file.go b/routers/web/repo/view_file.go index 2d5bddd939f10..afbbad48599db 100644 --- a/routers/web/repo/view_file.go +++ b/routers/web/repo/view_file.go @@ -12,7 +12,6 @@ import ( "strings" git_model "code.gitea.io/gitea/models/git" - issue_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/renderhelper" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/actions" @@ -22,6 +21,7 @@ import ( "code.gitea.io/gitea/modules/highlight" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup" + repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/typesniffer" "code.gitea.io/gitea/modules/util" @@ -199,9 +199,9 @@ func prepareFileView(ctx *context.Context, entry *git.TreeEntry) { if workFlowErr != nil { ctx.Data["FileError"] = ctx.Locale.Tr("actions.runs.invalid_workflow_helper", workFlowErr.Error()) } - } else if issue_service.IsCodeOwnerFile(ctx.Repo.TreePath) { + } else if repo_module.IsCodeOwnerFile(ctx.Repo.TreePath) { if data, err := blob.GetBlobContent(setting.UI.MaxDisplayFileSize); err == nil { - _, warnings := issue_model.GetCodeOwnersFromContent(ctx, data) + _, warnings := repo_module.GetCodeOwnersFromContent(ctx, data) if len(warnings) > 0 { ctx.Data["FileWarning"] = strings.Join(warnings, "\n") } diff --git a/services/issue/assignee.go b/services/issue/assignee.go index ba9c91e0edc4a..bf02d67609a5f 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -7,13 +7,7 @@ import ( "context" issues_model "code.gitea.io/gitea/models/issues" - "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" - access_model "code.gitea.io/gitea/models/perm/access" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" notify_service "code.gitea.io/gitea/services/notify" ) @@ -61,268 +55,3 @@ func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, do return removed, comment, err } - -// ReviewRequest add or remove a review request from a user for this PR, and make comment for it. -func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { - err = isValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) - if err != nil { - return nil, err - } - - if isAdd { - comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) - } else { - comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) - } - - if err != nil { - return nil, err - } - - if comment != nil { - notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewer, isAdd, comment) - } - - return comment, err -} - -// isValidReviewRequest Check permission for ReviewRequest -func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { - if reviewer.IsOrganization() { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Organization can't be added as reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - if doer.IsOrganization() { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Organization can't be doer to add reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer) - if err != nil { - return err - } - - if permDoer == nil { - permDoer = new(access_model.Permission) - *permDoer, err = access_model.GetUserRepoPermission(ctx, issue.Repo, doer) - if err != nil { - return err - } - } - - lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) - if err != nil && !issues_model.IsErrReviewNotExist(err) { - return err - } - - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) - - if isAdd { - if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Reviewer can't read", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 { - return issues_model.ErrNotValidReviewRequest{ - Reason: "poster of pr can't be reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if canDoerChangeReviewRequests { - return nil - } - - if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastReview != nil && lastReview.Type != issues_model.ReviewTypeRequest { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't choose reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if canDoerChangeReviewRequests { - return nil - } - - if lastReview != nil && lastReview.Type == issues_model.ReviewTypeRequest && lastReview.ReviewerID == doer.ID { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't remove reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } -} - -// isValidTeamReviewRequest Check permission for ReviewRequest Team -func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { - if doer.IsOrganization() { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Organization can't be doer to add reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) - - if isAdd { - if issue.Repo.IsPrivate { - hasTeam := organization.HasTeamRepo(ctx, reviewer.OrgID, reviewer.ID, issue.RepoID) - - if !hasTeam { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Reviewing team can't read repo", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - } - - if canDoerChangeReviewRequests { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't choose reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if canDoerChangeReviewRequests { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't remove reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } -} - -// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. -func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) { - err = isValidTeamReviewRequest(ctx, reviewer, doer, isAdd, issue) - if err != nil { - return nil, err - } - if isAdd { - comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) - } else { - comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) - } - - if err != nil { - return nil, err - } - - if comment == nil || !isAdd { - return nil, nil - } - - return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) -} - -func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { - for _, reviewNotifier := range reviewNotifiers { - if reviewNotifier.Reviewer != nil { - notify_service.PullRequestReviewRequest(ctx, issue.Poster, issue, reviewNotifier.Reviewer, reviewNotifier.IsAdd, reviewNotifier.Comment) - } else if reviewNotifier.ReviewTeam != nil { - if err := teamReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifier.ReviewTeam, reviewNotifier.IsAdd, reviewNotifier.Comment); err != nil { - log.Error("teamReviewRequestNotify: %v", err) - } - } - } -} - -// teamReviewRequestNotify notify all user in this team -func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool, comment *issues_model.Comment) error { - // notify all user in this team - if err := comment.LoadIssue(ctx); err != nil { - return err - } - - members, err := organization.GetTeamMembers(ctx, &organization.SearchMembersOptions{ - TeamID: reviewer.ID, - }) - if err != nil { - return err - } - - for _, member := range members { - if member.ID == comment.Issue.PosterID { - continue - } - comment.AssigneeID = member.ID - notify_service.PullRequestReviewRequest(ctx, doer, issue, member, isAdd, comment) - } - - return err -} - -// CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR -func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool { - if repo.IsArchived { - return false - } - // The poster of the PR can change the reviewers - if doer.ID == posterID { - return true - } - - // The owner of the repo can change the reviewers - if doer.ID == repo.OwnerID { - return true - } - - // Collaborators of the repo can change the reviewers - isCollaborator, err := repo_model.IsCollaborator(ctx, repo.ID, doer.ID) - if err != nil { - log.Error("IsCollaborator: %v", err) - return false - } - if isCollaborator { - return true - } - - // If the repo's owner is an organization, members of teams with read permission on pull requests can change reviewers - if repo.Owner.IsOrganization() { - teams, err := organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) - if err != nil { - log.Error("GetTeamsWithAccessToRepo: %v", err) - return false - } - for _, team := range teams { - if !team.UnitEnabled(ctx, unit.TypePullRequests) { - continue - } - isMember, err := organization.IsTeamMember(ctx, repo.OwnerID, team.ID, doer.ID) - if err != nil { - log.Error("IsTeamMember: %v", err) - continue - } - if isMember { - return true - } - } - } - - return false -} diff --git a/services/issue/issue.go b/services/issue/issue.go index f03be3e18f6c7..b9ee66c1f2717 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -17,7 +17,6 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" notify_service "code.gitea.io/gitea/services/notify" ) @@ -90,21 +89,7 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode return err } - var reviewNotifiers []*ReviewRequestNotifier - if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { - if err := issue.LoadPullRequest(ctx); err != nil { - return err - } - - var err error - reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue.PullRequest) - if err != nil { - log.Error("PullRequestCodeOwnersReview: %v", err) - } - } - notify_service.IssueChangeTitle(ctx, doer, issue, oldTitle) - ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) return nil } diff --git a/services/pull/pull.go b/services/pull/pull.go index e55d4f5bb194f..e1936441c7ff3 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -115,7 +115,12 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } defer baseGitRepo.Close() - var reviewNotifiers []*issue_service.ReviewRequestNotifier + permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) + if err != nil { + return err + } + + var reviewNotifiers []*ReviewRequestNotifier if err := db.WithTx(ctx, func(ctx context.Context) error { if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { return err @@ -174,12 +179,48 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } + // review request from CodeOwners if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) + reviewNotifiers, err = RequestCodeOwnersReview(ctx, pr) + if err != nil { + return err + } + } + + for _, reviewer := range opts.Reviewers { + err = IsValidReviewRequest(ctx, reviewer, issue.Poster, true, issue, &permDoer) + if err != nil { + return err + } + + comment, err := issues_model.AddReviewRequest(ctx, issue, reviewer, issue.Poster) + if err != nil { + return err + } + reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + Reviewer: reviewer, + }) + } + + for _, teamReviewer := range opts.TeamReviewers { + err = IsValidTeamReviewRequest(ctx, teamReviewer, issue.Poster, true, issue) + if err != nil { + return err + } + + comment, err := issues_model.AddTeamReviewRequest(ctx, issue, teamReviewer, issue.Poster) if err != nil { return err } + reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + ReviewTeam: teamReviewer, + }) } + return nil }); err != nil { // cleanup: this will only remove the reference, the real commit will be clean up when next GC @@ -190,7 +231,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } baseGitRepo.Close() // close immediately to avoid notifications will open the repository again - issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) + ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { @@ -210,17 +251,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID]) } - permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) - for _, reviewer := range opts.Reviewers { - if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { - return err - } - } - for _, teamReviewer := range opts.TeamReviewers { - if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil { - return err - } - } + return nil } @@ -462,12 +493,12 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(ctx, pr) + reviewNotifiers, err := RequestCodeOwnersReview(ctx, pr) if err != nil { - log.Error("PullRequestCodeOwnersReview: %v", err) + log.Error("RequestCodeOwnersReview: %v", err) } if len(reviewNotifiers) > 0 { - issue_service.ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) + ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) } } diff --git a/services/pull/review.go b/services/pull/review.go index ee18db38599e8..a501c52ea6332 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -1,5 +1,4 @@ -// Copyright 2019 The Gitea Authors. -// All rights reserved. +// Copyright 2019 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package pull diff --git a/services/pull/review_request.go b/services/pull/review_request.go new file mode 100644 index 0000000000000..ecfd7737d9ae0 --- /dev/null +++ b/services/pull/review_request.go @@ -0,0 +1,316 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + + issues_model "code.gitea.io/gitea/models/issues" + org_model "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" + notify_service "code.gitea.io/gitea/services/notify" +) + +// ReviewRequest add or remove a review request from a user for this PR, and make comment for it. +func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { + err = IsValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) + if err != nil { + return nil, err + } + + if isAdd { + comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) + } else { + comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) + } + + if err != nil { + return nil, err + } + + if comment != nil { + notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewer, isAdd, comment) + } + + return comment, err +} + +// IsValidReviewRequest Check permission for ReviewRequest +func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { + if reviewer.IsOrganization() { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Organization can't be added as reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + if doer.IsOrganization() { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Organization can't be doer to add reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer) + if err != nil { + return err + } + + if permDoer == nil { + permDoer = new(access_model.Permission) + *permDoer, err = access_model.GetUserRepoPermission(ctx, issue.Repo, doer) + if err != nil { + return err + } + } + + lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + if err != nil && !issues_model.IsErrReviewNotExist(err) { + return err + } + + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) + + if isAdd { + if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Reviewer can't read", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 { + return issues_model.ErrNotValidReviewRequest{ + Reason: "poster of pr can't be reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if canDoerChangeReviewRequests { + return nil + } + + if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastReview != nil && lastReview.Type != issues_model.ReviewTypeRequest { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't choose reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if canDoerChangeReviewRequests { + return nil + } + + if lastReview != nil && lastReview.Type == issues_model.ReviewTypeRequest && lastReview.ReviewerID == doer.ID { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't remove reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } +} + +// IsValidTeamReviewRequest Check permission for ReviewRequest Team +func IsValidTeamReviewRequest(ctx context.Context, reviewer *org_model.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { + if doer.IsOrganization() { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Organization can't be doer to add reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) + + if isAdd { + if issue.Repo.IsPrivate { + hasTeam := org_model.HasTeamRepo(ctx, reviewer.OrgID, reviewer.ID, issue.RepoID) + + if !hasTeam { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Reviewing team can't read repo", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + } + + if canDoerChangeReviewRequests { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't choose reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if canDoerChangeReviewRequests { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't remove reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } +} + +// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. +func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *org_model.Team, isAdd bool) (comment *issues_model.Comment, err error) { + err = IsValidTeamReviewRequest(ctx, reviewer, doer, isAdd, issue) + if err != nil { + return nil, err + } + if isAdd { + comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) + } else { + comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) + } + + if err != nil { + return nil, err + } + + if comment == nil || !isAdd { + return nil, nil + } + + return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) +} + +func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { + for _, reviewNotifier := range reviewNotifiers { + if reviewNotifier.Reviewer != nil { + notify_service.PullRequestReviewRequest(ctx, issue.Poster, issue, reviewNotifier.Reviewer, reviewNotifier.IsAdd, reviewNotifier.Comment) + } else if reviewNotifier.ReviewTeam != nil { + if err := teamReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifier.ReviewTeam, reviewNotifier.IsAdd, reviewNotifier.Comment); err != nil { + log.Error("teamReviewRequestNotify: %v", err) + } + } + } +} + +// teamReviewRequestNotify notify all user in this team +func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *org_model.Team, isAdd bool, comment *issues_model.Comment) error { + // notify all user in this team + if err := comment.LoadIssue(ctx); err != nil { + return err + } + + members, err := org_model.GetTeamMembers(ctx, &org_model.SearchMembersOptions{ + TeamID: reviewer.ID, + }) + if err != nil { + return err + } + + for _, member := range members { + if member.ID == comment.Issue.PosterID { + continue + } + comment.AssigneeID = member.ID + notify_service.PullRequestReviewRequest(ctx, doer, issue, member, isAdd, comment) + } + + return err +} + +type ReviewRequestNotifier struct { + Comment *issues_model.Comment + IsAdd bool + Reviewer *user_model.User + ReviewTeam *org_model.Team +} + +// CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR +func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool { + if repo.IsArchived { + return false + } + // The poster of the PR can change the reviewers + if doer.ID == posterID { + return true + } + + // The owner of the repo can change the reviewers + if doer.ID == repo.OwnerID { + return true + } + + // Collaborators of the repo can change the reviewers + isCollaborator, err := repo_model.IsCollaborator(ctx, repo.ID, doer.ID) + if err != nil { + log.Error("IsCollaborator: %v", err) + return false + } + if isCollaborator { + return true + } + + // If the repo's owner is an organization, members of teams with read permission on pull requests can change reviewers + if repo.Owner.IsOrganization() { + teams, err := org_model.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) + if err != nil { + log.Error("GetTeamsWithAccessToRepo: %v", err) + return false + } + for _, team := range teams { + if !team.UnitEnabled(ctx, unit.TypePullRequests) { + continue + } + isMember, err := org_model.IsTeamMember(ctx, repo.OwnerID, team.ID, doer.ID) + if err != nil { + log.Error("IsTeamMember: %v", err) + continue + } + if isMember { + return true + } + } + } + + return false +} + +func init() { + notify_service.RegisterNotifier(&reviewRequestNotifer{}) +} + +type reviewRequestNotifer struct { + notify_service.NullNotifier +} + +func (n *reviewRequestNotifer) IssueChangeTitle(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldTitle string) { + var reviewNotifiers []*ReviewRequestNotifier + if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(issue.Title) { + if err := issue.LoadPullRequest(ctx); err != nil { + log.Error("IssueChangeTitle: LoadPullRequest: %v", err) + return + } + + var err error + reviewNotifiers, err = RequestCodeOwnersReview(ctx, issue.PullRequest) + if err != nil { + log.Error("RequestCodeOwnersReview: %v", err) + } + } + + ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) +} diff --git a/services/issue/pull.go b/services/pull/review_request_code_owner.go similarity index 87% rename from services/issue/pull.go rename to services/pull/review_request_code_owner.go index 512fdf78e84be..e4bd281390df1 100644 --- a/services/issue/pull.go +++ b/services/pull/review_request_code_owner.go @@ -1,12 +1,11 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT -package issue +package pull import ( "context" "fmt" - "slices" "time" issues_model "code.gitea.io/gitea/models/issues" @@ -15,6 +14,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" + repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" ) @@ -34,20 +34,8 @@ func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch return mergeBase, err } -type ReviewRequestNotifier struct { - Comment *issues_model.Comment - IsAdd bool - Reviewer *user_model.User - ReviewTeam *org_model.Team -} - -var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - -func IsCodeOwnerFile(f string) bool { - return slices.Contains(codeOwnerFiles, f) -} - -func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { +// RequestCodeOwnersReview requests code owners review for a pull request and return notifiers +func RequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { if err := pr.LoadIssue(ctx); err != nil { return nil, err } @@ -79,7 +67,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque } var data string - for _, file := range codeOwnerFiles { + for _, file := range repo_module.GetCodeOwnerFiles() { if blob, err := commit.GetBlobByPath(file); err == nil { data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) if err == nil { @@ -91,7 +79,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque return nil, nil } - rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) + rules, _ := repo_module.GetCodeOwnersFromContent(ctx, data) if len(rules) == 0 { return nil, nil } diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 088491d5705f4..fa79cef016585 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -665,12 +665,12 @@ jobs: checkCommitStatusAndInsertFakeStatus(t, repo, sha) // review_requested - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user2, nil, user4, true) + _, err = pull_service.ReviewRequest(db.DefaultContext, pullIssue, user2, nil, user4, true) assert.NoError(t, err) checkCommitStatusAndInsertFakeStatus(t, repo, sha) // review_request_removed - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user2, nil, user4, false) + _, err = pull_service.ReviewRequest(db.DefaultContext, pullIssue, user2, nil, user4, false) assert.NoError(t, err) checkCommitStatusAndInsertFakeStatus(t, repo, sha) }) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 1fc65ddea89dc..0d0659535c615 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -17,7 +17,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/json" api "code.gitea.io/gitea/modules/structs" - issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -425,7 +425,7 @@ func TestAPIPullReviewStayDismissed(t *testing.T) { // user8 dismiss review permUser8, err := access_model.GetUserRepoPermission(db.DefaultContext, pullIssue.Repo, user8) assert.NoError(t, err) - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user8, &permUser8, user8, false) + _, err = pull_service.ReviewRequest(db.DefaultContext, pullIssue, user8, &permUser8, user8, false) assert.NoError(t, err) reviewsCountCheck(t, diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 1121751c88bc7..4d119a004eca7 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -19,6 +19,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" @@ -108,7 +109,7 @@ func TestPullView_CodeOwner(t *testing.T) { }) assert.NoError(t, err) - reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr) + reviewNotifiers, err := pull_service.RequestCodeOwnersReview(db.DefaultContext, pr) assert.NoError(t, err) assert.Len(t, reviewNotifiers, 1) assert.EqualValues(t, 8, reviewNotifiers[0].Reviewer.ID) From 193c6c05c1a353e62cfb8aed88f88502b22e52d6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Aug 2025 19:21:23 -0700 Subject: [PATCH 2/6] hide functions which will not be referenced outside of the package --- services/pull/pull.go | 4 ++-- services/pull/review_request.go | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index e1936441c7ff3..909201bc7f512 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -231,7 +231,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } baseGitRepo.Close() // close immediately to avoid notifications will open the repository again - ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) + reviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { @@ -498,7 +498,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Error("RequestCodeOwnersReview: %v", err) } if len(reviewNotifiers) > 0 { - ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) + reviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) } } diff --git a/services/pull/review_request.go b/services/pull/review_request.go index ecfd7737d9ae0..50c8dd5be2617 100644 --- a/services/pull/review_request.go +++ b/services/pull/review_request.go @@ -195,12 +195,12 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) } -func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { +func reviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { for _, reviewNotifier := range reviewNotifiers { if reviewNotifier.Reviewer != nil { - notify_service.PullRequestReviewRequest(ctx, issue.Poster, issue, reviewNotifier.Reviewer, reviewNotifier.IsAdd, reviewNotifier.Comment) + notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewNotifier.Reviewer, reviewNotifier.IsAdd, reviewNotifier.Comment) } else if reviewNotifier.ReviewTeam != nil { - if err := teamReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifier.ReviewTeam, reviewNotifier.IsAdd, reviewNotifier.Comment); err != nil { + if err := teamReviewRequestNotify(ctx, issue, doer, reviewNotifier.ReviewTeam, reviewNotifier.IsAdd, reviewNotifier.Comment); err != nil { log.Error("teamReviewRequestNotify: %v", err) } } @@ -298,19 +298,17 @@ type reviewRequestNotifer struct { } func (n *reviewRequestNotifer) IssueChangeTitle(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldTitle string) { - var reviewNotifiers []*ReviewRequestNotifier if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(issue.Title) { if err := issue.LoadPullRequest(ctx); err != nil { log.Error("IssueChangeTitle: LoadPullRequest: %v", err) return } - var err error - reviewNotifiers, err = RequestCodeOwnersReview(ctx, issue.PullRequest) + reviewNotifiers, err := RequestCodeOwnersReview(ctx, issue.PullRequest) if err != nil { log.Error("RequestCodeOwnersReview: %v", err) + } else { + reviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) } } - - ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) } From 76697604ae4b5f04f07fa4ff5ea53048fbb29fe0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Aug 2025 19:34:11 -0700 Subject: [PATCH 3/6] hide functions which will not be referenced outside of the package --- services/pull/pull.go | 28 ++++++++++++++++------------ services/pull/review_request.go | 12 ++++++------ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 909201bc7f512..f80f290333347 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -188,7 +188,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } for _, reviewer := range opts.Reviewers { - err = IsValidReviewRequest(ctx, reviewer, issue.Poster, true, issue, &permDoer) + err = isValidReviewRequest(ctx, reviewer, issue.Poster, true, issue, &permDoer) if err != nil { return err } @@ -197,15 +197,17 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { if err != nil { return err } - reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ - Comment: comment, - IsAdd: true, - Reviewer: reviewer, - }) + if comment != nil { + reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + Reviewer: reviewer, + }) + } } for _, teamReviewer := range opts.TeamReviewers { - err = IsValidTeamReviewRequest(ctx, teamReviewer, issue.Poster, true, issue) + err = isValidTeamReviewRequest(ctx, teamReviewer, issue.Poster, true, issue) if err != nil { return err } @@ -214,11 +216,13 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { if err != nil { return err } - reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ - Comment: comment, - IsAdd: true, - ReviewTeam: teamReviewer, - }) + if comment != nil { + reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + ReviewTeam: teamReviewer, + }) + } } return nil diff --git a/services/pull/review_request.go b/services/pull/review_request.go index 50c8dd5be2617..5ba9711b9a0ea 100644 --- a/services/pull/review_request.go +++ b/services/pull/review_request.go @@ -19,7 +19,7 @@ import ( // ReviewRequest add or remove a review request from a user for this PR, and make comment for it. func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { - err = IsValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) + err = isValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) if err != nil { return nil, err } @@ -41,8 +41,8 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_mo return comment, err } -// IsValidReviewRequest Check permission for ReviewRequest -func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { +// isValidReviewRequest Check permission for ReviewRequest +func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { if reviewer.IsOrganization() { return issues_model.ErrNotValidReviewRequest{ Reason: "Organization can't be added as reviewer", @@ -125,8 +125,8 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } -// IsValidTeamReviewRequest Check permission for ReviewRequest Team -func IsValidTeamReviewRequest(ctx context.Context, reviewer *org_model.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { +// isValidTeamReviewRequest Check permission for ReviewRequest Team +func isValidTeamReviewRequest(ctx context.Context, reviewer *org_model.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { if doer.IsOrganization() { return issues_model.ErrNotValidReviewRequest{ Reason: "Organization can't be doer to add reviewer", @@ -174,7 +174,7 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *org_model.Team, doe // TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *org_model.Team, isAdd bool) (comment *issues_model.Comment, err error) { - err = IsValidTeamReviewRequest(ctx, reviewer, doer, isAdd, issue) + err = isValidTeamReviewRequest(ctx, reviewer, doer, isAdd, issue) if err != nil { return nil, err } From 306922cacf52b018de7f9b409b7081ac09be121a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Aug 2025 20:00:54 -0700 Subject: [PATCH 4/6] add webhook test for requestreviwer --- tests/integration/pull_compare_test.go | 10 ++++- tests/integration/pull_create_test.go | 54 ++++++++++++++++++++------ tests/integration/pull_review_test.go | 20 +++++++++- tests/integration/repo_webhook_test.go | 13 ++++++- 4 files changed, 82 insertions(+), 15 deletions(-) diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index f95a2f1690929..456b9000c9476 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -103,7 +103,15 @@ func TestPullCompare_EnableAllowEditsFromMaintainer(t *testing.T) { // user4 creates a new branch and a PR testEditFileToNewBranch(t, user4Session, "user4", forkedRepoName, "master", "user4/update-readme", "README.md", "Hello, World\n(Edited by user4)\n") - resp := testPullCreateDirectly(t, user4Session, repo3.OwnerName, repo3.Name, "master", "user4", forkedRepoName, "user4/update-readme", "PR for user4 forked repo3") + resp := testPullCreateDirectly(t, user4Session, createPullRequestOptions{ + BaseRepoOwner: repo3.OwnerName, + BaseRepoName: repo3.Name, + BaseBranch: "master", + HeadRepoOwner: "user4", + HeadRepoName: forkedRepoName, + HeadBranch: "user4/update-readme", + Title: "PR for user4 forked repo3", + }) prURL := test.RedirectURL(resp) // user2 (admin of repo3) goes to the PR files page diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index 44ef5019611f5..c751942811af1 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -60,26 +60,50 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel return resp } -func testPullCreateDirectly(t *testing.T, session *TestSession, baseRepoOwner, baseRepoName, baseBranch, headRepoOwner, headRepoName, headBranch, title string) *httptest.ResponseRecorder { - headCompare := headBranch - if headRepoOwner != "" { - if headRepoName != "" { - headCompare = fmt.Sprintf("%s/%s:%s", headRepoOwner, headRepoName, headBranch) +type createPullRequestOptions struct { + BaseRepoOwner string + BaseRepoName string + BaseBranch string + HeadRepoOwner string + HeadRepoName string + HeadBranch string + Title string + ReviewerIDs string // comma-separated list of user IDs +} + +func (opts createPullRequestOptions) IsValid() bool { + return opts.BaseRepoOwner != "" && opts.BaseRepoName != "" && opts.BaseBranch != "" && + opts.HeadBranch != "" && opts.Title != "" +} + +func testPullCreateDirectly(t *testing.T, session *TestSession, opts createPullRequestOptions) *httptest.ResponseRecorder { + if !opts.IsValid() { + t.Fatal("Invalid pull request options") + } + + headCompare := opts.HeadBranch + if opts.HeadRepoOwner != "" { + if opts.HeadRepoName != "" { + headCompare = fmt.Sprintf("%s/%s:%s", opts.HeadRepoOwner, opts.HeadRepoName, opts.HeadBranch) } else { - headCompare = fmt.Sprintf("%s:%s", headRepoOwner, headBranch) + headCompare = fmt.Sprintf("%s:%s", opts.HeadRepoOwner, opts.HeadBranch) } } - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", baseRepoOwner, baseRepoName, baseBranch, headCompare)) + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", opts.BaseRepoOwner, opts.BaseRepoName, opts.BaseBranch, headCompare)) 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{ + params := map[string]string{ "_csrf": htmlDoc.GetCSRF(), - "title": title, - }) + "title": opts.Title, + } + if opts.ReviewerIDs != "" { + params["reviewer_ids"] = opts.ReviewerIDs + } + req = NewRequestWithValues(t, "POST", link, params) resp = session.MakeRequest(t, req, http.StatusOK) return resp } @@ -246,7 +270,15 @@ func TestPullCreatePrFromBaseToFork(t *testing.T) { testEditFile(t, sessionBase, "user2", "repo1", "master", "README.md", "Hello, World (Edited)\n") // Create a PR - resp := testPullCreateDirectly(t, sessionFork, "user1", "repo1", "master", "user2", "repo1", "master", "This is a pull title") + resp := testPullCreateDirectly(t, sessionFork, createPullRequestOptions{ + BaseRepoOwner: "user1", + BaseRepoName: "repo1", + BaseBranch: "master", + HeadRepoOwner: "user2", + HeadRepoName: "repo1", + HeadBranch: "master", + Title: "This is a pull title", + }) // check the redirected URL url := test.RedirectURL(resp) assert.Regexp(t, "^/user1/repo1/pulls/[0-9]*$", url) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 4d119a004eca7..4dd28c4af60e0 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -185,13 +185,29 @@ func TestPullView_CodeOwner(t *testing.T) { session := loginUser(t, "user5") // create a pull request on the forked repository, code reviewers should not be mentioned - testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository") + 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, repo.OwnerName, repo.Name, repo.DefaultBranch, forkedRepo.OwnerName, forkedRepo.Name, "codeowner-basebranch-forked", "Test Pull Request3") + 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}) diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index f1abac8cfa1fb..a5196debdfe71 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -686,7 +686,16 @@ func Test_WebhookPullRequest(t *testing.T) { testAPICreateBranch(t, session, "user2", "repo1", "master", "master2", http.StatusCreated) // 2. trigger the webhook repo1 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 1}) - testCreatePullToDefaultBranch(t, session, repo1, repo1, "master2", "first pull request") + testPullCreateDirectly(t, session, createPullRequestOptions{ + BaseRepoOwner: repo1.OwnerName, + BaseRepoName: repo1.Name, + BaseBranch: repo1.DefaultBranch, + HeadRepoOwner: "", + HeadRepoName: "", + HeadBranch: "master2", + Title: "first pull request", + ReviewerIDs: "1", + }) // 3. validate the webhook is triggered assert.Equal(t, "pull_request", triggeredEvent) @@ -698,6 +707,8 @@ func Test_WebhookPullRequest(t *testing.T) { assert.Equal(t, 0, *payloads[0].PullRequest.Additions) assert.Equal(t, 0, *payloads[0].PullRequest.ChangedFiles) assert.Equal(t, 0, *payloads[0].PullRequest.Deletions) + assert.Len(t, payloads[0].PullRequest.RequestedReviewers, 1) + assert.Equal(t, int64(1), payloads[0].PullRequest.RequestedReviewers[0].ID) }) } From b997afd548ba910884fdc3297f8d46c19e25ae2b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Aug 2025 21:18:10 -0700 Subject: [PATCH 5/6] Fix test --- services/pull/pull.go | 44 ++++++++++++-------------- tests/integration/repo_webhook_test.go | 20 ++++++++---- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index f80f290333347..3af9c9318e253 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -151,32 +151,30 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { if err != nil { return err } - if len(compareInfo.Commits) == 0 { - return nil - } - - data := issues_model.PushActionContent{IsForcePush: false} - data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) - for i := len(compareInfo.Commits) - 1; i >= 0; i-- { - data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String()) - } + if len(compareInfo.Commits) > 0 { + data := issues_model.PushActionContent{IsForcePush: false} + data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) + for i := len(compareInfo.Commits) - 1; i >= 0; i-- { + data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String()) + } - dataJSON, err := json.Marshal(data) - if err != nil { - return err - } + dataJSON, err := json.Marshal(data) + if err != nil { + return err + } - ops := &issues_model.CreateCommentOptions{ - Type: issues_model.CommentTypePullRequestPush, - Doer: issue.Poster, - Repo: repo, - Issue: pr.Issue, - IsForcePush: false, - Content: string(dataJSON), - } + ops := &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypePullRequestPush, + Doer: issue.Poster, + Repo: repo, + Issue: pr.Issue, + IsForcePush: false, + Content: string(dataJSON), + } - if _, err = issues_model.CreateComment(ctx, ops); err != nil { - return err + if _, err = issues_model.CreateComment(ctx, ops); err != nil { + return err + } } // review request from CodeOwners diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index a5196debdfe71..2d1565b797495 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -14,6 +14,7 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -678,15 +679,22 @@ func Test_WebhookPullRequest(t *testing.T) { }, http.StatusOK) defer provider.Close() + testCtx := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeAll) + // add user4 as collabrator so that it can be a reviewer + doAPIAddCollaborator(testCtx, "user4", perm.AccessModeWrite)(t) + // 1. create a new webhook with special webhook for repo1 - session := loginUser(t, "user2") + sessionUser2 := loginUser(t, "user2") + sessionUser4 := loginUser(t, "user4") - testAPICreateWebhookForRepo(t, session, "user2", "repo1", provider.URL(), "pull_request") + // ignore the possible review_requested event to keep the test deterministic + testAPICreateWebhookForRepo(t, sessionUser2, "user2", "repo1", provider.URL(), "pull_request_only") + + testAPICreateBranch(t, sessionUser2, "user2", "repo1", "master", "master2", http.StatusCreated) - testAPICreateBranch(t, session, "user2", "repo1", "master", "master2", http.StatusCreated) // 2. trigger the webhook repo1 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 1}) - testPullCreateDirectly(t, session, createPullRequestOptions{ + testPullCreateDirectly(t, sessionUser4, createPullRequestOptions{ BaseRepoOwner: repo1.OwnerName, BaseRepoName: repo1.Name, BaseBranch: repo1.DefaultBranch, @@ -694,7 +702,7 @@ func Test_WebhookPullRequest(t *testing.T) { HeadRepoName: "", HeadBranch: "master2", Title: "first pull request", - ReviewerIDs: "1", + ReviewerIDs: "2", // add user2 as reviewer }) // 3. validate the webhook is triggered @@ -708,7 +716,7 @@ func Test_WebhookPullRequest(t *testing.T) { assert.Equal(t, 0, *payloads[0].PullRequest.ChangedFiles) assert.Equal(t, 0, *payloads[0].PullRequest.Deletions) assert.Len(t, payloads[0].PullRequest.RequestedReviewers, 1) - assert.Equal(t, int64(1), payloads[0].PullRequest.RequestedReviewers[0].ID) + assert.Equal(t, int64(2), payloads[0].PullRequest.RequestedReviewers[0].ID) }) } From e433ff31d63aba6f2e142bd9884354872d0bfca0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Aug 2025 21:45:37 -0700 Subject: [PATCH 6/6] Add mergable test --- tests/integration/repo_webhook_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 2d1565b797495..85f846efd435f 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -717,6 +717,7 @@ func Test_WebhookPullRequest(t *testing.T) { assert.Equal(t, 0, *payloads[0].PullRequest.Deletions) assert.Len(t, payloads[0].PullRequest.RequestedReviewers, 1) assert.Equal(t, int64(2), payloads[0].PullRequest.RequestedReviewers[0].ID) + assert.True(t, payloads[0].PullRequest.Mergeable) }) }