Disable merging a WIP Pull request (#4529)

* prevent pull request to be merged when PR is a WIP

* add tests

* add helper to prepend WIP: in PR title

* move default wip prefixes into settings

* use configurable WIP prefixes in javascript and default to first one in templates

* add documentation

* add unit test on pull model

Signed-off-by: Julien Tant <julien@craftyx.fr>
This commit is contained in:
Julien Tant 2018-08-13 21:04:39 +02:00 committed by Lauris BH
parent 52c2cb15db
commit 7781e8cef2
16 changed files with 218 additions and 5 deletions

View File

@ -60,6 +60,10 @@ FILE_MAX_SIZE = 3
; Max number of files per upload. Defaults to 5 ; Max number of files per upload. Defaults to 5
MAX_FILES = 5 MAX_FILES = 5
[repository.pull-request]
; List of prefixes used in Pull Request title to mark them as Work In Progress
WORK_IN_PROGRESS_PREFIXES=WIP:,[WIP]
[ui] [ui]
; Number of repositories that are displayed on one explore page ; Number of repositories that are displayed on one explore page
EXPLORE_PAGING_NUM = 20 EXPLORE_PAGING_NUM = 20

View File

@ -63,6 +63,10 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
- `USE_COMPAT_SSH_URI`: **false**: Force ssh:// clone url instead of scp-style uri when - `USE_COMPAT_SSH_URI`: **false**: Force ssh:// clone url instead of scp-style uri when
default SSH port is used. default SSH port is used.
### Repository - Pull Request (`repository.pull-request`)
- `WORK_IN_PROGRESS_PREFIXES`: **WIP:,\[WIP\]**: List of prefixes used in Pull Request
title to mark them as Work In Progress
## UI (`ui`) ## UI (`ui`)
- `EXPLORE_PAGING_NUM`: **20**: Number of repositories that are shown in one explore page. - `EXPLORE_PAGING_NUM`: **20**: Number of repositories that are shown in one explore page.

View File

@ -0,0 +1,31 @@
---
date: "2018-06-01T19:00:00+02:00"
title: "Usage: Pull Request"
slug: "pull-request"
weight: 13
toc: true
draft: false
menu:
sidebar:
parent: "usage"
name: "Pull Request"
weight: 13
identifier: "pull-request"
---
# Pull Request
## "Work In Progress" pull requests
Marking a pull request as being a work in progress will prevent that pull request from being accidentally merged. To mark a pull request as being a work in progress, you must prefix its title by `WIP:` or `[WIP]` (case insensitive). Those values are configurable in your `app.ini` file :
```
[repository.pull-request]
WORK_IN_PROGRESS_PREFIXES=WIP:,[WIP]
```
The first value of the list will be used in helpers.
## Pull Request Templates
You can find more information about pull request templates in the dedicated page : [Issue and Pull Request templates](../issue-pull-request-templates)

View File

@ -5,10 +5,13 @@
package integrations package integrations
import ( import (
"fmt"
"net/http" "net/http"
"testing" "testing"
"code.gitea.io/gitea/models" "code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/auth"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/sdk/gitea" api "code.gitea.io/sdk/gitea"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@ -28,3 +31,26 @@ func TestAPIViewPulls(t *testing.T) {
expectedLen := models.GetCount(t, &models.Issue{RepoID: repo.ID}, models.Cond("is_pull = ?", true)) expectedLen := models.GetCount(t, &models.Issue{RepoID: repo.ID}, models.Cond("is_pull = ?", true))
assert.Len(t, pulls, expectedLen) assert.Len(t, pulls, expectedLen)
} }
// TestAPIMergePullWIP ensures that we can't merge a WIP pull request
func TestAPIMergePullWIP(t *testing.T) {
prepareTestEnv(t)
repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)
owner := models.AssertExistsAndLoadBean(t, &models.User{ID: repo.OwnerID}).(*models.User)
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{Status: models.PullRequestStatusMergeable}, models.Cond("has_merged = ?", false)).(*models.PullRequest)
pr.LoadIssue()
pr.Issue.ChangeTitle(owner, setting.Repository.PullRequest.WorkInProgressPrefixes[0]+" "+pr.Issue.Title)
// force reload
pr.LoadAttributes()
assert.Contains(t, pr.Issue.Title, setting.Repository.PullRequest.WorkInProgressPrefixes[0])
session := loginUser(t, owner.Name)
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, pr.Index), &auth.MergePullRequestForm{
MergeMessageField: pr.Issue.Title,
Do: string(models.MergeStyleMerge),
})
session.MakeRequest(t, req, http.StatusMethodNotAllowed)
}

View File

@ -14,6 +14,7 @@ import (
"code.gitea.io/gitea/models" "code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/test"
"github.com/Unknwon/i18n"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )
@ -123,3 +124,23 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
assert.EqualValues(t, "Branch 'user1/feature/test' has been deleted.", resultMsg) assert.EqualValues(t, "Branch 'user1/feature/test' has been deleted.", resultMsg)
} }
func TestCantMergeWorkInProgress(t *testing.T) {
prepareTestEnv(t)
session := loginUser(t, "user1")
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
resp := testPullCreate(t, session, "user1", "repo1", "master", "[wip] This is a pull title")
req := NewRequest(t, "GET", resp.Header().Get("Location"))
resp = session.MakeRequest(t, req, http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
text := strings.TrimSpace(htmlDoc.doc.Find(".merge.segment > .text.grey").Text())
assert.NotEmpty(t, text, "Can't find WIP text")
// remove <strong /> from lang
expected := i18n.Tr("en", "repo.pulls.cannot_merge_work_in_progress", "[wip]")
replacer := strings.NewReplacer("<strong>", "", "</strong>", "")
assert.Equal(t, replacer.Replace(expected), text, "Unable to find WIP text")
}

View File

@ -214,7 +214,7 @@ func (pr *PullRequest) APIFormat() *api.PullRequest {
} }
if pr.Status != PullRequestStatusChecking { if pr.Status != PullRequestStatusChecking {
mergeable := pr.Status != PullRequestStatusConflict mergeable := pr.Status != PullRequestStatusConflict && !pr.IsWorkInProgress()
apiPullRequest.Mergeable = mergeable apiPullRequest.Mergeable = mergeable
} }
if pr.HasMerged { if pr.HasMerged {
@ -1247,6 +1247,37 @@ func (pr *PullRequest) checkAndUpdateStatus() {
} }
} }
// IsWorkInProgress determine if the Pull Request is a Work In Progress by its title
func (pr *PullRequest) IsWorkInProgress() bool {
if err := pr.LoadIssue(); err != nil {
log.Error(4, "LoadIssue: %v", err)
return false
}
for _, prefix := range setting.Repository.PullRequest.WorkInProgressPrefixes {
if strings.HasPrefix(strings.ToUpper(pr.Issue.Title), prefix) {
return true
}
}
return false
}
// GetWorkInProgressPrefix returns the prefix used to mark the pull request as a work in progress.
// It returns an empty string when none were found
func (pr *PullRequest) GetWorkInProgressPrefix() string {
if err := pr.LoadIssue(); err != nil {
log.Error(4, "LoadIssue: %v", err)
return ""
}
for _, prefix := range setting.Repository.PullRequest.WorkInProgressPrefixes {
if strings.HasPrefix(strings.ToUpper(pr.Issue.Title), prefix) {
return pr.Issue.Title[0:len(prefix)]
}
}
return ""
}
// TestPullRequests checks and tests untested patches of pull requests. // TestPullRequests checks and tests untested patches of pull requests.
// TODO: test more pull requests at same time. // TODO: test more pull requests at same time.
func TestPullRequests() { func TestPullRequests() {

View File

@ -237,3 +237,34 @@ func TestChangeUsernameInPullRequests(t *testing.T) {
} }
CheckConsistencyFor(t, &PullRequest{}) CheckConsistencyFor(t, &PullRequest{})
} }
func TestPullRequest_IsWorkInProgress(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
pr.LoadIssue()
assert.False(t, pr.IsWorkInProgress())
pr.Issue.Title = "WIP: " + pr.Issue.Title
assert.True(t, pr.IsWorkInProgress())
pr.Issue.Title = "[wip]: " + pr.Issue.Title
assert.True(t, pr.IsWorkInProgress())
}
func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
pr.LoadIssue()
assert.Empty(t, pr.GetWorkInProgressPrefix())
original := pr.Issue.Title
pr.Issue.Title = "WIP: " + original
assert.Equal(t, "WIP:", pr.GetWorkInProgressPrefix())
pr.Issue.Title = "[wip] " + original
assert.Equal(t, "[wip]", pr.GetWorkInProgressPrefix())
}

View File

@ -7,4 +7,5 @@ import (
var ( var (
defaultLangs = strings.Split("en-US,zh-CN,zh-HK,zh-TW,de-DE,fr-FR,nl-NL,lv-LV,ru-RU,uk-UA,ja-JP,es-ES,pt-BR,pl-PL,bg-BG,it-IT,fi-FI,tr-TR,cs-CZ,sr-SP,sv-SE,ko-KR", ",") defaultLangs = strings.Split("en-US,zh-CN,zh-HK,zh-TW,de-DE,fr-FR,nl-NL,lv-LV,ru-RU,uk-UA,ja-JP,es-ES,pt-BR,pl-PL,bg-BG,it-IT,fi-FI,tr-TR,cs-CZ,sr-SP,sv-SE,ko-KR", ",")
defaultLangNames = strings.Split("English,简体中文,繁體中文(香港),繁體中文(台灣),Deutsch,français,Nederlands,latviešu,русский,Українська,日本語,español,português do Brasil,polski,български,italiano,suomi,Türkçe,čeština,српски,svenska,한국어", ",") defaultLangNames = strings.Split("English,简体中文,繁體中文(香港),繁體中文(台灣),Deutsch,français,Nederlands,latviešu,русский,Українська,日本語,español,português do Brasil,polski,български,italiano,suomi,Türkçe,čeština,српски,svenska,한국어", ",")
defaultPullRequestWorkInProgressPrefixes = strings.Split("WIP:,[WIP]", ",")
) )

View File

@ -224,6 +224,11 @@ var (
LocalCopyPath string LocalCopyPath string
LocalWikiPath string LocalWikiPath string
} `ini:"-"` } `ini:"-"`
// Pull request settings
PullRequest struct {
WorkInProgressPrefixes []string
} `ini:"repository.pull-request"`
}{ }{
AnsiCharset: "", AnsiCharset: "",
ForcePrivate: false, ForcePrivate: false,
@ -267,6 +272,13 @@ var (
LocalCopyPath: "tmp/local-repo", LocalCopyPath: "tmp/local-repo",
LocalWikiPath: "tmp/local-wiki", LocalWikiPath: "tmp/local-wiki",
}, },
// Pull request settings
PullRequest: struct {
WorkInProgressPrefixes []string
}{
WorkInProgressPrefixes: defaultPullRequestWorkInProgressPrefixes,
},
} }
RepoRootPath string RepoRootPath string
ScriptType = "bash" ScriptType = "bash"
@ -1031,6 +1043,8 @@ func NewContext() {
log.Fatal(4, "Failed to map Repository.Upload settings: %v", err) log.Fatal(4, "Failed to map Repository.Upload settings: %v", err)
} else if err = Cfg.Section("repository.local").MapTo(&Repository.Local); err != nil { } else if err = Cfg.Section("repository.local").MapTo(&Repository.Local); err != nil {
log.Fatal(4, "Failed to map Repository.Local settings: %v", err) log.Fatal(4, "Failed to map Repository.Local settings: %v", err)
} else if err = Cfg.Section("repository.pull-request").MapTo(&Repository.PullRequest); err != nil {
log.Fatal(4, "Failed to map Repository.PullRequest settings: %v", err)
} }
if !filepath.IsAbs(Repository.Upload.TempPath) { if !filepath.IsAbs(Repository.Upload.TempPath) {

View File

@ -842,6 +842,8 @@ pulls.tab_files = Files Changed
pulls.reopen_to_merge = Please reopen this pull request to perform a merge. pulls.reopen_to_merge = Please reopen this pull request to perform a merge.
pulls.merged = Merged pulls.merged = Merged
pulls.has_merged = The pull request has been merged. pulls.has_merged = The pull request has been merged.
pulls.title_wip_desc = `<a href="#">Start the title with <strong>%s</strong></a> to prevent the pull request from being merged accidentally.`
pulls.cannot_merge_work_in_progress = This pull request is marked as a work in progress. Remove the <strong>%s</strong> prefix from the title when it's ready
pulls.data_broken = This pull request is broken due to missing fork information. pulls.data_broken = This pull request is broken due to missing fork information.
pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments." pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments."
pulls.can_auto_merge_desc = This pull request can be merged automatically. pulls.can_auto_merge_desc = This pull request can be merged automatically.
@ -849,6 +851,7 @@ pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically
pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts. pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled. pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled.
pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually. pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually.
pulls.no_merge_wip = This pull request can not be merged because it is marked as being a work in progress.
pulls.merge_pull_request = Merge Pull Request pulls.merge_pull_request = Merge Pull Request
pulls.rebase_merge_pull_request = Rebase and Merge pulls.rebase_merge_pull_request = Rebase and Merge
pulls.squash_merge_pull_request = Squash and Merge pulls.squash_merge_pull_request = Squash and Merge

View File

@ -1655,6 +1655,23 @@ function u2fRegisterRequest() {
}); });
} }
function initWipTitle() {
$(".title_wip_desc > a").click(function (e) {
e.preventDefault();
var $issueTitle = $("#issue_title");
var value = $issueTitle.val().trim().toUpperCase();
for (var i in wipPrefixes) {
if (value.startsWith(wipPrefixes[i].toUpperCase())) {
return;
}
}
$issueTitle.val(wipPrefixes[0] + " " + $issueTitle.val());
});
}
$(document).ready(function () { $(document).ready(function () {
csrf = $('meta[name=_csrf]').attr("content"); csrf = $('meta[name=_csrf]').attr("content");
suburl = $('meta[name=_suburl]').attr("content"); suburl = $('meta[name=_suburl]').attr("content");
@ -1869,6 +1886,7 @@ $(document).ready(function () {
initU2FAuth(); initU2FAuth();
initU2FRegister(); initU2FRegister();
initIssueList(); initIssueList();
initWipTitle();
initPullRequestReview(); initPullRequestReview();
// Repo clone url. // Repo clone url.

View File

@ -510,7 +510,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
return return
} }
if !pr.CanAutoMerge() || pr.HasMerged { if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
ctx.Status(405) ctx.Status(405)
return return
} }

View File

@ -356,6 +356,7 @@ func NewIssue(ctx *context.Context) {
ctx.Data["RequireHighlightJS"] = true ctx.Data["RequireHighlightJS"] = true
ctx.Data["RequireSimpleMDE"] = true ctx.Data["RequireSimpleMDE"] = true
ctx.Data["RequireTribute"] = true ctx.Data["RequireTribute"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates) setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates)
renderAttachmentSettings(ctx) renderAttachmentSettings(ctx)
@ -449,6 +450,7 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) {
ctx.Data["RequireHighlightJS"] = true ctx.Data["RequireHighlightJS"] = true
ctx.Data["RequireSimpleMDE"] = true ctx.Data["RequireSimpleMDE"] = true
ctx.Data["ReadOnly"] = false ctx.Data["ReadOnly"] = false
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
renderAttachmentSettings(ctx) renderAttachmentSettings(ctx)
var ( var (

View File

@ -323,6 +323,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.PullReq
ctx.ServerError("GetPullRequestInfo", err) ctx.ServerError("GetPullRequestInfo", err)
return nil return nil
} }
if pull.IsWorkInProgress() {
ctx.Data["IsPullWorkInProgress"] = true
ctx.Data["WorkInProgressPrefix"] = pull.GetWorkInProgressPrefix()
}
ctx.Data["NumCommits"] = prInfo.Commits.Len() ctx.Data["NumCommits"] = prInfo.Commits.Len()
ctx.Data["NumFiles"] = prInfo.NumFiles ctx.Data["NumFiles"] = prInfo.NumFiles
return prInfo return prInfo
@ -516,6 +522,12 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
return return
} }
if pr.IsWorkInProgress() {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip"))
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
return
}
if ctx.HasError() { if ctx.HasError() {
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string)) ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
@ -747,6 +759,7 @@ func CompareAndPullRequest(ctx *context.Context) {
ctx.Data["IsDiffCompare"] = true ctx.Data["IsDiffCompare"] = true
ctx.Data["RequireHighlightJS"] = true ctx.Data["RequireHighlightJS"] = true
ctx.Data["RequireTribute"] = true ctx.Data["RequireTribute"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
setTemplateIfExists(ctx, pullRequestTemplateKey, pullRequestTemplateCandidates) setTemplateIfExists(ctx, pullRequestTemplateKey, pullRequestTemplateCandidates)
renderAttachmentSettings(ctx) renderAttachmentSettings(ctx)
@ -790,6 +803,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm)
ctx.Data["PageIsComparePull"] = true ctx.Data["PageIsComparePull"] = true
ctx.Data["IsDiffCompare"] = true ctx.Data["IsDiffCompare"] = true
ctx.Data["RequireHighlightJS"] = true ctx.Data["RequireHighlightJS"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
renderAttachmentSettings(ctx) renderAttachmentSettings(ctx)
var ( var (

View File

@ -13,7 +13,10 @@
</a> </a>
<div class="ui segment content"> <div class="ui segment content">
<div class="field"> <div class="field">
<input name="title" placeholder="{{.i18n.Tr "repo.milestones.title"}}" value="{{.title}}" tabindex="3" autofocus required> <input name="title" id="issue_title" placeholder="{{.i18n.Tr "repo.milestones.title"}}" value="{{.title}}" tabindex="3" autofocus required>
{{if .PageIsComparePull}}
<span class="title_wip_desc">{{.i18n.Tr "repo.pulls.title_wip_desc" (index .PullRequestWorkInProgressPrefixes 0) | Safe}}</span>
{{end}}
</div> </div>
{{template "repo/issue/comment_tab" .}} {{template "repo/issue/comment_tab" .}}
<div class="text right"> <div class="text right">
@ -150,3 +153,7 @@
</div> </div>
</div> </div>
</form> </form>
{{if .PageIsComparePull}}
<script>window.wipPrefixes = {{.PullRequestWorkInProgressPrefixes}}</script>
{{end}}

View File

@ -2,6 +2,7 @@
<a class="avatar text <a class="avatar text
{{if .Issue.PullRequest.HasMerged}}purple {{if .Issue.PullRequest.HasMerged}}purple
{{else if .Issue.IsClosed}}grey {{else if .Issue.IsClosed}}grey
{{else if .IsPullWorkInProgress}}grey
{{else if .IsPullRequestBroken}}red {{else if .IsPullRequestBroken}}red
{{else if .Issue.PullRequest.IsChecking}}yellow {{else if .Issue.PullRequest.IsChecking}}yellow
{{else if .Issue.PullRequest.CanAutoMerge}}green {{else if .Issue.PullRequest.CanAutoMerge}}green
@ -27,6 +28,11 @@
<span class="octicon octicon-x"></span> <span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.data_broken"}} {{$.i18n.Tr "repo.pulls.data_broken"}}
</div> </div>
{{else if .IsPullWorkInProgress}}
<div class="item text grey">
<span class="octicon octicon-x"></span>
{{$.i18n.Tr "repo.pulls.cannot_merge_work_in_progress" .WorkInProgressPrefix | Str2html}}
</div>
{{else if .Issue.PullRequest.IsChecking}} {{else if .Issue.PullRequest.IsChecking}}
<div class="item text yellow"> <div class="item text yellow">
<span class="octicon octicon-sync"></span> <span class="octicon octicon-sync"></span>