Modernize merge button (#28140)

- 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: Gusted <postmaster@gusted.xyz>

diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go
index a7e703d01c8d636d7aa90d3d0d611be96ba911a8..32d82b2a643cbfd942ac2dd764bcc8c96b111895 100644 (file)
--- a/routers/web/repo/pull.go
+++ b/routers/web/repo/pull.go
@@ -1149,30 +1149,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
        }
 
@@ -1180,18 +1178,18 @@ func MergePullRequest(ctx *context.Context) {
        if manuallyMerged {
                if err := pull_service.MergedManually(ctx, 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
        }
 
@@ -1221,15 +1219,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{
@@ -1242,7 +1239,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{
@@ -1286,7 +1283,7 @@ func MergePullRequest(ctx *context.Context) {
                                }
                                ctx.Flash.Error(flashError)
                        }
-                       ctx.Redirect(issue.Link())
+                       ctx.JSONRedirect(issue.Link())
                } else {
                        ctx.ServerError("Merge", err)
                }
@@ -1295,7 +1292,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
        }
 
@@ -1309,7 +1306,7 @@ func MergePullRequest(ctx *context.Context) {
                        return
                }
                if exist {
-                       ctx.Redirect(issue.Link())
+                       ctx.JSONRedirect(issue.Link())
                        return
                }
 
@@ -1327,7 +1324,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 d17bf4afda114ef96c1bbead973d8fdbc7d17f71..35af47f802fd62aa9ec770106e536828a2292542 100644 (file)
--- 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 54dcf9d86065d93fd375ef11dc4939c36f74751b..b0b10b6252e94c0bf29e2a32fe0a605ff6614261 100644 (file)
--- a/web_src/js/components/PullRequestMergeForm.vue
+++ b/web_src/js/components/PullRequestMergeForm.vue
@@ -94,48 +94,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 :action="mergeForm.baseLink+'/merge'" method="post">
-        <input type="hidden" name="_csrf" :value="csrfToken">
-        <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="force_merge" v-model="forceMerge">
-
-        <template v-if="!mergeStyleDetail.hideMergeMessageTexts">
-          <div class="field">
-            <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">
+    <form class="ui form form-fetch-action" v-if="showActionForm" :action="mergeForm.baseLink+'/merge'" method="post">
+      <input type="hidden" name="_csrf" :value="csrfToken">
+      <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="force_merge" v-model="forceMerge">
+
+      <template v-if="!mergeStyleDetail.hideMergeMessageTexts">
+        <div class="field">
+          <input type="text" name="merge_title_field" v-model="mergeTitleFieldValue">
         </div>
-
-        <button class="ui button" :class="mergeButtonStyleClass" type="submit" name="do" :value="mergeStyle">
-          {{ mergeStyleDetail.textDoMerge }}
-          <template v-if="autoMergeWhenSucceed">
-            {{ mergeForm.textAutoMergeButtonWhenSucceed }}
+        <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>
-        </button>
+        </div>
+      </template>
 
-        <button class="ui button merge-cancel" @click="toggleActionForm(false)">
-          {{ mergeForm.textCancel }}
-        </button>
+      <div class="field" v-if="mergeStyle === 'manually-merged'">
+        <input type="text" name="merge_commit_id" :placeholder="mergeForm.textMergeCommitId">
+      </div>
 
-        <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>
+      <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 -->