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 <postmaster@gusted.xyz>
(cherry picked from commit fbf29f29b5)

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.JSONError(ctx.Tr("repo.pulls.invalid_merge_option"))
-			ctx.Redirect(issue.Link())
 		} 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

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
 }

web_src/js/components/PullRequestMergeForm.vue

@@ -90,48 +90,46 @@ export default {
     <!-- eslint-disable-next-line vue/no-v-html -->
     <div v-if="mergeForm.hasPendingPullRequestMerge" v-html="mergeForm.hasPendingPullRequestMergeTip" class="ui info message"/>
 
-    <div class="ui form" v-if="showActionForm">
+    <form class="ui form form-fetch-action" v-if="showActionForm" :action="mergeForm.baseLink+'/merge'" method="post">
-      <form :action="mergeForm.baseLink+'/merge'" method="post">
+      <input type="hidden" name="_csrf" :value="csrfToken">
-        <input type="hidden" name="_csrf" :value="csrfToken">
+      <input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID">
-        <input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID">
+      <input type="hidden" name="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
-        <input type="hidden" name="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
+      <input type="hidden" name="force_merge" v-model="forceMerge">
-        <input type="hidden" name="force_merge" v-model="forceMerge">
 
-        <template v-if="!mergeStyleDetail.hideMergeMessageTexts">
+      <template v-if="!mergeStyleDetail.hideMergeMessageTexts">
-          <div class="field">
+        <div class="field">
-            <input type="text" name="merge_title_field" v-model="mergeTitleFieldValue">
+          <input type="text" name="merge_title_field" v-model="mergeTitleFieldValue">
-          </div>
-          <div class="field">
-            <textarea name="merge_message_field" rows="5" :placeholder="mergeForm.mergeMessageFieldPlaceHolder" v-model="mergeMessageFieldValue"/>
-            <template v-if="mergeMessageFieldValue !== mergeForm.defaultMergeMessage">
-              <button @click.prevent="clearMergeMessage" class="btn gt-mt-2 gt-p-2 interact-fg" :data-tooltip-content="mergeForm.textClearMergeMessageHint">
-                {{ mergeForm.textClearMergeMessage }}
-              </button>
-            </template>
-          </div>
-        </template>
-
-        <div class="field" v-if="mergeStyle === 'manually-merged'">
-          <input type="text" name="merge_commit_id" :placeholder="mergeForm.textMergeCommitId">
         </div>
-
+        <div class="field">
-        <button class="ui button" :class="mergeButtonStyleClass" type="submit" name="do" :value="mergeStyle">
+          <textarea name="merge_message_field" rows="5" :placeholder="mergeForm.mergeMessageFieldPlaceHolder" v-model="mergeMessageFieldValue"/>
-          {{ mergeStyleDetail.textDoMerge }}
+          <template v-if="mergeMessageFieldValue !== mergeForm.defaultMergeMessage">
-          <template v-if="autoMergeWhenSucceed">
+            <button @click.prevent="clearMergeMessage" class="btn gt-mt-2 gt-p-2 interact-fg" :data-tooltip-content="mergeForm.textClearMergeMessageHint">
-            {{ mergeForm.textAutoMergeButtonWhenSucceed }}
+              {{ mergeForm.textClearMergeMessage }}
+            </button>
           </template>
-        </button>
-
-        <button class="ui button merge-cancel" @click="toggleActionForm(false)">
-          {{ mergeForm.textCancel }}
-        </button>
-
-        <div class="ui checkbox gt-ml-2" v-if="mergeForm.isPullBranchDeletable && !autoMergeWhenSucceed">
-          <input name="delete_branch_after_merge" type="checkbox" v-model="deleteBranchAfterMerge" id="delete-branch-after-merge">
-          <label for="delete-branch-after-merge">{{ mergeForm.textDeleteBranch }}</label>
         </div>
-      </form>
+      </template>
-    </div>
+
+      <div class="field" v-if="mergeStyle === 'manually-merged'">
+        <input type="text" name="merge_commit_id" :placeholder="mergeForm.textMergeCommitId">
+      </div>
+
+      <button class="ui button" :class="mergeButtonStyleClass" type="submit" name="do" :value="mergeStyle">
+        {{ mergeStyleDetail.textDoMerge }}
+        <template v-if="autoMergeWhenSucceed">
+          {{ mergeForm.textAutoMergeButtonWhenSucceed }}
+        </template>
+      </button>
+
+      <button class="ui button merge-cancel" @click="toggleActionForm(false)">
+        {{ mergeForm.textCancel }}
+      </button>
+
+      <div class="ui checkbox gt-ml-2" v-if="mergeForm.isPullBranchDeletable && !autoMergeWhenSucceed">
+        <input name="delete_branch_after_merge" type="checkbox" v-model="deleteBranchAfterMerge" id="delete-branch-after-merge">
+        <label for="delete-branch-after-merge">{{ mergeForm.textDeleteBranch }}</label>
+      </div>
+    </form>
 
     <div v-if="!showActionForm" class="gt-df">
       <!-- the merge button -->