Skip to content

Commit 3f342d6

Browse files
earl-warrenGusted
earl-warren
and
Gusted
authored
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 <[email protected]>
1 parent 5a7bacb commit 3f342d6

File tree

3 files changed

+65
-63
lines changed

3 files changed

+65
-63
lines changed

‎routers/web/repo/pull.go‎

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1149,49 +1149,47 @@ func MergePullRequest(ctx *context.Context) {
11491149
switch {
11501150
case errors.Is(err, pull_service.ErrIsClosed):
11511151
if issue.IsPull {
1152-
ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed"))
1152+
ctx.JSONError(ctx.Tr("repo.pulls.is_closed"))
11531153
} else {
1154-
ctx.Flash.Error(ctx.Tr("repo.issues.closed_title"))
1154+
ctx.JSONError(ctx.Tr("repo.issues.closed_title"))
11551155
}
11561156
case errors.Is(err, pull_service.ErrUserNotAllowedToMerge):
1157-
ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed"))
1157+
ctx.JSONError(ctx.Tr("repo.pulls.update_not_allowed"))
11581158
case errors.Is(err, pull_service.ErrHasMerged):
1159-
ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged"))
1159+
ctx.JSONError(ctx.Tr("repo.pulls.has_merged"))
11601160
case errors.Is(err, pull_service.ErrIsWorkInProgress):
1161-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip"))
1161+
ctx.JSONError(ctx.Tr("repo.pulls.no_merge_wip"))
11621162
case errors.Is(err, pull_service.ErrNotMergableState):
1163-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
1163+
ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready"))
11641164
case models.IsErrDisallowedToMerge(err):
1165-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
1165+
ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready"))
11661166
case asymkey_service.IsErrWontSign(err):
1167-
ctx.Flash.Error(err.Error()) // has no translation ...
1167+
ctx.JSONError(err.Error()) // has no translation ...
11681168
case errors.Is(err, pull_service.ErrDependenciesLeft):
1169-
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
1169+
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
11701170
default:
11711171
ctx.ServerError("WebCheck", err)
1172-
return
11731172
}
11741173

1175-
ctx.Redirect(issue.Link())
11761174
return
11771175
}
11781176

11791177
// handle manually-merged mark
11801178
if manuallyMerged {
11811179
if err := pull_service.MergedManually(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
11821180
switch {
1183-
11841181
case models.IsErrInvalidMergeStyle(err):
1185-
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
1182+
ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option"))
11861183
case strings.Contains(err.Error(), "Wrong commit ID"):
1187-
ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id"))
1184+
ctx.JSONError(ctx.Tr("repo.pulls.wrong_commit_id"))
11881185
default:
11891186
ctx.ServerError("MergedManually", err)
1190-
return
11911187
}
1188+
1189+
return
11921190
}
11931191

1194-
ctx.Redirect(issue.Link())
1192+
ctx.JSONRedirect(issue.Link())
11951193
return
11961194
}
11971195

@@ -1221,15 +1219,14 @@ func MergePullRequest(ctx *context.Context) {
12211219
} else if scheduled {
12221220
// nothing more to do ...
12231221
ctx.Flash.Success(ctx.Tr("repo.pulls.auto_merge_newly_scheduled"))
1224-
ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pr.Index))
1222+
ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pr.Index))
12251223
return
12261224
}
12271225
}
12281226

12291227
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil {
12301228
if models.IsErrInvalidMergeStyle(err) {
1231-
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
1232-
ctx.Redirect(issue.Link())
1229+
ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option"))
12331230
} else if models.IsErrMergeConflicts(err) {
12341231
conflictError := err.(models.ErrMergeConflicts)
12351232
flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{
@@ -1242,7 +1239,7 @@ func MergePullRequest(ctx *context.Context) {
12421239
return
12431240
}
12441241
ctx.Flash.Error(flashError)
1245-
ctx.Redirect(issue.Link())
1242+
ctx.JSONRedirect(issue.Link())
12461243
} else if models.IsErrRebaseConflicts(err) {
12471244
conflictError := err.(models.ErrRebaseConflicts)
12481245
flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{
@@ -1286,7 +1283,7 @@ func MergePullRequest(ctx *context.Context) {
12861283
}
12871284
ctx.Flash.Error(flashError)
12881285
}
1289-
ctx.Redirect(issue.Link())
1286+
ctx.JSONRedirect(issue.Link())
12901287
} else {
12911288
ctx.ServerError("Merge", err)
12921289
}
@@ -1295,7 +1292,7 @@ func MergePullRequest(ctx *context.Context) {
12951292
log.Trace("Pull request merged: %d", pr.ID)
12961293

12971294
if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
1298-
ctx.ServerError("CreateOrStopIssueStopwatch", err)
1295+
ctx.ServerError("stopTimerIfAvailable", err)
12991296
return
13001297
}
13011298

@@ -1309,7 +1306,7 @@ func MergePullRequest(ctx *context.Context) {
13091306
return
13101307
}
13111308
if exist {
1312-
ctx.Redirect(issue.Link())
1309+
ctx.JSONRedirect(issue.Link())
13131310
return
13141311
}
13151312

@@ -1327,7 +1324,7 @@ func MergePullRequest(ctx *context.Context) {
13271324
deleteBranch(ctx, pr, headRepo)
13281325
}
13291326

1330-
ctx.Redirect(issue.Link())
1327+
ctx.JSONRedirect(issue.Link())
13311328
}
13321329

13331330
// CancelAutoMergePullRequest cancels a scheduled pr

‎tests/integration/pull_merge_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,14 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
4545
"_csrf": htmlDoc.GetCSRF(),
4646
"do": string(mergeStyle),
4747
})
48-
resp = session.MakeRequest(t, req, http.StatusSeeOther)
48+
resp = session.MakeRequest(t, req, http.StatusOK)
49+
50+
respJSON := struct {
51+
Redirect string
52+
}{}
53+
DecodeJSON(t, resp, &respJSON)
54+
55+
assert.EqualValues(t, fmt.Sprintf("/%s/%s/pulls/%s", user, repo, pullnum), respJSON.Redirect)
4956

5057
return resp
5158
}

‎web_src/js/components/PullRequestMergeForm.vue‎

Lines changed: 36 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -94,48 +94,46 @@ export default {
9494
<!-- eslint-disable-next-line vue/no-v-html -->
9595
<div v-if="mergeForm.hasPendingPullRequestMerge" v-html="mergeForm.hasPendingPullRequestMergeTip" class="ui info message"/>
9696
97-
<div class="ui form" v-if="showActionForm">
98-
<form :action="mergeForm.baseLink+'/merge'" method="post">
99-
<input type="hidden" name="_csrf" :value="csrfToken">
100-
<input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID">
101-
<input type="hidden" name="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
102-
<input type="hidden" name="force_merge" v-model="forceMerge">
103-
104-
<template v-if="!mergeStyleDetail.hideMergeMessageTexts">
105-
<div class="field">
106-
<input type="text" name="merge_title_field" v-model="mergeTitleFieldValue">
107-
</div>
108-
<div class="field">
109-
<textarea name="merge_message_field" rows="5" :placeholder="mergeForm.mergeMessageFieldPlaceHolder" v-model="mergeMessageFieldValue"/>
110-
<template v-if="mergeMessageFieldValue !== mergeForm.defaultMergeMessage">
111-
<button @click.prevent="clearMergeMessage" class="btn gt-mt-2 gt-p-2 interact-fg" :data-tooltip-content="mergeForm.textClearMergeMessageHint">
112-
{{ mergeForm.textClearMergeMessage }}
113-
</button>
114-
</template>
115-
</div>
116-
</template>
117-
118-
<div class="field" v-if="mergeStyle === 'manually-merged'">
119-
<input type="text" name="merge_commit_id" :placeholder="mergeForm.textMergeCommitId">
97+
<form class="ui form form-fetch-action" v-if="showActionForm" :action="mergeForm.baseLink+'/merge'" method="post">
98+
<input type="hidden" name="_csrf" :value="csrfToken">
99+
<input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID">
100+
<input type="hidden" name="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
101+
<input type="hidden" name="force_merge" v-model="forceMerge">
102+
103+
<template v-if="!mergeStyleDetail.hideMergeMessageTexts">
104+
<div class="field">
105+
<input type="text" name="merge_title_field" v-model="mergeTitleFieldValue">
120106
</div>
121-
122-
<button class="ui button" :class="mergeButtonStyleClass" type="submit" name="do" :value="mergeStyle">
123-
{{ mergeStyleDetail.textDoMerge }}
124-
<template v-if="autoMergeWhenSucceed">
125-
{{ mergeForm.textAutoMergeButtonWhenSucceed }}
107+
<div class="field">
108+
<textarea name="merge_message_field" rows="5" :placeholder="mergeForm.mergeMessageFieldPlaceHolder" v-model="mergeMessageFieldValue"/>
109+
<template v-if="mergeMessageFieldValue !== mergeForm.defaultMergeMessage">
110+
<button @click.prevent="clearMergeMessage" class="btn gt-mt-2 gt-p-2 interact-fg" :data-tooltip-content="mergeForm.textClearMergeMessageHint">
111+
{{ mergeForm.textClearMergeMessage }}
112+
</button>
126113
</template>
127-
</button>
114+
</div>
115+
</template>
128116
129-
<button class="ui button merge-cancel" @click="toggleActionForm(false)">
130-
{{ mergeForm.textCancel }}
131-
</button>
117+
<div class="field" v-if="mergeStyle === 'manually-merged'">
118+
<input type="text" name="merge_commit_id" :placeholder="mergeForm.textMergeCommitId">
119+
</div>
132120
133-
<div class="ui checkbox gt-ml-2" v-if="mergeForm.isPullBranchDeletable && !autoMergeWhenSucceed">
134-
<input name="delete_branch_after_merge" type="checkbox" v-model="deleteBranchAfterMerge" id="delete-branch-after-merge">
135-
<label for="delete-branch-after-merge">{{ mergeForm.textDeleteBranch }}</label>
136-
</div>
137-
</form>
138-
</div>
121+
<button class="ui button" :class="mergeButtonStyleClass" type="submit" name="do" :value="mergeStyle">
122+
{{ mergeStyleDetail.textDoMerge }}
123+
<template v-if="autoMergeWhenSucceed">
124+
{{ mergeForm.textAutoMergeButtonWhenSucceed }}
125+
</template>
126+
</button>
127+
128+
<button class="ui button merge-cancel" @click="toggleActionForm(false)">
129+
{{ mergeForm.textCancel }}
130+
</button>
131+
132+
<div class="ui checkbox gt-ml-2" v-if="mergeForm.isPullBranchDeletable && !autoMergeWhenSucceed">
133+
<input name="delete_branch_after_merge" type="checkbox" v-model="deleteBranchAfterMerge" id="delete-branch-after-merge">
134+
<label for="delete-branch-after-merge">{{ mergeForm.textDeleteBranch }}</label>
135+
</div>
136+
</form>
139137
140138
<div v-if="!showActionForm" class="gt-df">
141139
<!-- the merge button -->

0 commit comments

Comments
 (0)