Skip to content

Commit fbf29f2

Browse files
GiteaBotearl-warrenGusted
authored
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>
1 parent 6e29242 commit fbf29f2

File tree

3 files changed

+65
-63
lines changed

3 files changed

+65
-63
lines changed

‎routers/web/repo/pull.go

+21-24
Original file line numberDiff line numberDiff line change
@@ -1133,49 +1133,47 @@ func MergePullRequest(ctx *context.Context) {
11331133
switch {
11341134
case errors.Is(err, pull_service.ErrIsClosed):
11351135
if issue.IsPull {
1136-
ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed"))
1136+
ctx.JSONError(ctx.Tr("repo.pulls.is_closed"))
11371137
} else {
1138-
ctx.Flash.Error(ctx.Tr("repo.issues.closed_title"))
1138+
ctx.JSONError(ctx.Tr("repo.issues.closed_title"))
11391139
}
11401140
case errors.Is(err, pull_service.ErrUserNotAllowedToMerge):
1141-
ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed"))
1141+
ctx.JSONError(ctx.Tr("repo.pulls.update_not_allowed"))
11421142
case errors.Is(err, pull_service.ErrHasMerged):
1143-
ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged"))
1143+
ctx.JSONError(ctx.Tr("repo.pulls.has_merged"))
11441144
case errors.Is(err, pull_service.ErrIsWorkInProgress):
1145-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip"))
1145+
ctx.JSONError(ctx.Tr("repo.pulls.no_merge_wip"))
11461146
case errors.Is(err, pull_service.ErrNotMergableState):
1147-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
1147+
ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready"))
11481148
case models.IsErrDisallowedToMerge(err):
1149-
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready"))
1149+
ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready"))
11501150
case asymkey_service.IsErrWontSign(err):
1151-
ctx.Flash.Error(err.Error()) // has no translation ...
1151+
ctx.JSONError(err.Error()) // has no translation ...
11521152
case errors.Is(err, pull_service.ErrDependenciesLeft):
1153-
ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
1153+
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
11541154
default:
11551155
ctx.ServerError("WebCheck", err)
1156-
return
11571156
}
11581157

1159-
ctx.Redirect(issue.Link())
11601158
return
11611159
}
11621160

11631161
// handle manually-merged mark
11641162
if manuallyMerged {
11651163
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
11661164
switch {
1167-
11681165
case models.IsErrInvalidMergeStyle(err):
1169-
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
1166+
ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option"))
11701167
case strings.Contains(err.Error(), "Wrong commit ID"):
1171-
ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id"))
1168+
ctx.JSONError(ctx.Tr("repo.pulls.wrong_commit_id"))
11721169
default:
11731170
ctx.ServerError("MergedManually", err)
1174-
return
11751171
}
1172+
1173+
return
11761174
}
11771175

1178-
ctx.Redirect(issue.Link())
1176+
ctx.JSONRedirect(issue.Link())
11791177
return
11801178
}
11811179

@@ -1205,15 +1203,14 @@ func MergePullRequest(ctx *context.Context) {
12051203
} else if scheduled {
12061204
// nothing more to do ...
12071205
ctx.Flash.Success(ctx.Tr("repo.pulls.auto_merge_newly_scheduled"))
1208-
ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pr.Index))
1206+
ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, pr.Index))
12091207
return
12101208
}
12111209
}
12121210

12131211
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil {
12141212
if models.IsErrInvalidMergeStyle(err) {
1215-
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
1216-
ctx.Redirect(issue.Link())
1213+
ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option"))
12171214
} else if models.IsErrMergeConflicts(err) {
12181215
conflictError := err.(models.ErrMergeConflicts)
12191216
flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{
@@ -1226,7 +1223,7 @@ func MergePullRequest(ctx *context.Context) {
12261223
return
12271224
}
12281225
ctx.Flash.Error(flashError)
1229-
ctx.Redirect(issue.Link())
1226+
ctx.JSONRedirect(issue.Link())
12301227
} else if models.IsErrRebaseConflicts(err) {
12311228
conflictError := err.(models.ErrRebaseConflicts)
12321229
flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{
@@ -1270,7 +1267,7 @@ func MergePullRequest(ctx *context.Context) {
12701267
}
12711268
ctx.Flash.Error(flashError)
12721269
}
1273-
ctx.Redirect(issue.Link())
1270+
ctx.JSONRedirect(issue.Link())
12741271
} else {
12751272
ctx.ServerError("Merge", err)
12761273
}
@@ -1279,7 +1276,7 @@ func MergePullRequest(ctx *context.Context) {
12791276
log.Trace("Pull request merged: %d", pr.ID)
12801277

12811278
if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
1282-
ctx.ServerError("CreateOrStopIssueStopwatch", err)
1279+
ctx.ServerError("stopTimerIfAvailable", err)
12831280
return
12841281
}
12851282

@@ -1293,7 +1290,7 @@ func MergePullRequest(ctx *context.Context) {
12931290
return
12941291
}
12951292
if exist {
1296-
ctx.Redirect(issue.Link())
1293+
ctx.JSONRedirect(issue.Link())
12971294
return
12981295
}
12991296

@@ -1311,7 +1308,7 @@ func MergePullRequest(ctx *context.Context) {
13111308
deleteBranch(ctx, pr, headRepo)
13121309
}
13131310

1314-
ctx.Redirect(issue.Link())
1311+
ctx.JSONRedirect(issue.Link())
13151312
}
13161313

13171314
// CancelAutoMergePullRequest cancels a scheduled pr

‎tests/integration/pull_merge_test.go

+8-1
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

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

0 commit comments

Comments
 (0)