From 70c7ee54754e93fd2612287b1ccfc24006924651 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Mon, 15 Jan 2024 09:40:52 +0800 Subject: [PATCH] Modernize merge button (#28140) (#28786) Backport #28140 by @earl-warren - Make use of the `form-fetch-action` for the merge button, which will automatically prevent the action from happening multiple times and show a nice loading indicator as user feedback while the merge request is being processed by the server. - Adjust the merge PR code to JSON response as this is required for the `form-fetch-action` functionality. - Resolves https://codeberg.org/forgejo/forgejo/issues/774 - Likely resolves the cause of https://codeberg.org/forgejo/forgejo/issues/1688#issuecomment-1313044 (cherry picked from commit 4ec64c19507caefff7ddaad722b1b5792b97cc5a) Co-authored-by: Earl Warren <109468362+earl-warren@users.noreply.github.com> Co-authored-by: Gusted (cherry picked from commit fbf29f29b5225be8e5e682e45b6977e7dda9b318) --- routers/web/repo/pull.go | 45 ++++++----- tests/integration/pull_merge_test.go | 9 ++- .../js/components/PullRequestMergeForm.vue | 74 +++++++++---------- 3 files changed, 65 insertions(+), 63 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 67416927ac..6b00ec12d8 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1133,30 +1133,28 @@ func MergePullRequest(ctx *context.Context) { switch { case errors.Is(err, pull_service.ErrIsClosed): if issue.IsPull { - ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed")) + ctx.JSONError(ctx.Tr("repo.pulls.is_closed")) } else { - ctx.Flash.Error(ctx.Tr("repo.issues.closed_title")) + ctx.JSONError(ctx.Tr("repo.issues.closed_title")) } case errors.Is(err, pull_service.ErrUserNotAllowedToMerge): - ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed")) + ctx.JSONError(ctx.Tr("repo.pulls.update_not_allowed")) case errors.Is(err, pull_service.ErrHasMerged): - ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged")) + ctx.JSONError(ctx.Tr("repo.pulls.has_merged")) case errors.Is(err, pull_service.ErrIsWorkInProgress): - ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip")) + ctx.JSONError(ctx.Tr("repo.pulls.no_merge_wip")) case errors.Is(err, pull_service.ErrNotMergableState): - ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) + ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready")) case models.IsErrDisallowedToMerge(err): - ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) + ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready")) case asymkey_service.IsErrWontSign(err): - ctx.Flash.Error(err.Error()) // has no translation ... + ctx.JSONError(err.Error()) // has no translation ... case errors.Is(err, pull_service.ErrDependenciesLeft): - ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked")) + ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked")) default: ctx.ServerError("WebCheck", err) - return } - ctx.Redirect(issue.Link()) return } @@ -1164,18 +1162,18 @@ func MergePullRequest(ctx *context.Context) { if manuallyMerged { if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { switch { - case models.IsErrInvalidMergeStyle(err): - ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) + ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option")) case strings.Contains(err.Error(), "Wrong commit ID"): - ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id")) + ctx.JSONError(ctx.Tr("repo.pulls.wrong_commit_id")) default: ctx.ServerError("MergedManually", err) - return } + + return } - ctx.Redirect(issue.Link()) + ctx.JSONRedirect(issue.Link()) return } @@ -1205,15 +1203,14 @@ func MergePullRequest(ctx *context.Context) { } else if scheduled { // nothing more to do ... ctx.Flash.Success(ctx.Tr("repo.pulls.auto_merge_newly_scheduled")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pr.Index)) + ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pr.Index)) return } } if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if models.IsErrInvalidMergeStyle(err) { - ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) - ctx.Redirect(issue.Link()) + ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option")) } else if models.IsErrMergeConflicts(err) { conflictError := err.(models.ErrMergeConflicts) flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{ @@ -1226,7 +1223,7 @@ func MergePullRequest(ctx *context.Context) { return } ctx.Flash.Error(flashError) - ctx.Redirect(issue.Link()) + ctx.JSONRedirect(issue.Link()) } else if models.IsErrRebaseConflicts(err) { conflictError := err.(models.ErrRebaseConflicts) flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{ @@ -1270,7 +1267,7 @@ func MergePullRequest(ctx *context.Context) { } ctx.Flash.Error(flashError) } - ctx.Redirect(issue.Link()) + ctx.JSONRedirect(issue.Link()) } else { ctx.ServerError("Merge", err) } @@ -1279,7 +1276,7 @@ func MergePullRequest(ctx *context.Context) { log.Trace("Pull request merged: %d", pr.ID) if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { - ctx.ServerError("CreateOrStopIssueStopwatch", err) + ctx.ServerError("stopTimerIfAvailable", err) return } @@ -1293,7 +1290,7 @@ func MergePullRequest(ctx *context.Context) { return } if exist { - ctx.Redirect(issue.Link()) + ctx.JSONRedirect(issue.Link()) return } @@ -1311,7 +1308,7 @@ func MergePullRequest(ctx *context.Context) { deleteBranch(ctx, pr, headRepo) } - ctx.Redirect(issue.Link()) + ctx.JSONRedirect(issue.Link()) } // CancelAutoMergePullRequest cancels a scheduled pr diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index adb0e982c0..42bf584f58 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -45,7 +45,14 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin "_csrf": htmlDoc.GetCSRF(), "do": string(mergeStyle), }) - resp = session.MakeRequest(t, req, http.StatusSeeOther) + resp = session.MakeRequest(t, req, http.StatusOK) + + respJSON := struct { + Redirect string + }{} + DecodeJSON(t, resp, &respJSON) + + assert.EqualValues(t, fmt.Sprintf("/%s/%s/pulls/%s", user, repo, pullnum), respJSON.Redirect) return resp } diff --git a/web_src/js/components/PullRequestMergeForm.vue b/web_src/js/components/PullRequestMergeForm.vue index 03d1f99683..aef4c8b5ac 100644 --- a/web_src/js/components/PullRequestMergeForm.vue +++ b/web_src/js/components/PullRequestMergeForm.vue @@ -90,48 +90,46 @@ export default {
-
-
- - - - + + + + + -