diff --git a/modules/structs/pull.go b/modules/structs/pull.go index 55831e642c..d0f4eabc9e 100644 --- a/modules/structs/pull.go +++ b/modules/structs/pull.go @@ -27,9 +27,10 @@ type PullRequest struct { Comments int `json:"comments"` // number of review comments made on the diff of a PR review (not including comments on commits or issues in a PR) ReviewComments int `json:"review_comments"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - ChangedFiles int `json:"changed_files"` + + Additions *int `json:"additions,omitempty"` + Deletions *int `json:"deletions,omitempty"` + ChangedFiles *int `json:"changed_files,omitempty"` HTMLURL string `json:"html_url"` DiffURL string `json:"diff_url"` diff --git a/services/convert/pull.go b/services/convert/pull.go index 209d2bd79d..e0548eb635 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -239,9 +239,11 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u // Calculate diff startCommitID = pr.MergeBase - apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID) + diffChangedFiles, diffAdditions, diffDeletions, err := gitRepo.GetDiffShortStat(startCommitID, endCommitID) if err != nil { log.Error("GetDiffShortStat: %v", err) + } else { + apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions = &diffChangedFiles, &diffAdditions, &diffDeletions } } @@ -459,12 +461,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs return nil, err } - // Outer scope variables to be used in diff calculation - var ( - startCommitID string - endCommitID string - ) - if git.IsErrBranchNotExist(err) { headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref) if err != nil && !git.IsErrNotExist(err) { @@ -473,7 +469,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs } if err == nil { apiPullRequest.Head.Sha = headCommitID - endCommitID = headCommitID } } else { commit, err := headBranch.GetCommit() @@ -484,17 +479,8 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs if err == nil { apiPullRequest.Head.Ref = pr.HeadBranch apiPullRequest.Head.Sha = commit.ID.String() - endCommitID = commit.ID.String() } } - - // Calculate diff - startCommitID = pr.MergeBase - - apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID) - if err != nil { - log.Error("GetDiffShortStat: %v", err) - } } if len(apiPullRequest.Head.Sha) == 0 && len(apiPullRequest.Head.Ref) != 0 { diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 969e110895..4dfc8a332a 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -51,7 +51,6 @@ func TestAPIViewPulls(t *testing.T) { assert.Empty(t, pull.RequestedReviewersTeams) assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID) assert.EqualValues(t, 6, pull.RequestedReviewers[1].ID) - assert.EqualValues(t, 1, pull.ChangedFiles) if assert.EqualValues(t, 5, pull.ID) { resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK) @@ -59,22 +58,23 @@ func TestAPIViewPulls(t *testing.T) { assert.NoError(t, err) patch, err := gitdiff.ParsePatch(context.Background(), 1000, 5000, 10, bytes.NewReader(bs), "") assert.NoError(t, err) - if assert.Len(t, patch.Files, pull.ChangedFiles) { + if assert.Len(t, patch.Files, 1) { assert.Equal(t, "File-WoW", patch.Files[0].Name) // FIXME: The old name should be empty if it's a file add type assert.Equal(t, "File-WoW", patch.Files[0].OldName) - assert.EqualValues(t, pull.Additions, patch.Files[0].Addition) - assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion) + assert.EqualValues(t, 1, patch.Files[0].Addition) + assert.EqualValues(t, 0, patch.Files[0].Deletion) assert.Equal(t, gitdiff.DiffFileAdd, patch.Files[0].Type) } t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID), doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) { - if assert.Len(t, files, pull.ChangedFiles) { + if assert.Len(t, files, 1) { assert.Equal(t, "File-WoW", files[0].Filename) assert.Empty(t, files[0].PreviousFilename) - assert.EqualValues(t, pull.Additions, files[0].Additions) - assert.EqualValues(t, pull.Deletions, files[0].Deletions) + assert.EqualValues(t, 1, files[0].Additions) + assert.EqualValues(t, 1, files[0].Changes) + assert.EqualValues(t, 0, files[0].Deletions) assert.Equal(t, "added", files[0].Status) } })) @@ -88,7 +88,6 @@ func TestAPIViewPulls(t *testing.T) { assert.EqualValues(t, 4, pull.RequestedReviewers[1].ID) assert.EqualValues(t, 2, pull.RequestedReviewers[2].ID) assert.EqualValues(t, 5, pull.RequestedReviewers[3].ID) - assert.EqualValues(t, 1, pull.ChangedFiles) if assert.EqualValues(t, 2, pull.ID) { resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK) @@ -96,45 +95,44 @@ func TestAPIViewPulls(t *testing.T) { assert.NoError(t, err) patch, err := gitdiff.ParsePatch(context.Background(), 1000, 5000, 10, bytes.NewReader(bs), "") assert.NoError(t, err) - if assert.Len(t, patch.Files, pull.ChangedFiles) { + if assert.Len(t, patch.Files, 1) { assert.Equal(t, "README.md", patch.Files[0].Name) assert.Equal(t, "README.md", patch.Files[0].OldName) - assert.EqualValues(t, pull.Additions, patch.Files[0].Addition) - assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion) + assert.EqualValues(t, 4, patch.Files[0].Addition) + assert.EqualValues(t, 1, patch.Files[0].Deletion) assert.Equal(t, gitdiff.DiffFileChange, patch.Files[0].Type) } t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID), doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) { - if assert.Len(t, files, pull.ChangedFiles) { + if assert.Len(t, files, 1) { assert.Equal(t, "README.md", files[0].Filename) // FIXME: The PreviousFilename name should be the same as Filename if it's a file change assert.Equal(t, "", files[0].PreviousFilename) - assert.EqualValues(t, pull.Additions, files[0].Additions) - assert.EqualValues(t, pull.Deletions, files[0].Deletions) + assert.EqualValues(t, 4, files[0].Additions) + assert.EqualValues(t, 1, files[0].Deletions) assert.Equal(t, "changed", files[0].Status) } })) } - pull = pulls[2] + pull = pulls[0] assert.EqualValues(t, 1, pull.Poster.ID) - assert.Len(t, pull.RequestedReviewers, 1) + assert.Len(t, pull.RequestedReviewers, 2) assert.Empty(t, pull.RequestedReviewersTeams) - assert.EqualValues(t, 1, pull.RequestedReviewers[0].ID) - assert.EqualValues(t, 0, pull.ChangedFiles) + assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID) - if assert.EqualValues(t, 1, pull.ID) { + if assert.EqualValues(t, 5, pull.ID) { resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK) bs, err := io.ReadAll(resp.Body) assert.NoError(t, err) patch, err := gitdiff.ParsePatch(context.Background(), 1000, 5000, 10, bytes.NewReader(bs), "") assert.NoError(t, err) - assert.EqualValues(t, pull.ChangedFiles, patch.NumFiles) + assert.EqualValues(t, 1, patch.NumFiles) t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID), doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) { - assert.Len(t, files, pull.ChangedFiles) + assert.Len(t, files, 1) })) } } diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 5a50d7d491..0952263968 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -25,6 +25,7 @@ import ( "github.com/PuerkitoBio/goquery" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewWebHookLink(t *testing.T) { @@ -378,12 +379,14 @@ func Test_WebhookPullRequest(t *testing.T) { // 3. validate the webhook is triggered assert.EqualValues(t, "pull_request", triggeredEvent) - assert.Len(t, payloads, 1) + require.Len(t, payloads, 1) assert.EqualValues(t, "repo1", payloads[0].PullRequest.Base.Repository.Name) assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Base.Repository.FullName) assert.EqualValues(t, "repo1", payloads[0].PullRequest.Head.Repository.Name) assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Head.Repository.FullName) - assert.EqualValues(t, 0, payloads[0].PullRequest.Additions) + assert.EqualValues(t, 0, *payloads[0].PullRequest.Additions) + assert.EqualValues(t, 0, *payloads[0].PullRequest.ChangedFiles) + assert.EqualValues(t, 0, *payloads[0].PullRequest.Deletions) }) }