Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable merging a WIP Pull request #4529

Merged
merged 15 commits into from
Aug 13, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions custom/conf/app.ini.sample
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions docs/content/doc/advanced/config-cheat-sheet.en-us.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`)

Expand Down
31 changes: 31 additions & 0 deletions docs/content/doc/usage/pull-request.en-us.md
Original file line number Diff line number Diff line change
@@ -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 :
Copy link
Member

@adelowo adelowo Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it’s title

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm pretty sure that its is correct, http://grammarist.com/spelling/its-its/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops... Thanks for the link


```
[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 into the dedicated page : [Issue and Pull Request templates](../issue-pull-request-templates)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be in the dedicated page not into the dedicated page

19 changes: 19 additions & 0 deletions integrations/api_pull_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
package integrations

import (
"fmt"
"net/http"
"testing"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/auth"
api "code.gitea.io/sdk/gitea"

"github.com/stretchr/testify/assert"
Expand All @@ -28,3 +30,20 @@ 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, HasMerged: false}).(*models.PullRequest)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this PR really have WIP prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow this is a mistake i'm sorry :/

pr.LoadIssue()

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)
}
21 changes: 21 additions & 0 deletions integrations/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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")
}
33 changes: 32 additions & 1 deletion models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -1195,6 +1195,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() {
Expand Down
31 changes: 31 additions & 0 deletions models/pull_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
5 changes: 3 additions & 2 deletions modules/setting/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]", ",")
)
14 changes: 14 additions & 0 deletions modules/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,11 @@ var (
LocalCopyPath string
LocalWikiPath string
} `ini:"-"`

// Pull request settings
PullRequest struct {
WorkInProgressPrefixes []string
} `ini:"repository.pull-request"`
}{
AnsiCharset: "",
ForcePrivate: false,
Expand Down Expand Up @@ -266,6 +271,13 @@ var (
LocalCopyPath: "tmp/local-repo",
LocalWikiPath: "tmp/local-wiki",
},

// Pull request settings
PullRequest: struct {
WorkInProgressPrefixes []string
}{
WorkInProgressPrefixes: defaultPullRequestWorkInProgressPrefixes,
},
}
RepoRootPath string
ScriptType = "bash"
Expand Down Expand Up @@ -1024,6 +1036,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) {
Expand Down
3 changes: 3 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -829,13 +829,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.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be needed backtick (`) quotes

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for quoting with "

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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for quoting with "

pulls.merge_pull_request = Merge Pull Request
pulls.rebase_merge_pull_request = Rebase and Merge
pulls.squash_merge_pull_request = Squash and Merge
Expand Down
22 changes: 22 additions & 0 deletions public/js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,27 @@ function u2fRegisterRequest() {
});
}

function initWipTitle() {
$(".title_wip_desc > a").click(function (e) {
e.preventDefault();

var $issueTitle = $("#issue_title");
var value = $issueTitle.val().trim().toUpperCase();

var addPrefix = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for this variable. Can be changed so:

        for (var i in wipPrefixes) {
            if (value.startsWith(wipPrefixes[i].toUpperCase())) {
                return;
            }
        }

        $issueTitle.val(wipPrefixes[0] + " " + $issueTitle.val());

for (var i in wipPrefixes) {
if (value.startsWith(wipPrefixes[i].toUpperCase())) {
addPrefix = false;
break;
}
}

if (addPrefix) {
$issueTitle.val(wipPrefixes[0] + " " + $issueTitle.val());
}
});
}

$(document).ready(function () {
csrf = $('meta[name=_csrf]').attr("content");
suburl = $('meta[name=_suburl]').attr("content");
Expand Down Expand Up @@ -1771,6 +1792,7 @@ $(document).ready(function () {
initU2FAuth();
initU2FRegister();
initIssueList();
initWipTitle();

// Repo clone url.
if ($('#repo-clone-url').length > 0) {
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
return
}

if !pr.CanAutoMerge() || pr.HasMerged {
if !pr.CanAutoMerge() || pr.HasMerged || pr.IsWorkInProgress() {
ctx.Status(405)
return
}
Expand Down
2 changes: 2 additions & 0 deletions routers/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ func NewIssue(ctx *context.Context) {
ctx.Data["RequireHighlightJS"] = true
ctx.Data["RequireSimpleMDE"] = true
ctx.Data["RequireTribute"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates)
renderAttachmentSettings(ctx)

Expand Down Expand Up @@ -449,6 +450,7 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) {
ctx.Data["RequireHighlightJS"] = true
ctx.Data["RequireSimpleMDE"] = true
ctx.Data["ReadOnly"] = false
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
renderAttachmentSettings(ctx)

var (
Expand Down
14 changes: 14 additions & 0 deletions routers/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.PullReq
ctx.ServerError("GetPullRequestInfo", err)
return nil
}

if pull.IsWorkInProgress() {
ctx.Data["IsPullWorkInProgress"] = true
ctx.Data["WorkInProgressPrefix"] = pull.GetWorkInProgressPrefix()
}

ctx.Data["NumCommits"] = prInfo.Commits.Len()
ctx.Data["NumFiles"] = prInfo.NumFiles
return prInfo
Expand Down Expand Up @@ -501,6 +507,12 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
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() {
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
Expand Down Expand Up @@ -732,6 +744,7 @@ func CompareAndPullRequest(ctx *context.Context) {
ctx.Data["IsDiffCompare"] = true
ctx.Data["RequireHighlightJS"] = true
ctx.Data["RequireTribute"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
setTemplateIfExists(ctx, pullRequestTemplateKey, pullRequestTemplateCandidates)
renderAttachmentSettings(ctx)

Expand Down Expand Up @@ -775,6 +788,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm)
ctx.Data["PageIsComparePull"] = true
ctx.Data["IsDiffCompare"] = true
ctx.Data["RequireHighlightJS"] = true
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
renderAttachmentSettings(ctx)

var (
Expand Down
9 changes: 8 additions & 1 deletion templates/repo/issue/new_form.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
</a>
<div class="ui segment content">
<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>
{{template "repo/issue/comment_tab" .}}
<div class="text right">
Expand Down Expand Up @@ -150,3 +153,7 @@
</div>
</div>
</form>
{{if .PageIsComparePull}}
<script>window.workInProgressPrefixes = {{.PullRequestWorkInProgressPrefixes}}</script>
{{end}}

Loading