From b4e4b7ad51e4c52a42eb114c9a70b4383f1871ef Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 19 Jun 2023 16:25:36 +0800 Subject: [PATCH] Make backend code respond correct JSON when creating PR (#25353) Fix #25351 --- routers/web/repo/pull.go | 57 ++++++++------------------- tests/integration/pull_create_test.go | 10 ++--- tests/integration/pull_merge_test.go | 2 +- tests/integration/pull_status_test.go | 6 +-- 4 files changed, 25 insertions(+), 50 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 09dbc23eac..309e61cf6e 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -37,7 +37,6 @@ import ( "code.gitea.io/gitea/modules/upload" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" - "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/routers/utils" asymkey_service "code.gitea.io/gitea/services/asymkey" "code.gitea.io/gitea/services/automerge" @@ -1206,36 +1205,12 @@ func CompareAndPullRequestPost(ctx *context.Context) { } if ctx.HasError() { - middleware.AssignForm(form, ctx.Data) - - // This stage is already stop creating new pull request, so it does not matter if it has - // something to compare or not. - PrepareCompareDiff(ctx, ci, - gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string))) - if ctx.Written() { - return - } - - if len(form.Title) > 255 { - var trailer string - form.Title, trailer = util.SplitStringAtByteN(form.Title, 255) - - form.Content = trailer + "\n\n" + form.Content - } - middleware.AssignForm(form, ctx.Data) - - ctx.HTML(http.StatusOK, tplCompareDiff) + ctx.JSONError(ctx.GetErrMsg()) return } if util.IsEmptyString(form.Title) { - PrepareCompareDiff(ctx, ci, - gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string))) - if ctx.Written() { - return - } - - ctx.RenderWithErr(ctx.Tr("repo.issues.new.title_empty"), tplCompareDiff, form) + ctx.JSONError(ctx.Tr("repo.issues.new.title_empty")) return } @@ -1278,20 +1253,20 @@ func CompareAndPullRequestPost(ctx *context.Context) { pushrejErr := err.(*git.ErrPushRejected) message := pushrejErr.Message if len(message) == 0 { - ctx.Flash.Error(ctx.Tr("repo.pulls.push_rejected_no_message")) - } else { - flashError, err := ctx.RenderToString(tplAlertDetails, map[string]interface{}{ - "Message": ctx.Tr("repo.pulls.push_rejected"), - "Summary": ctx.Tr("repo.pulls.push_rejected_summary"), - "Details": utils.SanitizeFlashErrorString(pushrejErr.Message), - }) - if err != nil { - ctx.ServerError("CompareAndPullRequest.HTMLString", err) - return - } - ctx.Flash.Error(flashError) + ctx.JSONError(ctx.Tr("repo.pulls.push_rejected_no_message")) + return } - ctx.Redirect(pullIssue.Link()) + flashError, err := ctx.RenderToString(tplAlertDetails, map[string]interface{}{ + "Message": ctx.Tr("repo.pulls.push_rejected"), + "Summary": ctx.Tr("repo.pulls.push_rejected_summary"), + "Details": utils.SanitizeFlashErrorString(pushrejErr.Message), + }) + if err != nil { + ctx.ServerError("CompareAndPullRequest.HTMLString", err) + return + } + ctx.Flash.Error(flashError) + ctx.JSONRedirect(pullIssue.Link()) // FIXME: it's unfriendly, and will make the content lost return } ctx.ServerError("NewPullRequest", err) @@ -1299,7 +1274,7 @@ func CompareAndPullRequestPost(ctx *context.Context) { } log.Trace("Pull request created: %d/%d", repo.ID, pullIssue.ID) - ctx.Redirect(pullIssue.Link()) + ctx.JSONRedirect(pullIssue.Link()) } // CleanUpPullRequest responses for delete merged branch when PR has been merged diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index 6e2d65ca0a..a6ee0d9dfa 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -11,6 +11,7 @@ import ( "strings" "testing" + "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -39,8 +40,7 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo, branch, titl "_csrf": htmlDoc.GetCSRF(), "title": title, }) - resp = session.MakeRequest(t, req, http.StatusSeeOther) - + resp = session.MakeRequest(t, req, http.StatusOK) return resp } @@ -52,7 +52,7 @@ func TestPullCreate(t *testing.T) { resp := testPullCreate(t, session, "user1", "repo1", "master", "This is a pull title") // check the redirected URL - url := resp.Header().Get("Location") + url := test.RedirectURL(resp) assert.Regexp(t, "^/user2/repo1/pulls/[0-9]*$", url) // check .diff can be accessed and matches performed change @@ -80,7 +80,7 @@ func TestPullCreate_TitleEscape(t *testing.T) { resp := testPullCreate(t, session, "user1", "repo1", "master", "XSS PR") // check the redirected URL - url := resp.Header().Get("Location") + url := test.RedirectURL(resp) assert.Regexp(t, "^/user2/repo1/pulls/[0-9]*$", url) // Edit title @@ -145,7 +145,7 @@ func TestPullBranchDelete(t *testing.T) { resp := testPullCreate(t, session, "user1", "repo1", "master1", "This is a pull title") // check the redirected URL - url := resp.Header().Get("Location") + url := test.RedirectURL(resp) assert.Regexp(t, "^/user2/repo1/pulls/[0-9]*$", url) req := NewRequest(t, "GET", url) session.MakeRequest(t, req, http.StatusOK) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 8890347c36..7b7c8864c7 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -199,7 +199,7 @@ func TestCantMergeWorkInProgress(t *testing.T) { resp := testPullCreate(t, session, "user1", "repo1", "master", "[wip] This is a pull title") - req := NewRequest(t, "GET", resp.Header().Get("Location")) + req := NewRequest(t, "GET", test.RedirectURL(resp)) resp = session.MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) text := strings.TrimSpace(htmlDoc.doc.Find(".merge-section > .item").Last().Text()) diff --git a/tests/integration/pull_status_test.go b/tests/integration/pull_status_test.go index 0bdb80ecbf..6a6fd2e859 100644 --- a/tests/integration/pull_status_test.go +++ b/tests/integration/pull_status_test.go @@ -30,7 +30,7 @@ func TestPullCreate_CommitStatus(t *testing.T) { "title": "pull request from status1", }, ) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) req = NewRequest(t, "GET", "/user1/repo1/pulls") resp := session.MakeRequest(t, req, http.StatusOK) @@ -127,7 +127,7 @@ func TestPullCreate_EmptyChangesWithDifferentCommits(t *testing.T) { "title": "pull request from status1", }, ) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) req = NewRequest(t, "GET", "/user1/repo1/pulls/1") resp := session.MakeRequest(t, req, http.StatusOK) @@ -150,7 +150,7 @@ func TestPullCreate_EmptyChangesWithSameCommits(t *testing.T) { "title": "pull request from status1", }, ) - session.MakeRequest(t, req, http.StatusSeeOther) + session.MakeRequest(t, req, http.StatusOK) req = NewRequest(t, "GET", "/user1/repo1/pulls/1") resp := session.MakeRequest(t, req, http.StatusOK) doc := NewHTMLParser(t, resp.Body)