From 9a9379ff0c9d2d7ed24f5a0db9e59e271da619f4 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/comment.go | 5 ++- services/pull/pull.go | 26 +++++++------ 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 +++++++++++-- 6 files changed, 111 insertions(+), 30 deletions(-) diff --git a/services/pull/comment.go b/services/pull/comment.go index ca156d3d924fb..d85e7e3e32e09 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -46,7 +46,7 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC return commitIDs, err } -// CreatePushPullComment create push code to pull base comment +// CreatePushPullComment create push code to pull base comment, if no diff in this pr, no comment created func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *issues_model.PullRequest, oldCommitID, newCommitID string, isForcePush bool) (comment *issues_model.Comment, err error) { if pr.HasMerged || oldCommitID == "" || newCommitID == "" { return nil, nil @@ -69,6 +69,9 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss if err != nil { return nil, err } + if len(data.CommitIDs) == 0 { + return nil, nil + } } dataJSON, err := json.Marshal(data) diff --git a/services/pull/pull.go b/services/pull/pull.go index d374d90a62e21..0754ea3014b5d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -156,6 +156,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 @@ -174,17 +188,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 01989e8f89b7f..fc921d62686da 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -102,7 +102,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 2eb5e94cf9d98..83babaca85bf4 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 73a40c9440649..f6ced08184deb 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 d54a604655561..9c742240e1e76 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -15,6 +15,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" @@ -681,15 +682,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) @@ -701,6 +717,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) }) }