From 5e10def7f7f1e85b6b7bbdf536268b19e8f058d7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 28 Aug 2025 21:26:29 -0700 Subject: [PATCH] Fix review request webhook bug (#35339) partially backport #35337 Fix #35327 --- services/pull/pull.go | 71 ++++++++++++++------------ 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 | 26 ++++++++-- 5 files changed, 129 insertions(+), 52 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 81be797832..f1742ac8f6 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -147,32 +147,31 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { if err != nil { return err } - if len(compareInfo.Commits) == 0 { - return nil - } + // It maybe an empty pull request. Only non-empty pull request need to create push comment + 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()) + } - 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 + } } if !pr.IsWorkInProgress(ctx) { @@ -193,6 +192,20 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) + // Request reviews, these should be requested before other notifications because they will add request reviews record + // on database + 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 + } + } + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { return err @@ -211,17 +224,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 } diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index 86bdd1b9e3..9e03366bf1 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -105,7 +105,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 179c84e673..11bff3e417 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 1121751c88..628ac1c980 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -184,13 +184,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 3ccc34f20f..81de5481a9 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -14,6 +14,7 @@ import ( "time" 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" @@ -529,15 +530,30 @@ 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, session, "user2", "repo1", "master", "master2", http.StatusCreated) + testAPICreateBranch(t, sessionUser2, "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, sessionUser4, createPullRequestOptions{ + BaseRepoOwner: repo1.OwnerName, + BaseRepoName: repo1.Name, + BaseBranch: repo1.DefaultBranch, + HeadRepoOwner: "", + HeadRepoName: "", + HeadBranch: "master2", + Title: "first pull request", + ReviewerIDs: "2", // add user2 as reviewer + }) // 3. validate the webhook is triggered assert.Equal(t, "pull_request", triggeredEvent) @@ -549,6 +565,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(2), payloads[0].PullRequest.RequestedReviewers[0].ID) }) }