Fix review request webhook bug (#35339) (#35596)

Frontport from #35339

Author: lunny

services/pull/comment.go

@@ -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)

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
 }
 

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

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)

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})

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)
 	})
 }