only return head: null if source branch was deleted (#6705)

* only return head: null if source branch was deleted * add URL into GetPullRequest * TestPullRequest_APIFormat * log error if it is not Err(Branch)NotExist
go-gitea · Jul 8, 2019 · ff85a63 · ff85a63
1 parent cb2ec41
commit ff85a63
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 33 deletions.
diff --git a/models/pull.go b/models/pull.go
@@ -189,36 +189,6 @@ func (pr *PullRequest) apiFormat(e Engine) *api.PullRequest {
 			return nil
 		}
 	}
-	if baseBranch, err = pr.BaseRepo.GetBranch(pr.BaseBranch); err != nil {
-		log.Error("pr.BaseRepo.GetBranch[%d]: %v", pr.BaseBranch, err)
-		return nil
-	}
-	if baseCommit, err = baseBranch.GetCommit(); err != nil {
-		log.Error("baseBranch.GetCommit[%d]: %v", pr.ID, err)
-		return nil
-	}
-	if headBranch, err = pr.HeadRepo.GetBranch(pr.HeadBranch); err != nil {
-		log.Error("pr.HeadRepo.GetBranch[%d]: %v", pr.HeadBranch, err)
-		return nil
-	}
-	if headCommit, err = headBranch.GetCommit(); err != nil {
-		log.Error("headBranch.GetCommit[%d]: %v", pr.ID, err)
-		return nil
-	}
-	apiBaseBranchInfo := &api.PRBranchInfo{
-		Name:       pr.BaseBranch,
-		Ref:        pr.BaseBranch,
-		Sha:        baseCommit.ID.String(),
-		RepoID:     pr.BaseRepoID,
-		Repository: pr.BaseRepo.innerAPIFormat(e, AccessModeNone, false),
-	}
-	apiHeadBranchInfo := &api.PRBranchInfo{
-		Name:       pr.HeadBranch,
-		Ref:        pr.HeadBranch,
-		Sha:        headCommit.ID.String(),
-		RepoID:     pr.HeadRepoID,
-		Repository: pr.HeadRepo.innerAPIFormat(e, AccessModeNone, false),
-	}
 
 	if err = pr.Issue.loadRepo(e); err != nil {
 		log.Error("pr.Issue.loadRepo[%d]: %v", pr.ID, err)
@@ -227,6 +197,7 @@ func (pr *PullRequest) apiFormat(e Engine) *api.PullRequest {
 
 	apiPullRequest := &api.PullRequest{
 		ID:        pr.ID,
+		URL:       pr.Issue.HTMLURL(),
 		Index:     pr.Index,
 		Poster:    apiIssue.Poster,
 		Title:     apiIssue.Title,
@@ -241,13 +212,68 @@ func (pr *PullRequest) apiFormat(e Engine) *api.PullRequest {
 		DiffURL:   pr.Issue.DiffURL(),
 		PatchURL:  pr.Issue.PatchURL(),
 		HasMerged: pr.HasMerged,
-		Base:      apiBaseBranchInfo,
-		Head:      apiHeadBranchInfo,
 		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.IsWorkInProgress()

diff --git a/models/pull_test.go b/models/pull_test.go
@@ -31,7 +31,15 @@ func TestPullRequest_LoadIssue(t *testing.T) {
 	assert.Equal(t, int64(2), pr.Issue.ID)
 }
 
-// TODO TestPullRequest_APIFormat
+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())