Mirrors/gitea
0
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2025-09-09 09:04:08 +02:00
Code Issues Activity

Fix review request webhook bug (#35339)

Browse Source 
partially backport #35337
Fix #35327
This commit is contained in:
Lunny Xiao 2025-08-28 21:26:29 -07:00 committed by GitHub
parent 1b8efb6fc7
commit 5e10def7f7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 129 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
services/pull/pull.go
View File

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

10
tests/integration/pull_compare_test.go
View File

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

54
tests/integration/pull_create_test.go
View File

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

20
tests/integration/pull_review_test.go
View File

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

26
tests/integration/repo_webhook_test.go
View File

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