From 3d2957ad1ae555576009823191aa942fc563bdd1 Mon Sep 17 00:00:00 2001 From: Harshit Bansal Date: Wed, 2 Jan 2019 11:00:53 +0530 Subject: [PATCH 1/2] branch: Close all the associated PRs when a branch is deleted. Fixes: #2466. --- models/action.go | 2 + models/fixtures/pull_request.yml | 2 +- models/issue.go | 14 ++++++ models/pull.go | 43 +++++++++++++++++ models/pull_test.go | 80 ++++++++++++++++++++++++++++++-- 5 files changed, 137 insertions(+), 4 deletions(-) diff --git a/models/action.go b/models/action.go index 5dc0eb18e699e..50b62d6e72357 100644 --- a/models/action.go +++ b/models/action.go @@ -669,6 +669,8 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { case ActionDeleteBranch: // Delete Branch isHookEventPush = true + CloseActivePullRequests(pusher, repo, refName) + if err = PrepareWebhooks(repo, HookEventDelete, &api.DeletePayload{ Ref: refName, RefType: "branch", diff --git a/models/fixtures/pull_request.yml b/models/fixtures/pull_request.yml index d8313f9f90585..989282cb05b99 100644 --- a/models/fixtures/pull_request.yml +++ b/models/fixtures/pull_request.yml @@ -22,7 +22,7 @@ head_repo_id: 1 base_repo_id: 1 head_user_name: user1 - head_branch: branch2 + head_branch: develop base_branch: master merge_base: fedcba9876543210 has_merged: false diff --git a/models/issue.go b/models/issue.go index 850346674b5a6..b9e038cc7d830 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1702,6 +1702,15 @@ func (issue *Issue) getBlockingDependencies(e Engine) (issueDeps []*Issue, err e Find(&issueDeps) } +// Returns true if the issue has any open issue as its dependency. +func (issue *Issue) isBlocked(e Engine) (bool, error) { + count, err := e.Table("issue_dependency"). + Join("INNER", "issue", "issue.id = issue_dependency.issue_id"). + Where("dependency_id = ? AND issue.is_closed = ?", issue.ID, false). + Count() + return count > 0, err +} + // BlockedByDependencies finds all Dependencies an issue is blocked by func (issue *Issue) BlockedByDependencies() ([]*Issue, error) { return issue.getBlockedByDependencies(x) @@ -1711,3 +1720,8 @@ func (issue *Issue) BlockedByDependencies() ([]*Issue, error) { func (issue *Issue) BlockingDependencies() ([]*Issue, error) { return issue.getBlockingDependencies(x) } + +// IsBlocked returns true if the issue has any open issue as its dependency. +func (issue *Issue) IsBlocked() (bool, error) { + return issue.isBlocked(x) +} diff --git a/models/pull.go b/models/pull.go index d84be139c9677..364ec5bdcb217 100644 --- a/models/pull.go +++ b/models/pull.go @@ -1472,3 +1472,46 @@ func TestPullRequests() { func InitTestPullRequests() { go TestPullRequests() } + +// CloseActivePullRequests closes the issue of all the active pull requests associated with a branch +func CloseActivePullRequests(doer *User, repo *Repository, branchName string) { + baseprs, err := GetUnmergedPullRequestsByBaseInfo(repo.ID, branchName) + if err != nil { + log.Error(4, "Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repo.ID, branchName, err) + return + } + headprs, err := GetUnmergedPullRequestsByHeadInfo(repo.ID, branchName) + if err != nil { + log.Error(4, "Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repo.ID, branchName, err) + return + } + prs := append(baseprs, headprs...) + for statusChanged := true; statusChanged; { + statusChanged = false + for _, pr := range prs { + if err = pr.LoadIssue(); err != nil { + log.Error(4, "LoadIssue: %v", err) + } + if pr.Issue.IsClosed { + continue + } + if blocked, err := pr.Issue.IsBlocked(); err != nil { + log.Error(4, "IsBlocked: %v", err) + } else if !blocked { + err = pr.Issue.ChangeStatus(doer, true) + if err != nil { + log.Error(4, "ChangeStatus: %v", err) + } else { + log.Trace("Issue [%d] status changed to closed", pr.IssueID) + statusChanged = true + } + } + } + } + + for _, pr := range prs { + if !pr.Issue.IsClosed { + log.Warn("Issue [%d] status change blocked by existing dependencies.", pr.Issue.ID) + } + } +} diff --git a/models/pull_test.go b/models/pull_test.go index 1dad664077711..53611a5389e5d 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -90,7 +90,7 @@ func TestPullRequestsOldest(t *testing.T) { func TestGetUnmergedPullRequest(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - pr, err := GetUnmergedPullRequest(1, 1, "branch2", "master") + pr, err := GetUnmergedPullRequest(1, 1, "develop", "master") assert.NoError(t, err) assert.Equal(t, int64(2), pr.ID) @@ -101,12 +101,12 @@ func TestGetUnmergedPullRequest(t *testing.T) { func TestGetUnmergedPullRequestsByHeadInfo(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - prs, err := GetUnmergedPullRequestsByHeadInfo(1, "branch2") + prs, err := GetUnmergedPullRequestsByHeadInfo(1, "develop") assert.NoError(t, err) assert.Len(t, prs, 1) for _, pr := range prs { assert.Equal(t, int64(1), pr.HeadRepoID) - assert.Equal(t, "branch2", pr.HeadBranch) + assert.Equal(t, "develop", pr.HeadBranch) } } @@ -268,3 +268,77 @@ func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) { pr.Issue.Title = "[wip] " + original assert.Equal(t, "[wip]", pr.GetWorkInProgressPrefix()) } + +func closeActivePullRequests(t *testing.T, pr *PullRequest, repo *Repository, branchName string) { + assert.NoError(t, repo.GetOwner()) + + CloseActivePullRequests(repo.Owner, repo, branchName) + pr.Issue = nil + pr.LoadIssue() + assert.Equal(t, true, pr.Issue.IsClosed) +} + +func TestPullRequest_CloseActivePullRequests(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + // Delete head branch. + pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest) + pr.GetHeadRepo() + closeActivePullRequests(t, pr, pr.HeadRepo, pr.HeadBranch) + + // Reopen pull request. + assert.NoError(t, pr.Issue.ChangeStatus(pr.HeadRepo.Owner, false)) + + // Delete base branch. + pr = AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest) + pr.GetBaseRepo() + closeActivePullRequests(t, pr, pr.BaseRepo, pr.BaseBranch) +} + +func createTestPullRequest(t *testing.T, repo *Repository, user *User, baseBranch string, headBranch string) *PullRequest { + prIssue := &Issue{ + RepoID: repo.ID, + Repo: repo, + Title: "test", + PosterID: user.ID, + Poster: user, + } + pr := &PullRequest{ + HeadRepoID: repo.ID, + BaseRepoID: repo.ID, + HeadRepo: repo, + BaseRepo: repo, + HeadUserName: user.Name, + HeadBranch: headBranch, + BaseBranch: baseBranch, + Type: PullRequestGitea, + } + labelIDs := make([]int64, 0) + uuids := make([]string, 0) + patch := make([]byte, 0) + assigneeIDs := make([]int64, 0) + err := NewPullRequest(repo, prIssue, labelIDs, uuids, pr, patch, assigneeIDs) + assert.NoError(t, err) + return pr +} + +func TestPullRequest_CloseActivePullRequestsDependent(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + // Create a test pull request. + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + user := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) + pr1 := AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest) + pr2 := createTestPullRequest(t, repo, user, "develop", "master") + pr1.LoadIssue() + pr2.LoadIssue() + + // Add a dependency from pr2 to pr1. + err := CreateIssueDependency(user, pr2.Issue, pr1.Issue) + assert.NoError(t, err) + + closeActivePullRequests(t, pr2, pr2.BaseRepo, pr2.BaseBranch) + pr1.Issue = nil + pr1.LoadIssue() + assert.Equal(t, true, pr1.Issue.IsClosed) +} From 70c1e28fa9a0ab98e6de67dc2c23d5a9af60639d Mon Sep 17 00:00:00 2001 From: techknowlogick Date: Tue, 7 May 2019 16:19:22 -0400 Subject: [PATCH 2/2] update to use new logging --- models/pull.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/models/pull.go b/models/pull.go index 0ebe8af5ad9c7..2378254482664 100644 --- a/models/pull.go +++ b/models/pull.go @@ -1532,12 +1532,12 @@ func InitTestPullRequests() { func CloseActivePullRequests(doer *User, repo *Repository, branchName string) { baseprs, err := GetUnmergedPullRequestsByBaseInfo(repo.ID, branchName) if err != nil { - log.Error(4, "Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repo.ID, branchName, err) + log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repo.ID, branchName, err) return } headprs, err := GetUnmergedPullRequestsByHeadInfo(repo.ID, branchName) if err != nil { - log.Error(4, "Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repo.ID, branchName, err) + log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repo.ID, branchName, err) return } prs := append(baseprs, headprs...) @@ -1545,17 +1545,17 @@ func CloseActivePullRequests(doer *User, repo *Repository, branchName string) { statusChanged = false for _, pr := range prs { if err = pr.LoadIssue(); err != nil { - log.Error(4, "LoadIssue: %v", err) + log.Error("LoadIssue: %v", err) } if pr.Issue.IsClosed { continue } if blocked, err := pr.Issue.IsBlocked(); err != nil { - log.Error(4, "IsBlocked: %v", err) + log.Error("IsBlocked: %v", err) } else if !blocked { err = pr.Issue.ChangeStatus(doer, true) if err != nil { - log.Error(4, "ChangeStatus: %v", err) + log.Error("ChangeStatus: %v", err) } else { log.Trace("Issue [%d] status changed to closed", pr.IssueID) statusChanged = true