Move pull request api convert to convert package (#9664)

* Move pull request api convert to convert package

* Rename ToPullRequest to ToAPIPullRequest

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

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)

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("..", ".."))
+}

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

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

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 {

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