From 384c2b342ec01fadb520572666127cb5564e1050 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 10 Jan 2020 15:53:53 +0800 Subject: [PATCH] Move pull request api convert to convert package (#9664) * Move pull request api convert to convert package * Rename ToPullRequest to ToAPIPullRequest --- models/pull.go | 134 ---------------------- models/pull_test.go | 10 -- modules/convert/main_test.go | 16 +++ modules/convert/pull.go | 141 ++++++++++++++++++++++++ modules/convert/pull_test.go | 23 ++++ modules/notification/webhook/webhook.go | 25 +++-- routers/api/v1/repo/pull.go | 9 +- 7 files changed, 198 insertions(+), 160 deletions(-) create mode 100644 modules/convert/main_test.go create mode 100644 modules/convert/pull.go create mode 100644 modules/convert/pull_test.go diff --git a/models/pull.go b/models/pull.go index a7f51683d..e2a35c592 100644 --- a/models/pull.go +++ b/models/pull.go @@ -13,7 +13,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" ) @@ -397,139 +396,6 @@ func (pr *PullRequest) GetGitRefName() string { return fmt.Sprintf("refs/pull/%d/head", pr.Index) } -// APIFormat assumes following fields have been assigned with valid values: -// Required - Issue -// Optional - Merger -func (pr *PullRequest) APIFormat() *api.PullRequest { - return pr.apiFormat(x) -} - -func (pr *PullRequest) apiFormat(e Engine) *api.PullRequest { - var ( - baseBranch *git.Branch - headBranch *git.Branch - baseCommit *git.Commit - headCommit *git.Commit - err error - ) - if err = pr.Issue.loadRepo(e); err != nil { - log.Error("loadRepo[%d]: %v", pr.ID, err) - return nil - } - apiIssue := pr.Issue.apiFormat(e) - if pr.BaseRepo == nil { - pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID) - if err != nil { - log.Error("GetRepositoryById[%d]: %v", pr.ID, err) - return nil - } - } - if pr.HeadRepo == nil { - pr.HeadRepo, err = getRepositoryByID(e, pr.HeadRepoID) - if err != nil { - log.Error("GetRepositoryById[%d]: %v", pr.ID, err) - return nil - } - } - - if err = pr.Issue.loadRepo(e); err != nil { - log.Error("pr.Issue.loadRepo[%d]: %v", pr.ID, err) - return nil - } - - apiPullRequest := &api.PullRequest{ - ID: pr.ID, - URL: pr.Issue.HTMLURL(), - Index: pr.Index, - Poster: apiIssue.Poster, - Title: apiIssue.Title, - Body: apiIssue.Body, - Labels: apiIssue.Labels, - Milestone: apiIssue.Milestone, - Assignee: apiIssue.Assignee, - Assignees: apiIssue.Assignees, - State: apiIssue.State, - Comments: apiIssue.Comments, - HTMLURL: pr.Issue.HTMLURL(), - DiffURL: pr.Issue.DiffURL(), - PatchURL: pr.Issue.PatchURL(), - HasMerged: pr.HasMerged, - MergeBase: pr.MergeBase, - Deadline: apiIssue.Deadline, - Created: pr.Issue.CreatedUnix.AsTimePtr(), - Updated: pr.Issue.UpdatedUnix.AsTimePtr(), - } - baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch) - if err != nil { - if git.IsErrBranchNotExist(err) { - apiPullRequest.Base = nil - } else { - log.Error("GetBranch[%s]: %v", pr.BaseBranch, err) - return nil - } - } else { - apiBaseBranchInfo := &api.PRBranchInfo{ - Name: pr.BaseBranch, - Ref: pr.BaseBranch, - RepoID: pr.BaseRepoID, - Repository: pr.BaseRepo.innerAPIFormat(e, AccessModeNone, false), - } - baseCommit, err = baseBranch.GetCommit() - if err != nil { - if git.IsErrNotExist(err) { - apiBaseBranchInfo.Sha = "" - } else { - log.Error("GetCommit[%s]: %v", baseBranch.Name, err) - return nil - } - } else { - apiBaseBranchInfo.Sha = baseCommit.ID.String() - } - apiPullRequest.Base = apiBaseBranchInfo - } - - headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch) - if err != nil { - if git.IsErrBranchNotExist(err) { - apiPullRequest.Head = nil - } else { - log.Error("GetBranch[%s]: %v", pr.HeadBranch, err) - return nil - } - } else { - apiHeadBranchInfo := &api.PRBranchInfo{ - Name: pr.HeadBranch, - Ref: pr.HeadBranch, - RepoID: pr.HeadRepoID, - Repository: pr.HeadRepo.innerAPIFormat(e, AccessModeNone, false), - } - headCommit, err = headBranch.GetCommit() - if err != nil { - if git.IsErrNotExist(err) { - apiHeadBranchInfo.Sha = "" - } else { - log.Error("GetCommit[%s]: %v", headBranch.Name, err) - return nil - } - } else { - apiHeadBranchInfo.Sha = headCommit.ID.String() - } - apiPullRequest.Head = apiHeadBranchInfo - } - - if pr.Status != PullRequestStatusChecking { - mergeable := !(pr.Status == PullRequestStatusConflict || pr.Status == PullRequestStatusError) && !pr.IsWorkInProgress() - apiPullRequest.Mergeable = mergeable - } - if pr.HasMerged { - apiPullRequest.Merged = pr.MergedUnix.AsTimePtr() - apiPullRequest.MergedCommitID = &pr.MergedCommitID - apiPullRequest.MergedBy = pr.Merger.APIFormat() - } - - return apiPullRequest -} - func (pr *PullRequest) getHeadRepo(e Engine) (err error) { pr.HeadRepo, err = getRepositoryByID(e, pr.HeadRepoID) if err != nil && !IsErrRepoNotExist(err) { diff --git a/models/pull_test.go b/models/pull_test.go index 153739f54..9c27b603a 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -29,16 +29,6 @@ func TestPullRequest_LoadIssue(t *testing.T) { assert.Equal(t, int64(2), pr.Issue.ID) } -func TestPullRequest_APIFormat(t *testing.T) { - assert.NoError(t, PrepareTestDatabase()) - pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) - assert.NoError(t, pr.LoadAttributes()) - assert.NoError(t, pr.LoadIssue()) - apiPullRequest := pr.APIFormat() - assert.NotNil(t, apiPullRequest) - assert.Nil(t, apiPullRequest.Head) -} - func TestPullRequest_GetBaseRepo(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) diff --git a/modules/convert/main_test.go b/modules/convert/main_test.go new file mode 100644 index 000000000..8720ff042 --- /dev/null +++ b/modules/convert/main_test.go @@ -0,0 +1,16 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package convert + +import ( + "path/filepath" + "testing" + + "code.gitea.io/gitea/models" +) + +func TestMain(m *testing.M) { + models.MainTest(m, filepath.Join("..", "..")) +} diff --git a/modules/convert/pull.go b/modules/convert/pull.go new file mode 100644 index 000000000..f8534f839 --- /dev/null +++ b/modules/convert/pull.go @@ -0,0 +1,141 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package convert + +import ( + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" + api "code.gitea.io/gitea/modules/structs" +) + +// ToAPIPullRequest assumes following fields have been assigned with valid values: +// Required - Issue +// Optional - Merger +func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { + var ( + baseBranch *git.Branch + headBranch *git.Branch + baseCommit *git.Commit + headCommit *git.Commit + err error + ) + if err = pr.Issue.LoadRepo(); err != nil { + log.Error("loadRepo[%d]: %v", pr.ID, err) + return nil + } + apiIssue := pr.Issue.APIFormat() + if pr.BaseRepo == nil { + pr.BaseRepo, err = models.GetRepositoryByID(pr.BaseRepoID) + if err != nil { + log.Error("GetRepositoryById[%d]: %v", pr.ID, err) + return nil + } + } + if pr.HeadRepo == nil { + pr.HeadRepo, err = models.GetRepositoryByID(pr.HeadRepoID) + if err != nil { + log.Error("GetRepositoryById[%d]: %v", pr.ID, err) + return nil + } + } + + if err = pr.Issue.LoadRepo(); err != nil { + log.Error("pr.Issue.loadRepo[%d]: %v", pr.ID, err) + return nil + } + + apiPullRequest := &api.PullRequest{ + ID: pr.ID, + URL: pr.Issue.HTMLURL(), + Index: pr.Index, + Poster: apiIssue.Poster, + Title: apiIssue.Title, + Body: apiIssue.Body, + Labels: apiIssue.Labels, + Milestone: apiIssue.Milestone, + Assignee: apiIssue.Assignee, + Assignees: apiIssue.Assignees, + State: apiIssue.State, + Comments: apiIssue.Comments, + HTMLURL: pr.Issue.HTMLURL(), + DiffURL: pr.Issue.DiffURL(), + PatchURL: pr.Issue.PatchURL(), + HasMerged: pr.HasMerged, + MergeBase: pr.MergeBase, + Deadline: apiIssue.Deadline, + Created: pr.Issue.CreatedUnix.AsTimePtr(), + Updated: pr.Issue.UpdatedUnix.AsTimePtr(), + } + baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch) + if err != nil { + if git.IsErrBranchNotExist(err) { + apiPullRequest.Base = nil + } else { + log.Error("GetBranch[%s]: %v", pr.BaseBranch, err) + return nil + } + } else { + apiBaseBranchInfo := &api.PRBranchInfo{ + Name: pr.BaseBranch, + Ref: pr.BaseBranch, + RepoID: pr.BaseRepoID, + Repository: pr.BaseRepo.APIFormat(models.AccessModeNone), + } + baseCommit, err = baseBranch.GetCommit() + if err != nil { + if git.IsErrNotExist(err) { + apiBaseBranchInfo.Sha = "" + } else { + log.Error("GetCommit[%s]: %v", baseBranch.Name, err) + return nil + } + } else { + apiBaseBranchInfo.Sha = baseCommit.ID.String() + } + apiPullRequest.Base = apiBaseBranchInfo + } + + headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch) + if err != nil { + if git.IsErrBranchNotExist(err) { + apiPullRequest.Head = nil + } else { + log.Error("GetBranch[%s]: %v", pr.HeadBranch, err) + return nil + } + } else { + apiHeadBranchInfo := &api.PRBranchInfo{ + Name: pr.HeadBranch, + Ref: pr.HeadBranch, + RepoID: pr.HeadRepoID, + Repository: pr.HeadRepo.APIFormat(models.AccessModeNone), + } + headCommit, err = headBranch.GetCommit() + if err != nil { + if git.IsErrNotExist(err) { + apiHeadBranchInfo.Sha = "" + } else { + log.Error("GetCommit[%s]: %v", headBranch.Name, err) + return nil + } + } else { + apiHeadBranchInfo.Sha = headCommit.ID.String() + } + apiPullRequest.Head = apiHeadBranchInfo + } + + if pr.Status != models.PullRequestStatusChecking { + mergeable := !(pr.Status == models.PullRequestStatusConflict || pr.Status == models.PullRequestStatusError) && !pr.IsWorkInProgress() + apiPullRequest.Mergeable = mergeable + } + if pr.HasMerged { + apiPullRequest.Merged = pr.MergedUnix.AsTimePtr() + apiPullRequest.MergedCommitID = &pr.MergedCommitID + apiPullRequest.MergedBy = pr.Merger.APIFormat() + } + + return apiPullRequest +} diff --git a/modules/convert/pull_test.go b/modules/convert/pull_test.go new file mode 100644 index 000000000..9055d555e --- /dev/null +++ b/modules/convert/pull_test.go @@ -0,0 +1,23 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package convert + +import ( + "testing" + + "code.gitea.io/gitea/models" + + "github.com/stretchr/testify/assert" +) + +func TestPullRequest_APIFormat(t *testing.T) { + assert.NoError(t, models.PrepareTestDatabase()) + pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + assert.NoError(t, pr.LoadAttributes()) + assert.NoError(t, pr.LoadIssue()) + apiPullRequest := ToAPIPullRequest(pr) + assert.NotNil(t, apiPullRequest) + assert.Nil(t, apiPullRequest.Head) +} diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index e0801445d..cf4117666 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -6,6 +6,7 @@ package webhook import ( "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/convert" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification/base" @@ -49,7 +50,7 @@ func (m *webhookNotifier) NotifyIssueClearLabels(doer *models.User, issue *model err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueLabelCleared, Index: issue.Index, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -135,7 +136,7 @@ func (m *webhookNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *mo issue.PullRequest.Issue = issue apiPullRequest := &api.PullRequestPayload{ Index: issue.Index, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), } @@ -187,7 +188,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model From: oldTitle, }, }, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -222,7 +223,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *mode // Merge pull request calls issue.changeStatus so we need to handle separately. apiPullRequest := &api.PullRequestPayload{ Index: issue.Index, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), } @@ -291,7 +292,7 @@ func (m *webhookNotifier) NotifyNewPullRequest(pull *models.PullRequest) { if err := webhook_module.PrepareWebhooks(pull.Issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueOpened, Index: pull.Issue.Index, - PullRequest: pull.APIFormat(), + PullRequest: convert.ToAPIPullRequest(pull), Repository: pull.Issue.Repo.APIFormat(mode), Sender: pull.Issue.Poster.APIFormat(), }); err != nil { @@ -312,7 +313,7 @@ func (m *webhookNotifier) NotifyIssueChangeContent(doer *models.User, issue *mod From: oldContent, }, }, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -439,7 +440,7 @@ func (m *webhookNotifier) NotifyIssueChangeLabels(doer *models.User, issue *mode err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueLabelUpdated, Index: issue.Index, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(models.AccessModeNone), Sender: doer.APIFormat(), }) @@ -481,7 +482,7 @@ func (m *webhookNotifier) NotifyIssueChangeMilestone(doer *models.User, issue *m err = webhook_module.PrepareWebhooks(issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ Action: hookAction, Index: issue.Index, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -547,7 +548,7 @@ func (*webhookNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mod // Merge pull request calls issue.changeStatus so we need to handle separately. apiPullRequest := &api.PullRequestPayload{ Index: pr.Issue.Index, - PullRequest: pr.APIFormat(), + PullRequest: convert.ToAPIPullRequest(pr), Repository: pr.Issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), Action: api.HookIssueClosed, @@ -580,7 +581,7 @@ func (m *webhookNotifier) NotifyPullRequestChangeTargetBranch(doer *models.User, From: oldBranch, }, }, - PullRequest: issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest), Repository: issue.Repo.APIFormat(mode), Sender: doer.APIFormat(), }) @@ -619,7 +620,7 @@ func (m *webhookNotifier) NotifyPullRequestReview(pr *models.PullRequest, review if err := webhook_module.PrepareWebhooks(review.Issue.Repo, reviewHookType, &api.PullRequestPayload{ Action: api.HookIssueSynchronized, Index: review.Issue.Index, - PullRequest: pr.APIFormat(), + PullRequest: convert.ToAPIPullRequest(pr), Repository: review.Issue.Repo.APIFormat(mode), Sender: review.Reviewer.APIFormat(), Review: &api.ReviewPayload{ @@ -674,7 +675,7 @@ func (m *webhookNotifier) NotifyPullRequestSynchronized(doer *models.User, pr *m if err := webhook_module.PrepareWebhooks(pr.Issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueSynchronized, Index: pr.Issue.Index, - PullRequest: pr.Issue.PullRequest.APIFormat(), + PullRequest: convert.ToAPIPullRequest(pr), Repository: pr.Issue.Repo.APIFormat(models.AccessModeNone), Sender: doer.APIFormat(), }); err != nil { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 85ef41978..8e6677116 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/convert" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" @@ -102,7 +103,7 @@ func ListPullRequests(ctx *context.APIContext, form api.ListPullRequestsOptions) ctx.Error(http.StatusInternalServerError, "GetHeadRepo", err) return } - apiPrs[i] = prs[i].APIFormat() + apiPrs[i] = convert.ToAPIPullRequest(prs[i]) } ctx.SetLinkHeader(int(maxResults), models.ItemsPerPage) @@ -157,7 +158,7 @@ func GetPullRequest(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "GetHeadRepo", err) return } - ctx.JSON(http.StatusOK, pr.APIFormat()) + ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(pr)) } // CreatePullRequest does what it says @@ -321,7 +322,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption notification.NotifyNewPullRequest(pr) log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID) - ctx.JSON(http.StatusCreated, pr.APIFormat()) + ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr)) } // EditPullRequest does what it says @@ -479,7 +480,7 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) { } // TODO this should be 200, not 201 - ctx.JSON(http.StatusCreated, pr.APIFormat()) + ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr)) } // IsPullRequestMerged checks if a PR exists given an index