finn/forgejo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Modernize merge button (#28140)

Browse source 
- 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>
This commit is contained in:
Earl Warren 2024-01-14 23:00:47 +01:00 committed by GitHub
parent 5a7bacb005
commit 3f342d6dff
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 65 additions and 63 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

45
routers/web/repo/pull.go
View file

@ -1149,30 +1149,28 @@ func MergePullRequest(ctx *context.Context) {
switch { switch {
case errors.Is(err, pull_service.ErrIsClosed): case errors.Is(err, pull_service.ErrIsClosed):
if issue.IsPull { if issue.IsPull {
ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed")) ctx.JSONError(ctx.Tr("repo.pulls.is_closed"))
} else { } 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): 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): 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): 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): 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): 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): 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): 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: default:
ctx.ServerError("WebCheck", err) ctx.ServerError("WebCheck", err)
return
} }
ctx.Redirect(issue.Link())
return return
} }
@ -1180,18 +1178,18 @@ func MergePullRequest(ctx *context.Context) {
if manuallyMerged { if manuallyMerged {
if err := pull_service.MergedManually(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { if err := pull_service.MergedManually(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
switch { switch {
case models.IsErrInvalidMergeStyle(err): 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"): 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: default:
ctx.ServerError("MergedManually", err) ctx.ServerError("MergedManually", err)
return
} }
return
} }
ctx.Redirect(issue.Link()) ctx.JSONRedirect(issue.Link())
return return
} }
@ -1221,15 +1219,14 @@ func MergePullRequest(ctx *context.Context) {
} else if scheduled { } else if scheduled {
// nothing more to do ... // nothing more to do ...
ctx.Flash.Success(ctx.Tr("repo.pulls.auto_merge_newly_scheduled")) 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 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 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) { 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) { } else if models.IsErrMergeConflicts(err) {
conflictError := err.(models.ErrMergeConflicts) conflictError := err.(models.ErrMergeConflicts)
flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{ flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{
@ -1242,7 +1239,7 @@ func MergePullRequest(ctx *context.Context) {
return return
} }
ctx.Flash.Error(flashError) ctx.Flash.Error(flashError)
ctx.Redirect(issue.Link()) ctx.JSONRedirect(issue.Link())
} else if models.IsErrRebaseConflicts(err) { } else if models.IsErrRebaseConflicts(err) {
conflictError := err.(models.ErrRebaseConflicts) conflictError := err.(models.ErrRebaseConflicts)
flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{ flashError, err := ctx.RenderToString(tplAlertDetails, map[string]any{
@ -1286,7 +1283,7 @@ func MergePullRequest(ctx *context.Context) {
} }
ctx.Flash.Error(flashError) ctx.Flash.Error(flashError)
} }
ctx.Redirect(issue.Link()) ctx.JSONRedirect(issue.Link())
} else { } else {
ctx.ServerError("Merge", err) ctx.ServerError("Merge", err)
} }
@ -1295,7 +1292,7 @@ func MergePullRequest(ctx *context.Context) {
log.Trace("Pull request merged: %d", pr.ID) log.Trace("Pull request merged: %d", pr.ID)
if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
ctx.ServerError("CreateOrStopIssueStopwatch", err) ctx.ServerError("stopTimerIfAvailable", err)
return return
} }
@ -1309,7 +1306,7 @@ func MergePullRequest(ctx *context.Context) {
return return
} }
if exist { if exist {
ctx.Redirect(issue.Link()) ctx.JSONRedirect(issue.Link())
return return
} }
@ -1327,7 +1324,7 @@ func MergePullRequest(ctx *context.Context) {
deleteBranch(ctx, pr, headRepo) deleteBranch(ctx, pr, headRepo)
} }
ctx.Redirect(issue.Link()) ctx.JSONRedirect(issue.Link())
} }
// CancelAutoMergePullRequest cancels a scheduled pr // CancelAutoMergePullRequest cancels a scheduled pr

9
tests/integration/pull_merge_test.go
View file

@ -45,7 +45,14 @@ func testPullMerge(t *testing.T, session *TestSession, user, repo, pullnum strin
"_csrf": htmlDoc.GetCSRF(), "_csrf": htmlDoc.GetCSRF(),
"do": string(mergeStyle), "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 return resp
} }

74
web_src/js/components/PullRequestMergeForm.vue
View file

@ -94,48 +94,46 @@ export default {
<!-- eslint-disable-next-line vue/no-v-html --> <!-- eslint-disable-next-line vue/no-v-html -->
<div v-if="mergeForm.hasPendingPullRequestMerge" v-html="mergeForm.hasPendingPullRequestMergeTip" class="ui info message"/> <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>
<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> </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> </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"> <div v-if="!showActionForm" class="gt-df">
<!-- the merge button --> <!-- the merge button -->