Skip to content

Commit 5e10def

Browse files
lunnylunny
authored
Fix review request webhook bug (#35339)
partially backport #35337 Fix #35327
1 parent 1b8efb6 commit 5e10def

File tree

5 files changed

+129
-52
lines changed

5 files changed

+129
-52
lines changed

‎services/pull/pull.go‎

Lines changed: 37 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -147,32 +147,31 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
147147
if err != nil {
148148
return err
149149
}
150-
if len(compareInfo.Commits) == 0 {
151-
return nil
152-
}
153-
154-
data := issues_model.PushActionContent{IsForcePush: false}
155-
data.CommitIDs = make([]string, 0, len(compareInfo.Commits))
156-
for i := len(compareInfo.Commits) - 1; i >= 0; i-- {
157-
data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String())
158-
}
150+
// It maybe an empty pull request. Only non-empty pull request need to create push comment
151+
if len(compareInfo.Commits) > 0 {
152+
data := issues_model.PushActionContent{IsForcePush: false}
153+
data.CommitIDs = make([]string, 0, len(compareInfo.Commits))
154+
for i := len(compareInfo.Commits) - 1; i >= 0; i-- {
155+
data.CommitIDs = append(data.CommitIDs, compareInfo.Commits[i].ID.String())
156+
}
159157

160-
dataJSON, err := json.Marshal(data)
161-
if err != nil {
162-
return err
163-
}
158+
dataJSON, err := json.Marshal(data)
159+
if err != nil {
160+
return err
161+
}
164162

165-
ops := &issues_model.CreateCommentOptions{
166-
Type: issues_model.CommentTypePullRequestPush,
167-
Doer: issue.Poster,
168-
Repo: repo,
169-
Issue: pr.Issue,
170-
IsForcePush: false,
171-
Content: string(dataJSON),
172-
}
163+
ops := &issues_model.CreateCommentOptions{
164+
Type: issues_model.CommentTypePullRequestPush,
165+
Doer: issue.Poster,
166+
Repo: repo,
167+
Issue: pr.Issue,
168+
IsForcePush: false,
169+
Content: string(dataJSON),
170+
}
173171

174-
if _, err = issues_model.CreateComment(ctx, ops); err != nil {
175-
return err
172+
if _, err = issues_model.CreateComment(ctx, ops); err != nil {
173+
return err
174+
}
176175
}
177176

178177
if !pr.IsWorkInProgress(ctx) {
@@ -193,6 +192,20 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
193192

194193
issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers)
195194

195+
// Request reviews, these should be requested before other notifications because they will add request reviews record
196+
// on database
197+
permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster)
198+
for _, reviewer := range opts.Reviewers {
199+
if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil {
200+
return err
201+
}
202+
}
203+
for _, teamReviewer := range opts.TeamReviewers {
204+
if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil {
205+
return err
206+
}
207+
}
208+
196209
mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content)
197210
if err != nil {
198211
return err
@@ -211,17 +224,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
211224
}
212225
notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID])
213226
}
214-
permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster)
215-
for _, reviewer := range opts.Reviewers {
216-
if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil {
217-
return err
218-
}
219-
}
220-
for _, teamReviewer := range opts.TeamReviewers {
221-
if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil {
222-
return err
223-
}
224-
}
227+
225228
return nil
226229
}
227230

‎tests/integration/pull_compare_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,15 @@ func TestPullCompare_EnableAllowEditsFromMaintainer(t *testing.T) {
105105

106106
// user4 creates a new branch and a PR
107107
testEditFileToNewBranch(t, user4Session, "user4", forkedRepoName, "master", "user4/update-readme", "README.md", "Hello, World\n(Edited by user4)\n")
108-
resp := testPullCreateDirectly(t, user4Session, repo3.OwnerName, repo3.Name, "master", "user4", forkedRepoName, "user4/update-readme", "PR for user4 forked repo3")
108+
resp := testPullCreateDirectly(t, user4Session, createPullRequestOptions{
109+
BaseRepoOwner: repo3.OwnerName,
110+
BaseRepoName: repo3.Name,
111+
BaseBranch: "master",
112+
HeadRepoOwner: "user4",
113+
HeadRepoName: forkedRepoName,
114+
HeadBranch: "user4/update-readme",
115+
Title: "PR for user4 forked repo3",
116+
})
109117
prURL := test.RedirectURL(resp)
110118

111119
// user2 (admin of repo3) goes to the PR files page

‎tests/integration/pull_create_test.go‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -60,26 +60,50 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel
6060
return resp
6161
}
6262

63-
func testPullCreateDirectly(t *testing.T, session *TestSession, baseRepoOwner, baseRepoName, baseBranch, headRepoOwner, headRepoName, headBranch, title string) *httptest.ResponseRecorder {
64-
headCompare := headBranch
65-
if headRepoOwner != "" {
66-
if headRepoName != "" {
67-
headCompare = fmt.Sprintf("%s/%s:%s", headRepoOwner, headRepoName, headBranch)
63+
type createPullRequestOptions struct {
64+
BaseRepoOwner string
65+
BaseRepoName string
66+
BaseBranch string
67+
HeadRepoOwner string
68+
HeadRepoName string
69+
HeadBranch string
70+
Title string
71+
ReviewerIDs string // comma-separated list of user IDs
72+
}
73+
74+
func (opts createPullRequestOptions) IsValid() bool {
75+
return opts.BaseRepoOwner != "" && opts.BaseRepoName != "" && opts.BaseBranch != "" &&
76+
opts.HeadBranch != "" && opts.Title != ""
77+
}
78+
79+
func testPullCreateDirectly(t *testing.T, session *TestSession, opts createPullRequestOptions) *httptest.ResponseRecorder {
80+
if !opts.IsValid() {
81+
t.Fatal("Invalid pull request options")
82+
}
83+
84+
headCompare := opts.HeadBranch
85+
if opts.HeadRepoOwner != "" {
86+
if opts.HeadRepoName != "" {
87+
headCompare = fmt.Sprintf("%s/%s:%s", opts.HeadRepoOwner, opts.HeadRepoName, opts.HeadBranch)
6888
} else {
69-
headCompare = fmt.Sprintf("%s:%s", headRepoOwner, headBranch)
89+
headCompare = fmt.Sprintf("%s:%s", opts.HeadRepoOwner, opts.HeadBranch)
7090
}
7191
}
72-
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", baseRepoOwner, baseRepoName, baseBranch, headCompare))
92+
req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", opts.BaseRepoOwner, opts.BaseRepoName, opts.BaseBranch, headCompare))
7393
resp := session.MakeRequest(t, req, http.StatusOK)
7494

7595
// Submit the form for creating the pull
7696
htmlDoc := NewHTMLParser(t, resp.Body)
7797
link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action")
7898
assert.True(t, exists, "The template has changed")
79-
req = NewRequestWithValues(t, "POST", link, map[string]string{
99+
params := map[string]string{
80100
"_csrf": htmlDoc.GetCSRF(),
81-
"title": title,
82-
})
101+
"title": opts.Title,
102+
}
103+
if opts.ReviewerIDs != "" {
104+
params["reviewer_ids"] = opts.ReviewerIDs
105+
}
106+
req = NewRequestWithValues(t, "POST", link, params)
83107
resp = session.MakeRequest(t, req, http.StatusOK)
84108
return resp
85109
}
@@ -246,7 +270,15 @@ func TestPullCreatePrFromBaseToFork(t *testing.T) {
246270
testEditFile(t, sessionBase, "user2", "repo1", "master", "README.md", "Hello, World (Edited)\n")
247271

248272
// Create a PR
249-
resp := testPullCreateDirectly(t, sessionFork, "user1", "repo1", "master", "user2", "repo1", "master", "This is a pull title")
273+
resp := testPullCreateDirectly(t, sessionFork, createPullRequestOptions{
274+
BaseRepoOwner: "user1",
275+
BaseRepoName: "repo1",
276+
BaseBranch: "master",
277+
HeadRepoOwner: "user2",
278+
HeadRepoName: "repo1",
279+
HeadBranch: "master",
280+
Title: "This is a pull title",
281+
})
250282
// check the redirected URL
251283
url := test.RedirectURL(resp)
252284
assert.Regexp(t, "^/user1/repo1/pulls/[0-9]*$", url)

‎tests/integration/pull_review_test.go‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,13 +184,29 @@ func TestPullView_CodeOwner(t *testing.T) {
184184
session := loginUser(t, "user5")
185185

186186
// create a pull request on the forked repository, code reviewers should not be mentioned
187-
testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository")
187+
testPullCreateDirectly(t, session, createPullRequestOptions{
188+
BaseRepoOwner: "user5",
189+
BaseRepoName: "test_codeowner",
190+
BaseBranch: forkedRepo.DefaultBranch,
191+
HeadRepoOwner: "",
192+
HeadRepoName: "",
193+
HeadBranch: "codeowner-basebranch-forked",
194+
Title: "Test Pull Request on Forked Repository",
195+
})
188196

189197
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"})
190198
unittest.AssertNotExistsBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
191199

192200
// create a pull request to base repository, code reviewers should be mentioned
193-
testPullCreateDirectly(t, session, repo.OwnerName, repo.Name, repo.DefaultBranch, forkedRepo.OwnerName, forkedRepo.Name, "codeowner-basebranch-forked", "Test Pull Request3")
201+
testPullCreateDirectly(t, session, createPullRequestOptions{
202+
BaseRepoOwner: repo.OwnerName,
203+
BaseRepoName: repo.Name,
204+
BaseBranch: repo.DefaultBranch,
205+
HeadRepoOwner: forkedRepo.OwnerName,
206+
HeadRepoName: forkedRepo.Name,
207+
HeadBranch: "codeowner-basebranch-forked",
208+
Title: "Test Pull Request3",
209+
})
194210

195211
pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"})
196212
unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})

‎tests/integration/repo_webhook_test.go‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"time"
1515

1616
auth_model "code.gitea.io/gitea/models/auth"
17+
"code.gitea.io/gitea/models/perm"
1718
"code.gitea.io/gitea/models/repo"
1819
"code.gitea.io/gitea/models/unittest"
1920
user_model "code.gitea.io/gitea/models/user"
@@ -529,15 +530,30 @@ func Test_WebhookPullRequest(t *testing.T) {
529530
}, http.StatusOK)
530531
defer provider.Close()
531532

533+
testCtx := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeAll)
534+
// add user4 as collabrator so that it can be a reviewer
535+
doAPIAddCollaborator(testCtx, "user4", perm.AccessModeWrite)(t)
536+
532537
// 1. create a new webhook with special webhook for repo1
533-
session := loginUser(t, "user2")
538+
sessionUser2 := loginUser(t, "user2")
539+
sessionUser4 := loginUser(t, "user4")
534540

535-
testAPICreateWebhookForRepo(t, session, "user2", "repo1", provider.URL(), "pull_request")
541+
// ignore the possible review_requested event to keep the test deterministic
542+
testAPICreateWebhookForRepo(t, sessionUser2, "user2", "repo1", provider.URL(), "pull_request_only")
536543

537-
testAPICreateBranch(t, session, "user2", "repo1", "master", "master2", http.StatusCreated)
544+
testAPICreateBranch(t, sessionUser2, "user2", "repo1", "master", "master2", http.StatusCreated)
538545
// 2. trigger the webhook
539546
repo1 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 1})
540-
testCreatePullToDefaultBranch(t, session, repo1, repo1, "master2", "first pull request")
547+
testPullCreateDirectly(t, sessionUser4, createPullRequestOptions{
548+
BaseRepoOwner: repo1.OwnerName,
549+
BaseRepoName: repo1.Name,
550+
BaseBranch: repo1.DefaultBranch,
551+
HeadRepoOwner: "",
552+
HeadRepoName: "",
553+
HeadBranch: "master2",
554+
Title: "first pull request",
555+
ReviewerIDs: "2", // add user2 as reviewer
556+
})
541557

542558
// 3. validate the webhook is triggered
543559
assert.Equal(t, "pull_request", triggeredEvent)
@@ -549,6 +565,8 @@ func Test_WebhookPullRequest(t *testing.T) {
549565
assert.Equal(t, 0, *payloads[0].PullRequest.Additions)
550566
assert.Equal(t, 0, *payloads[0].PullRequest.ChangedFiles)
551567
assert.Equal(t, 0, *payloads[0].PullRequest.Deletions)
568+
assert.Len(t, payloads[0].PullRequest.RequestedReviewers, 1)
569+
assert.Equal(t, int64(2), payloads[0].PullRequest.RequestedReviewers[0].ID)
552570
})
553571
}
554572

0 commit comments

Comments
 (0)