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 <[email protected]>

Author: JulienTant

custom/conf/app.ini.sample

@@ -60,6 +60,10 @@ FILE_MAX_SIZE = 3
 ; Max number of files per upload. Defaults to 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]
 ; Number of repositories that are displayed on one explore page
 EXPLORE_PAGING_NUM = 20

docs/content/doc/advanced/config-cheat-sheet.en-us.md

@@ -62,6 +62,10 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
    HTTP protocol.
 - `USE_COMPAT_SSH_URI`: **false**: Force ssh:// clone url instead of scp-style uri when
    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`)
 

docs/content/doc/usage/pull-request.en-us.md

@@ -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)

integrations/api_pull_test.go

@@ -5,10 +5,13 @@
 package integrations
 
 import (
+	"fmt"
 	"net/http"
 	"testing"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/modules/auth"
+	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/sdk/gitea"
 
 	"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))
 	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)
+}

integrations/pull_merge_test.go

@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/test"
 
+	"github.com/Unknwon/i18n"
 	"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)
 }
+
+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")
+}

models/pull.go

@@ -214,7 +214,7 @@ func (pr *PullRequest) APIFormat() *api.PullRequest {
 	}
 
 	if pr.Status != PullRequestStatusChecking {
-		mergeable := pr.Status != PullRequestStatusConflict
+		mergeable := pr.Status != PullRequestStatusConflict && !pr.IsWorkInProgress()
 		apiPullRequest.Mergeable = mergeable
 	}
 	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.
 // TODO: test more pull requests at same time.
 func TestPullRequests() {

models/pull_test.go

@@ -237,3 +237,34 @@ func TestChangeUsernameInPullRequests(t *testing.T) {
 	}
 	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())
+}

modules/setting/defaults.go

@@ -5,6 +5,7 @@ import (
 )
 
 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", ",")
-	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,한국어", ",")
+	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,한국어", ",")
+	defaultPullRequestWorkInProgressPrefixes = strings.Split("WIP:,[WIP]", ",")
 )

modules/setting/setting.go

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

options/locale/locale_en-US.ini

@@ -842,13 +842,16 @@ pulls.tab_files = Files Changed
 pulls.reopen_to_merge = Please reopen this pull request to perform a merge.
 pulls.merged = 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.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.cannot_auto_merge_desc = This pull request cannot be merged automatically due to 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_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.rebase_merge_pull_request = Rebase and Merge
 pulls.squash_merge_pull_request = Squash and Merge