fix(pulls): hide branch deletion when an open PR uses the branch (#39193) (#39218)

Co-authored-by: 开心 <happys2333@outlook.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>

routers/web/repo/issue_view.go

@@ -594,7 +594,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxDeleteBranch(ctx *context.Cont
 		isPullBranchDeletable, _ = git_model.IsBranchExist(ctx, pull.HeadRepo.ID, pull.HeadBranch)
 	}
 
-	if isPullBranchDeletable && pull.HasMerged {
+	if isPullBranchDeletable && prInfo.issue.IsClosed {
 		exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pull.HeadRepoID, pull.HeadBranch)
 		if err != nil {
 			ctx.ServerError("HasUnmergedPullRequestsByHeadInfo", err)

tests/integration/pull_merge_test.go

@@ -81,20 +81,6 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullNum strin
 	return resp
 }
 
-func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum string) *httptest.ResponseRecorder {
-	req := NewRequest(t, "GET", "/"+path.Join(user, repo, "pulls", pullnum))
-	resp := session.MakeRequest(t, req, http.StatusOK)
-
-	// Click the little button to create a pull
-	htmlDoc := NewHTMLParser(t, resp.Body)
-	link, exists := htmlDoc.doc.Find(".timeline-item .delete-branch-after-merge").Attr("data-url")
-	assert.True(t, exists, "The template has changed, can not find delete button url")
-	req = NewRequest(t, "POST", link)
-	resp = session.MakeRequest(t, req, http.StatusOK)
-
-	return resp
-}
-
 func preparePullMergeWebhook(t *testing.T, repoID int64) {
 	require.NoError(t, db.TruncateBeans(t.Context(), &webhook.Webhook{}, &webhook.HookTask{}))
 	require.NoError(t, db.Insert(t.Context(), &webhook.Webhook{
@@ -267,7 +253,7 @@ func TestPullSquashWithHeadCommitID(t *testing.T) {
 	})
 }
 
-func TestPullCleanUpAfterMerge(t *testing.T) {
+func TestPullCleanUpAfterClose(t *testing.T) {
 	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
 		session := loginUser(t, "user1") // FIXME: don't use admin user for testing
 		testRepoFork(t, session, "user2", "repo1", "user1", "repo1", "")
@@ -277,39 +263,60 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
 		assert.Equal(t, 3, repo.NumPulls)
 		assert.Equal(t, 3, repo.NumOpenPulls)
 
-		resp := testPullCreate(t, session, "user1", "repo1", false, "master", "feature/test", "This is a pull title")
+		getDeleteBranchLink := func(t *testing.T, session *TestSession, user, repo, pullnum string) string {
+			req := NewRequest(t, "GET", "/"+path.Join(user, repo, "pulls", pullnum))
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			return htmlDoc.doc.Find(".timeline-item .delete-branch-after-merge").AttrOr("data-url", "")
+		}
 
-		elem := strings.Split(test.RedirectURL(resp), "/")
+		var closedPullNumStr string
-		assert.Equal(t, "pulls", elem[3])
+		t.Run("CreateAndClosePR", func(t *testing.T) {
+			resp := testPullCreate(t, session, "user1", "repo1", false, "master", "feature/test", "This is a pull title")
+			pullNumStr := path.Base(test.RedirectURL(resp))
 
-		repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repo.ID})
+			testIssueClose(t, session, "user2", "repo1", pullNumStr)
-		assert.Equal(t, 4, repo.NumPulls)
-		assert.Equal(t, 4, repo.NumOpenPulls)
 
-		testPullMerge(t, session, elem[1], elem[2], elem[4], MergeOptions{
+			repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repo.ID})
-			Style:        repo_model.MergeStyleMerge,
+			assert.Equal(t, 4, repo.NumPulls)
-			DeleteBranch: false,
+			assert.Equal(t, 3, repo.NumOpenPulls)
+
+			closedPullNumStr = pullNumStr
+
+			// the closed but unmerged PR should have the "delete branch" button
+			link := getDeleteBranchLink(t, session, "user2", "repo1", closedPullNumStr)
+			assert.NotEmpty(t, link)
 		})
 
-		repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repo.ID})
+		t.Run("CreateAndMergePR", func(t *testing.T) {
-		assert.Equal(t, 4, repo.NumPulls)
+			resp := testPullCreate(t, session, "user1", "repo1", false, "master", "feature/test", "This is a pull title")
-		assert.Equal(t, 3, repo.NumOpenPulls)
+			pullNumStr := path.Base(test.RedirectURL(resp))
 
-		// Check PR branch deletion
+			// the closed but unmerged PR should not have the "delete branch" button because there is a new PR for the same branch
-		resp = testPullCleanUp(t, session, elem[1], elem[2], elem[4])
+			link := getDeleteBranchLink(t, session, "user2", "repo1", closedPullNumStr)
-		respJSON := test.ParseJSONRedirect(resp.Body.Bytes())
+			assert.Empty(t, link)
-		require.NotEmpty(t, respJSON.Redirect, "Redirected URL is not found")
 
-		elem = strings.Split(*respJSON.Redirect, "/")
+			testPullMerge(t, session, "user2", "repo1", pullNumStr, MergeOptions{
-		assert.Equal(t, "pulls", elem[3])
+				Style:        repo_model.MergeStyleMerge,
+				DeleteBranch: false,
+			})
 
-		// Check branch deletion result
+			// Check PR branch deletion
-		req := NewRequest(t, "GET", *respJSON.Redirect)
+			link = getDeleteBranchLink(t, session, "user2", "repo1", pullNumStr)
-		resp = session.MakeRequest(t, req, http.StatusOK)
+			assert.NotEmpty(t, link)
+			resp = session.MakeRequest(t, NewRequest(t, "POST", link), http.StatusOK)
 
-		htmlDoc := NewHTMLParser(t, resp.Body)
+			// Check branch deletion result
-		resultMsg := strings.TrimSpace(htmlDoc.doc.Find(".ui.message.flash-message").Text())
+			req := NewRequest(t, "GET", test.RedirectURL(resp))
-		assert.Equal(t, `Branch "user1/repo1:feature/test" has been deleted.`, resultMsg)
+			resp = session.MakeRequest(t, req, http.StatusOK)
+			htmlDoc := NewHTMLParser(t, resp.Body)
+			resultMsg := strings.TrimSpace(htmlDoc.doc.Find(".ui.message.flash-message").Text())
+			assert.Equal(t, `Branch "user1/repo1:feature/test" has been deleted.`, resultMsg)
+
+			// the "delete branch" button should be gone since the PR has been merged
+			link = getDeleteBranchLink(t, session, "user2", "repo1", pullNumStr)
+			assert.Empty(t, link)
+		})
 	})
 }