diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index d0064e763ef82..59e85dd8123a7 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -10,6 +10,7 @@ import ( "net/url" "sort" "strconv" + "strings" activities_model "code.gitea.io/gitea/models/activities" "code.gitea.io/gitea/models/db" @@ -387,8 +388,10 @@ func ViewIssue(ctx *context.Context) { prepareIssueViewSidebarTimeTracker, prepareIssueViewSidebarDependency, prepareIssueViewSidebarPin, - func(ctx *context.Context, issue *issues_model.Issue) { preparePullViewPullInfo(ctx, issue) }, - preparePullViewReviewAndMerge, + func(ctx *context.Context, issue *issues_model.Issue) { + compareInfo := preparePullViewPullInfo(ctx, issue) + preparePullViewReviewAndMerge(ctx, issue, compareInfo) + }, } for _, prepareFunc := range prepareFuncs { @@ -440,8 +443,8 @@ func ViewPullMergeBox(ctx *context.Context) { ctx.NotFound(nil) return } - preparePullViewPullInfo(ctx, issue) - preparePullViewReviewAndMerge(ctx, issue) + compareInfo := preparePullViewPullInfo(ctx, issue) + preparePullViewReviewAndMerge(ctx, issue, compareInfo) ctx.Data["PullMergeBoxReloading"] = issue.PullRequest.IsChecking() // TODO: it should use a dedicated struct to render the pull merge box, to make sure all data is prepared correctly @@ -822,7 +825,7 @@ func prepareIssueViewCommentsAndSidebarParticipants(ctx *context.Context, issue ctx.Data["NumParticipants"] = len(participants) } -func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Issue) { +func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Issue, compareInfo *pull_service.CompareInfo) { getBranchData(ctx, issue) if !issue.IsPull { return @@ -933,8 +936,22 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss ctx.ServerError("GetDefaultSquashMergeMessage", err) return } + _, coAuthors := pull_service.GetSquashMergeCommitMessages(ctx, pull) + + defaultSquashMergeBody = fmt.Sprintf("%s%s", defaultSquashMergeBody, coAuthors) + + commitsBuilder := strings.Builder{} + for _, c := range compareInfo.Commits { + commitsBuilder.WriteString("* ") + commitsBuilder.WriteString(c.CommitMessage) + commitsBuilder.WriteRune('\n') + } + ctx.Data["DefaultSquashMergeMessage"] = defaultSquashMergeMessage - ctx.Data["DefaultSquashMergeBody"] = defaultSquashMergeBody + ctx.Data["DefaultSquashMergeBody"] = fmt.Sprintf("--------------------\n%s%s", commitsBuilder.String(), defaultSquashMergeBody) + if len(pull.Issue.Content) == 0 { + ctx.Data["DefaultSquashMergeBody"] = fmt.Sprintf("%s%s", commitsBuilder.String(), defaultSquashMergeBody) + } pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) if err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 83aaf75363fbd..24244103ad764 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -430,7 +430,7 @@ func prepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *pull_ ctx.ServerError("IsUserAllowedToUpdate", err) return nil } - ctx.Data["GetCommitMessages"] = pull_service.GetSquashMergeCommitMessages(ctx, pull) + ctx.Data["GetCommitMessages"], _ = pull_service.GetSquashMergeCommitMessages(ctx, pull) } else { ctx.Data["GetCommitMessages"] = "" } diff --git a/services/pull/pull.go b/services/pull/pull.go index a369e64dbe08f..89c719d9c6f64 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -784,15 +784,15 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re var commitMessageTrailersPattern = regexp.MustCompile(`(?:^|\n\n)(?:[\w-]+[ \t]*:[^\n]+\n*(?:[ \t]+[^\n]+\n*)*)+$`) // GetSquashMergeCommitMessages returns the commit messages between head and merge base (if there is one) -func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequest) string { +func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequest) (string, string) { if err := pr.LoadIssue(ctx); err != nil { log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err) - return "" + return "", "" } if err := pr.Issue.LoadPoster(ctx); err != nil { log.Error("Cannot load poster %d for pr id %d, index %d Error: %v", pr.Issue.PosterID, pr.ID, pr.Index, err) - return "" + return "", "" } if pr.HeadRepo == nil { @@ -800,14 +800,14 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ pr.HeadRepo, err = repo_model.GetRepositoryByID(ctx, pr.HeadRepoID) if err != nil { log.Error("GetRepositoryByIdCtx[%d]: %v", pr.HeadRepoID, err) - return "" + return "", "" } } gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pr.HeadRepo) if err != nil { log.Error("Unable to open head repository: Error: %v", err) - return "" + return "", "" } defer closer.Close() @@ -818,19 +818,19 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ pr.HeadCommitID, err = gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err) - return "" + return "", "" } headCommit, err = gitRepo.GetCommit(pr.HeadCommitID) } if err != nil { log.Error("Unable to get head commit: %s Error: %v", pr.HeadBranch, err) - return "" + return "", "" } mergeBase, err := gitRepo.GetCommit(pr.MergeBase) if err != nil { log.Error("Unable to get merge base commit: %s Error: %v", pr.MergeBase, err) - return "" + return "", "" } limit := setting.Repository.PullRequest.DefaultMergeMessageCommitsLimit @@ -838,7 +838,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ commits, err := gitRepo.CommitsBetweenLimit(headCommit, mergeBase, limit, 0) if err != nil { log.Error("Unable to get commits between: %s %s Error: %v", pr.HeadBranch, pr.MergeBase, err) - return "" + return "", "" } posterSig := pr.Issue.Poster.NewGitSig().String() @@ -846,6 +846,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ uniqueAuthors := make(container.Set[string]) authors := make([]string, 0, len(commits)) stringBuilder := strings.Builder{} + coAuthorStringBuilder := strings.Builder{} if !setting.Repository.PullRequest.PopulateSquashCommentWithCommitMessages { message := strings.TrimSpace(pr.Issue.Content) @@ -879,12 +880,12 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } if _, err := stringBuilder.Write(toWrite); err != nil { log.Error("Unable to write commit message Error: %v", err) - return "" + return "", "" } if _, err := stringBuilder.WriteRune('\n'); err != nil { log.Error("Unable to write commit message Error: %v", err) - return "" + return "", "" } } } @@ -908,7 +909,7 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ commits, err := gitRepo.CommitsBetweenLimit(headCommit, mergeBase, limit, skip) if err != nil { log.Error("Unable to get commits between: %s %s Error: %v", pr.HeadBranch, pr.MergeBase, err) - return "" + return "", "" } if len(commits) == 0 { break @@ -927,21 +928,21 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ } for _, author := range authors { - if _, err := stringBuilder.WriteString("Co-authored-by: "); err != nil { + if _, err := coAuthorStringBuilder.WriteString("Co-authored-by: "); err != nil { log.Error("Unable to write to string builder Error: %v", err) - return "" + return "", "" } - if _, err := stringBuilder.WriteString(author); err != nil { + if _, err := coAuthorStringBuilder.WriteString(author); err != nil { log.Error("Unable to write to string builder Error: %v", err) - return "" + return "", "" } - if _, err := stringBuilder.WriteRune('\n'); err != nil { + if _, err := coAuthorStringBuilder.WriteRune('\n'); err != nil { log.Error("Unable to write to string builder Error: %v", err) - return "" + return "", "" } } - return stringBuilder.String() + return stringBuilder.String(), coAuthorStringBuilder.String() } // GetIssuesAllCommitStatus returns a map of issue ID to a list of all statuses for the most recent commit as well as a map of issue ID to only the commit's latest status diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index aeff5e2b4e076..9224d9897183b 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -20,7 +20,7 @@ import ( "github.com/stretchr/testify/assert" ) -func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSelf bool, targetBranch, sourceBranch, title string) *httptest.ResponseRecorder { +func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSelf bool, targetBranch, sourceBranch, title string, contents ...string) *httptest.ResponseRecorder { req := NewRequest(t, "GET", path.Join(user, repo)) resp := session.MakeRequest(t, req, http.StatusOK) @@ -52,9 +52,15 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel htmlDoc = NewHTMLParser(t, resp.Body) link, exists = htmlDoc.doc.Find("form.ui.form").Attr("action") assert.True(t, exists, "The template has changed") + + content := "" + if len(contents) > 0 { + content = contents[0] + } req = NewRequestWithValues(t, "POST", link, map[string]string{ - "_csrf": htmlDoc.GetCSRF(), - "title": title, + "_csrf": htmlDoc.GetCSRF(), + "title": title, + "content": content, }) resp = session.MakeRequest(t, req, http.StatusOK) return resp diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 56395a5ba245b..c74155a661e89 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -12,6 +12,7 @@ import ( "os" "path" "path/filepath" + "regexp" "strconv" "strings" "testing" @@ -40,19 +41,27 @@ import ( commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" files_service "code.gitea.io/gitea/services/repository/files" + "github.com/PuerkitoBio/goquery" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool) *httptest.ResponseRecorder { +func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum string, mergeStyle repo_model.MergeStyle, deleteBranch bool, messages ...string) *httptest.ResponseRecorder { req := NewRequest(t, "GET", path.Join(user, repo, "pulls", pullnum)) resp := session.MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) link := path.Join(user, repo, "pulls", pullnum, "merge") + message := "" + if len(messages) > 0 { + message = messages[0] + } + options := map[string]string{ - "_csrf": htmlDoc.GetCSRF(), - "do": string(mergeStyle), + "_csrf": htmlDoc.GetCSRF(), + "do": string(mergeStyle), + "merge_message_field": message, } if deleteBranch { @@ -165,11 +174,116 @@ func TestPullSquash(t *testing.T) { testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited!)\n") - resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title") - - elem := strings.Split(test.RedirectURL(resp), "/") + resp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "This is a pull title", "This is a pull content") + prURL := test.RedirectURL(resp) + elem := strings.Split(prURL, "/") assert.Equal(t, "pulls", elem[3]) - testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false) + + resp = session.MakeRequest(t, NewRequest(t, "GET", prURL), http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + dataURL, exists := htmlDoc.doc.Find("#allow-edits-from-maintainers").Attr("data-url") + assert.True(t, exists) + req := NewRequestWithValues(t, "POST", dataURL+"/set_allow_maintainer_edit", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "allow_maintainer_edit": "true", + }) + session.MakeRequest(t, req, http.StatusOK) + + user2Session := loginUser(t, "user2") + resp = user2Session.MakeRequest(t, NewRequest(t, "GET", prURL+"/files"), http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + nodes := htmlDoc.doc.Find(".diff-file-box[data-new-filename=\"README.md\"] .diff-file-header-actions .tippy-target a") + if assert.Equal(t, 2, nodes.Length()) { + assert.Equal(t, "Edit File", nodes.Last().Text()) + editFileLink, exists := nodes.Last().Attr("href") + if assert.True(t, exists) { + // edit the file + resp := user2Session.MakeRequest(t, NewRequest(t, "GET", editFileLink), http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + lastCommit := htmlDoc.GetInputValueByName("last_commit") + assert.NotEmpty(t, lastCommit) + req := NewRequestWithValues(t, "POST", editFileLink, map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "last_commit": lastCommit, + "tree_path": "README.md", + "content": "Hello, World (Edite!!)", + "commit_summary": "user2 updated the file", + "commit_choice": "direct", + }) + resp = user2Session.MakeRequest(t, req, http.StatusOK) + assert.NotEmpty(t, test.RedirectURL(resp)) + } + } + resp = user2Session.MakeRequest(t, NewRequest(t, "GET", prURL+"/files"), http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + nodes = htmlDoc.doc.Find(".diff-file-box[data-new-filename=\"README.md\"] .diff-file-header-actions .tippy-target a") + if assert.Equal(t, 2, nodes.Length()) { + assert.Equal(t, "Edit File", nodes.Last().Text()) + editFileLink, exists := nodes.Last().Attr("href") + if assert.True(t, exists) { + // edit the file + resp := user2Session.MakeRequest(t, NewRequest(t, "GET", editFileLink), http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + lastCommit := htmlDoc.GetInputValueByName("last_commit") + assert.NotEmpty(t, lastCommit) + req := NewRequestWithValues(t, "POST", editFileLink, map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "last_commit": lastCommit, + "tree_path": "README.md", + "content": "Hello, World (Edite!!!)", + "commit_summary": "user2 updated the file!", + "commit_choice": "direct", + }) + resp = user2Session.MakeRequest(t, req, http.StatusOK) + assert.NotEmpty(t, test.RedirectURL(resp)) + } + } + + resp = session.MakeRequest(t, NewRequest(t, "GET", prURL+"/merge_box"), http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + message := "" + htmlDoc.doc.Find("script").Each(func(i int, s *goquery.Selection) { + scriptContent := s.Text() + re := regexp.MustCompile(`const\s+defaultSquashMergeMessage\s*=\s*"(.*?)"\s*;`) + matches := re.FindStringSubmatch(scriptContent) + if len(matches) > 1 { + message = matches[1] + } + }) + + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleSquash, false, message) + + req = NewRequest(t, "GET", "/user2/repo1/src/branch/master/") + resp = user2Session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + commitHref := htmlDoc.doc.Find(".latest-commit .commit-id-short").AttrOr("href", "") + assert.NotEmpty(t, commitHref) + + req = NewRequest(t, "GET", commitHref) + resp = user2Session.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + prTitle, exists := htmlDoc.doc.Find(".commit-summary").Attr("title") + assert.True(t, exists) + assert.Contains(t, prTitle, "This is a pull title") + + commitBody := htmlDoc.doc.Find(".commit-body").Text() + assert.NotEmpty(t, commitBody) + assert.Contains(t, commitBody, "--------------------") + + req = NewRequest(t, "GET", fmt.Sprintf("/user2/repo1/pulls/%s/commits/list", elem[4])) + resp = user2Session.MakeRequest(t, req, http.StatusOK) + + var pullCommitList struct { + Commits []pull_service.CommitInfo `json:"commits"` + LastReviewCommitSha string `json:"last_review_commit_sha"` + } + DecodeJSON(t, resp, &pullCommitList) + + require.Len(t, pullCommitList.Commits, 4) + + for _, commit := range pullCommitList.Commits { + assert.Contains(t, commitBody, "* "+commit.Summary) + } hookTasks, err = webhook.HookTasks(t.Context(), 1, 1) assert.NoError(t, err)