From f779aa79f2fdedf37c29ec79c179e1285b87a249 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 17 Feb 2024 15:30:41 +0100 Subject: [PATCH] [BUG] Fix pull request reopen conditions - Move the conditions code around, such that the existence of the head and base is first checked (so a clear error can be given, instead of a possible server error). This makes it easier to read this code. As the logic is now grouped together. - Adds integration testing that simulates the deletion of the base and head branch and ensures the pull request cannot be opened. The 'normal' testcase also 'informally' ensures that the previous incorrect condition is not there, because the branch `base-branch` doesn't exist on the head repository. - Resolves #2321 --- options/locale/locale_en-US.ini | 2 + routers/web/repo/issue.go | 29 ++-- tests/integration/pull_reopen_test.go | 214 ++++++++++++++++++++++++++ 3 files changed, 234 insertions(+), 11 deletions(-) create mode 100644 tests/integration/pull_reopen_test.go diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index e81d493b19..e3707243a5 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1859,6 +1859,8 @@ pulls.cmd_instruction_merge_title = Merge pulls.cmd_instruction_merge_desc = Merge the changes and update on Forgejo. pulls.clear_merge_message = Clear merge message pulls.clear_merge_message_hint = Clearing the merge message will only remove the commit message content and keep generated git trailers such as "Co-Authored-By …". +pulls.reopen_failed.head_branch = The pull request cannot be reopened, because the head branch doesn't exist anymore. +pulls.reopen_failed.base_branch = The pull request cannot be reopened, because the base branch doesn't exist anymore. pulls.auto_merge_button_when_succeed = (When checks succeed) pulls.auto_merge_when_succeed = Auto merge when all checks succeed diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 7b171510bb..f4e9203c9f 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3028,27 +3028,34 @@ func NewComment(ctx *context.Context) { // check whether the ref of PR in base repo is consistent with the head commit of head branch in the head repo // get head commit of PR if pull.Flow == issues_model.PullRequestFlowGithub { - prHeadRef := pull.GetGitRefName() if err := pull.LoadBaseRepo(ctx); err != nil { ctx.ServerError("Unable to load base repo", err) return } + if err := pull.LoadHeadRepo(ctx); err != nil { + ctx.ServerError("Unable to load head repo", err) + return + } + + // Check if the base branch of the pull request still exists. + if ok := git.IsBranchExist(ctx, pull.BaseRepo.RepoPath(), pull.BaseBranch); !ok { + ctx.JSONError(ctx.Tr("repo.pulls.reopen_failed.base_branch")) + return + } + + // Check if the head branch of the pull request still exists. + if ok := git.IsBranchExist(ctx, pull.HeadRepo.RepoPath(), pull.HeadBranch); !ok { + ctx.JSONError(ctx.Tr("repo.pulls.reopen_failed.head_branch")) + return + } + + prHeadRef := pull.GetGitRefName() prHeadCommitID, err := git.GetFullCommitID(ctx, pull.BaseRepo.RepoPath(), prHeadRef) if err != nil { ctx.ServerError("Get head commit Id of pr fail", err) return } - // get head commit of branch in the head repo - if err := pull.LoadHeadRepo(ctx); err != nil { - ctx.ServerError("Unable to load head repo", err) - return - } - if ok := git.IsBranchExist(ctx, pull.HeadRepo.RepoPath(), pull.BaseBranch); !ok { - // todo localize - ctx.JSONError("The origin branch is delete, cannot reopen.") - return - } headBranchRef := pull.GetGitHeadBranchRefName() headBranchCommitID, err := git.GetFullCommitID(ctx, pull.HeadRepo.RepoPath(), headBranchRef) if err != nil { diff --git a/tests/integration/pull_reopen_test.go b/tests/integration/pull_reopen_test.go new file mode 100644 index 0000000000..d8dfffc36a --- /dev/null +++ b/tests/integration/pull_reopen_test.go @@ -0,0 +1,214 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "code.gitea.io/gitea/models/db" + git_model "code.gitea.io/gitea/models/git" + issues_model "code.gitea.io/gitea/models/issues" + unit_model "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + gitea_context "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/translation" + issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" + files_service "code.gitea.io/gitea/services/repository/files" + "code.gitea.io/gitea/tests" + "github.com/stretchr/testify/assert" +) + +func TestPullrequestReopen(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) + + // Create an base repository. + baseRepo, _, f := CreateDeclarativeRepo(t, user2, "reopen-base", + []unit_model.Type{unit_model.TypePullRequests}, nil, nil, + ) + defer f() + + // Create a new branch on the base branch, so it can be deleted later. + _, err := files_service.ChangeRepoFiles(git.DefaultContext, baseRepo, user2, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("New README.md"), + }, + }, + Message: "Modify README for base", + OldBranch: "main", + NewBranch: "base-branch", + Author: &files_service.IdentityOptions{ + Name: user2.Name, + Email: user2.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: user2.Name, + Email: user2.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + assert.NoError(t, err) + + // Create an head repository. + headRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, org26, repo_service.ForkRepoOptions{ + BaseRepo: baseRepo, + Name: "reopen-head", + }) + assert.NoError(t, err) + assert.NotEmpty(t, headRepo) + + // Add a change to the head repository, so a pull request can be opened. + _, err = files_service.ChangeRepoFiles(git.DefaultContext, headRepo, user2, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("Updated README.md"), + }, + }, + Message: "Modify README for head", + OldBranch: "main", + NewBranch: "head-branch", + Author: &files_service.IdentityOptions{ + Name: user2.Name, + Email: user2.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: user2.Name, + Email: user2.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + assert.NoError(t, err) + + // Create the pull reuqest. + pullIssue := &issues_model.Issue{ + RepoID: baseRepo.ID, + Title: "Testing reopen functionality", + PosterID: user2.ID, + Poster: user2, + IsPull: true, + } + pullRequest := &issues_model.PullRequest{ + HeadRepoID: headRepo.ID, + BaseRepoID: baseRepo.ID, + HeadBranch: "head-branch", + BaseBranch: "base-branch", + HeadRepo: headRepo, + BaseRepo: baseRepo, + Type: issues_model.PullRequestGitea, + } + err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + assert.NoError(t, err) + + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "Testing reopen functionality"}) + + // Close the PR. + err = issue_service.ChangeStatus(db.DefaultContext, issue, user2, "", true) + assert.NoError(t, err) + + session := loginUser(t, "user2") + + reopenPR := func(t *testing.T, expectedStatus int) *httptest.ResponseRecorder { + t.Helper() + + link := fmt.Sprintf("%s/pulls/%d/comments", baseRepo.FullName(), issue.Index) + req := NewRequestWithValues(t, "POST", link, map[string]string{ + "_csrf": GetCSRF(t, session, fmt.Sprintf("%s/pulls/%d", baseRepo.FullName(), issue.Index)), + "status": "reopen", + }) + return session.MakeRequest(t, req, expectedStatus) + } + + restoreBranch := func(t *testing.T, repoName, branchName string, branchID int64) { + t.Helper() + + link := fmt.Sprintf("/%s/branches", repoName) + req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/restore?branch_id=%d&name=%s", link, branchID, branchName), map[string]string{ + "_csrf": GetCSRF(t, session, link), + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(gitea_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "success%3DBranch%2B%2522"+branchName+"%2522%2Bhas%2Bbeen%2Brestored.") + } + + deleteBranch := func(t *testing.T, repoName, branchName string) { + t.Helper() + + link := fmt.Sprintf("/%s/branches", repoName) + req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/delete?name=%s", link, branchName), map[string]string{ + "_csrf": GetCSRF(t, session, link), + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(gitea_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Contains(t, flashCookie.Value, "success%3DBranch%2B%2522"+branchName+"%2522%2Bhas%2Bbeen%2Bdeleted.") + } + + type errorJSON struct { + Error string `json:"errorMessage"` + } + + t.Run("Base branch deleted", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{Name: "base-branch", RepoID: baseRepo.ID}) + defer func() { + restoreBranch(t, baseRepo.FullName(), branch.Name, branch.ID) + }() + + deleteBranch(t, baseRepo.FullName(), branch.Name) + resp := reopenPR(t, http.StatusBadRequest) + + var errorResp errorJSON + DecodeJSON(t, resp, &errorResp) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("repo.pulls.reopen_failed.base_branch"), errorResp.Error) + }) + + t.Run("Head branch deleted", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{Name: "head-branch", RepoID: headRepo.ID}) + defer func() { + restoreBranch(t, headRepo.FullName(), branch.Name, branch.ID) + }() + + deleteBranch(t, headRepo.FullName(), branch.Name) + resp := reopenPR(t, http.StatusBadRequest) + + var errorResp errorJSON + DecodeJSON(t, resp, &errorResp) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("repo.pulls.reopen_failed.head_branch"), errorResp.Error) + }) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + reopenPR(t, http.StatusOK) + }) + }) +}