From b15f26b1cf3dc976ae400d4a3c73ec3bd4a50bc6 Mon Sep 17 00:00:00 2001
From: guillep2k <18600385+guillep2k@users.noreply.github.com>
Date: Mon, 18 Nov 2019 10:13:07 -0300
Subject: [PATCH] Close/reopen issues by keywords in titles and comments
(#8866)
* Add close/reopen from comment functionality
* Fix comment
* Rewrite closing/reopening template
* Check xref permissions, move action to services/pull
* Fix RefIsPull field
* Add xref tests
* Fix xref unique filter
* Only highlight keywords for actionable xrefs
* Fix xref neuter filter
* Fix check return status
* Restart CI
---
models/issue_xref.go | 103 ++++++++---
models/issue_xref_test.go | 164 ++++++++++++++++++
modules/markup/html.go | 12 +-
modules/references/references.go | 5 +
options/locale/locale_en-US.ini | 11 +-
services/pull/merge.go | 22 +++
.../repo/issue/view_content/comments.tmpl | 29 ++--
7 files changed, 300 insertions(+), 46 deletions(-)
create mode 100644 models/issue_xref_test.go
diff --git a/models/issue_xref.go b/models/issue_xref.go
index 787a0e5119a..41937dc6753 100644
--- a/models/issue_xref.go
+++ b/models/issue_xref.go
@@ -5,6 +5,8 @@
package models
import (
+ "fmt"
+
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/references"
@@ -27,13 +29,9 @@ type crossReferencesContext struct {
func neuterCrossReferences(e Engine, issueID int64, commentID int64) error {
active := make([]*Comment, 0, 10)
- sess := e.Where("`ref_action` IN (?, ?, ?)", references.XRefActionNone, references.XRefActionCloses, references.XRefActionReopens)
- if issueID != 0 {
- sess = sess.And("`ref_issue_id` = ?", issueID)
- }
- if commentID != 0 {
- sess = sess.And("`ref_comment_id` = ?", commentID)
- }
+ sess := e.Where("`ref_action` IN (?, ?, ?)", references.XRefActionNone, references.XRefActionCloses, references.XRefActionReopens).
+ And("`ref_issue_id` = ?", issueID).
+ And("`ref_comment_id` = ?", commentID)
if err := sess.Find(&active); err != nil || len(active) == 0 {
return err
}
@@ -87,7 +85,7 @@ func (issue *Issue) createCrossReferences(e *xorm.Session, ctx *crossReferencesC
RefIssueID: ctx.OrigIssue.ID,
RefCommentID: refCommentID,
RefAction: xref.Action,
- RefIsPull: xref.Issue.IsPull,
+ RefIsPull: ctx.OrigIssue.IsPull,
}); err != nil {
return err
}
@@ -98,9 +96,10 @@ func (issue *Issue) createCrossReferences(e *xorm.Session, ctx *crossReferencesC
func (issue *Issue) getCrossReferences(e *xorm.Session, ctx *crossReferencesContext, plaincontent, mdcontent string) ([]*crossReference, error) {
xreflist := make([]*crossReference, 0, 5)
var (
- refRepo *Repository
- refIssue *Issue
- err error
+ refRepo *Repository
+ refIssue *Issue
+ refAction references.XRefAction
+ err error
)
allrefs := append(references.FindAllIssueReferences(plaincontent), references.FindAllIssueReferencesMarkdown(mdcontent)...)
@@ -122,15 +121,13 @@ func (issue *Issue) getCrossReferences(e *xorm.Session, ctx *crossReferencesCont
return nil, err
}
}
- if refIssue, err = ctx.OrigIssue.findReferencedIssue(e, ctx, refRepo, ref.Index); err != nil {
+ if refIssue, refAction, err = ctx.OrigIssue.verifyReferencedIssue(e, ctx, refRepo, ref); err != nil {
return nil, err
}
if refIssue != nil {
xreflist = ctx.OrigIssue.updateCrossReferenceList(xreflist, &crossReference{
- Issue: refIssue,
- // FIXME: currently ignore keywords
- // Action: ref.Action,
- Action: references.XRefActionNone,
+ Issue: refIssue,
+ Action: refAction,
})
}
}
@@ -153,25 +150,42 @@ func (issue *Issue) updateCrossReferenceList(list []*crossReference, xref *cross
return append(list, xref)
}
-func (issue *Issue) findReferencedIssue(e Engine, ctx *crossReferencesContext, repo *Repository, index int64) (*Issue, error) {
- refIssue := &Issue{RepoID: repo.ID, Index: index}
+// verifyReferencedIssue will check if the referenced issue exists, and whether the doer has permission to do what
+func (issue *Issue) verifyReferencedIssue(e Engine, ctx *crossReferencesContext, repo *Repository,
+ ref references.IssueReference) (*Issue, references.XRefAction, error) {
+
+ refIssue := &Issue{RepoID: repo.ID, Index: ref.Index}
+ refAction := ref.Action
+
if has, _ := e.Get(refIssue); !has {
- return nil, nil
+ return nil, references.XRefActionNone, nil
}
if err := refIssue.loadRepo(e); err != nil {
- return nil, err
+ return nil, references.XRefActionNone, err
}
- // Check user permissions
- if refIssue.RepoID != ctx.OrigIssue.RepoID {
+
+ // Close/reopen actions can only be set from pull requests to issues
+ if refIssue.IsPull || !issue.IsPull {
+ refAction = references.XRefActionNone
+ }
+
+ // Check doer permissions; set action to None if the doer can't change the destination
+ if refIssue.RepoID != ctx.OrigIssue.RepoID || ref.Action != references.XRefActionNone {
perm, err := getUserRepoPermission(e, refIssue.Repo, ctx.Doer)
if err != nil {
- return nil, err
+ return nil, references.XRefActionNone, err
}
if !perm.CanReadIssuesOrPulls(refIssue.IsPull) {
- return nil, nil
+ return nil, references.XRefActionNone, nil
+ }
+ if ref.Action != references.XRefActionNone &&
+ ctx.Doer.ID != refIssue.PosterID &&
+ !perm.CanWriteIssuesOrPulls(refIssue.IsPull) {
+ refAction = references.XRefActionNone
}
}
- return refIssue, nil
+
+ return refIssue, refAction, nil
}
func (issue *Issue) neuterCrossReferences(e Engine) error {
@@ -203,7 +217,7 @@ func (comment *Comment) addCrossReferences(e *xorm.Session, doer *User) error {
}
func (comment *Comment) neuterCrossReferences(e Engine) error {
- return neuterCrossReferences(e, 0, comment.ID)
+ return neuterCrossReferences(e, comment.IssueID, comment.ID)
}
// LoadRefComment loads comment that created this reference from database
@@ -268,3 +282,40 @@ func (comment *Comment) RefIssueIdent() string {
// FIXME: check this name for cross-repository references (#7901 if it gets merged)
return "#" + com.ToStr(comment.RefIssue.Index)
}
+
+// __________ .__ .__ __________ __
+// \______ \__ __| | | |\______ \ ____ ________ __ ____ _______/ |_
+// | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\
+// | | | | / |_| |_| | \ ___< <_| | | /\ ___/ \___ \ | |
+// |____| |____/|____/____/____|_ /\___ >__ |____/ \___ >____ > |__|
+// \/ \/ |__| \/ \/
+
+// ResolveCrossReferences will return the list of references to close/reopen by this PR
+func (pr *PullRequest) ResolveCrossReferences() ([]*Comment, error) {
+ unfiltered := make([]*Comment, 0, 5)
+ if err := x.
+ Where("ref_repo_id = ? AND ref_issue_id = ?", pr.Issue.RepoID, pr.Issue.ID).
+ In("ref_action", []references.XRefAction{references.XRefActionCloses, references.XRefActionReopens}).
+ OrderBy("id").
+ Find(&unfiltered); err != nil {
+ return nil, fmt.Errorf("get reference: %v", err)
+ }
+
+ refs := make([]*Comment, 0, len(unfiltered))
+ for _, ref := range unfiltered {
+ found := false
+ for i, r := range refs {
+ if r.IssueID == ref.IssueID {
+ // Keep only the latest
+ refs[i] = ref
+ found = true
+ break
+ }
+ }
+ if !found {
+ refs = append(refs, ref)
+ }
+ }
+
+ return refs, nil
+}
diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go
new file mode 100644
index 00000000000..4fc6011d788
--- /dev/null
+++ b/models/issue_xref_test.go
@@ -0,0 +1,164 @@
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package models
+
+import (
+ "fmt"
+ "testing"
+
+ "code.gitea.io/gitea/modules/references"
+
+ "github.com/stretchr/testify/assert"
+)
+
+func TestXRef_AddCrossReferences(t *testing.T) {
+ assert.NoError(t, PrepareTestDatabase())
+
+ // Issue #1 to test against
+ itarget := testCreateIssue(t, 1, 2, "title1", "content1", false)
+
+ // PR to close issue #1
+ content := fmt.Sprintf("content2, closes #%d", itarget.Index)
+ pr := testCreateIssue(t, 1, 2, "title2", content, true)
+ ref := AssertExistsAndLoadBean(t, &Comment{IssueID: itarget.ID, RefIssueID: pr.ID, RefCommentID: 0}).(*Comment)
+ assert.Equal(t, CommentTypePullRef, ref.Type)
+ assert.Equal(t, pr.RepoID, ref.RefRepoID)
+ assert.Equal(t, true, ref.RefIsPull)
+ assert.Equal(t, references.XRefActionCloses, ref.RefAction)
+
+ // Comment on PR to reopen issue #1
+ content = fmt.Sprintf("content2, reopens #%d", itarget.Index)
+ c := testCreateComment(t, 1, 2, pr.ID, content)
+ ref = AssertExistsAndLoadBean(t, &Comment{IssueID: itarget.ID, RefIssueID: pr.ID, RefCommentID: c.ID}).(*Comment)
+ assert.Equal(t, CommentTypeCommentRef, ref.Type)
+ assert.Equal(t, pr.RepoID, ref.RefRepoID)
+ assert.Equal(t, true, ref.RefIsPull)
+ assert.Equal(t, references.XRefActionReopens, ref.RefAction)
+
+ // Issue mentioning issue #1
+ content = fmt.Sprintf("content3, mentions #%d", itarget.Index)
+ i := testCreateIssue(t, 1, 2, "title3", content, false)
+ ref = AssertExistsAndLoadBean(t, &Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}).(*Comment)
+ assert.Equal(t, CommentTypeIssueRef, ref.Type)
+ assert.Equal(t, pr.RepoID, ref.RefRepoID)
+ assert.Equal(t, false, ref.RefIsPull)
+ assert.Equal(t, references.XRefActionNone, ref.RefAction)
+
+ // Issue #4 to test against
+ itarget = testCreateIssue(t, 3, 3, "title4", "content4", false)
+
+ // Cross-reference to issue #4 by admin
+ content = fmt.Sprintf("content5, mentions user3/repo3#%d", itarget.Index)
+ i = testCreateIssue(t, 2, 1, "title5", content, false)
+ ref = AssertExistsAndLoadBean(t, &Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}).(*Comment)
+ assert.Equal(t, CommentTypeIssueRef, ref.Type)
+ assert.Equal(t, i.RepoID, ref.RefRepoID)
+ assert.Equal(t, false, ref.RefIsPull)
+ assert.Equal(t, references.XRefActionNone, ref.RefAction)
+
+ // Cross-reference to issue #4 with no permission
+ content = fmt.Sprintf("content6, mentions user3/repo3#%d", itarget.Index)
+ i = testCreateIssue(t, 4, 5, "title6", content, false)
+ AssertNotExistsBean(t, &Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0})
+}
+
+func TestXRef_NeuterCrossReferences(t *testing.T) {
+ assert.NoError(t, PrepareTestDatabase())
+
+ // Issue #1 to test against
+ itarget := testCreateIssue(t, 1, 2, "title1", "content1", false)
+
+ // Issue mentioning issue #1
+ title := fmt.Sprintf("title2, mentions #%d", itarget.Index)
+ i := testCreateIssue(t, 1, 2, title, "content2", false)
+ ref := AssertExistsAndLoadBean(t, &Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}).(*Comment)
+ assert.Equal(t, CommentTypeIssueRef, ref.Type)
+ assert.Equal(t, references.XRefActionNone, ref.RefAction)
+
+ d := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
+ i.Title = "title2, no mentions"
+ assert.NoError(t, i.ChangeTitle(d, title))
+
+ ref = AssertExistsAndLoadBean(t, &Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}).(*Comment)
+ assert.Equal(t, CommentTypeIssueRef, ref.Type)
+ assert.Equal(t, references.XRefActionNeutered, ref.RefAction)
+}
+
+func TestXRef_ResolveCrossReferences(t *testing.T) {
+ assert.NoError(t, PrepareTestDatabase())
+
+ d := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
+
+ i1 := testCreateIssue(t, 1, 2, "title1", "content1", false)
+ i2 := testCreateIssue(t, 1, 2, "title2", "content2", false)
+ i3 := testCreateIssue(t, 1, 2, "title3", "content3", false)
+ assert.NoError(t, i3.ChangeStatus(d, true))
+
+ pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index))
+ rp := AssertExistsAndLoadBean(t, &Comment{IssueID: i1.ID, RefIssueID: pr.Issue.ID, RefCommentID: 0}).(*Comment)
+
+ c1 := testCreateComment(t, 1, 2, pr.Issue.ID, fmt.Sprintf("closes #%d", i2.Index))
+ r1 := AssertExistsAndLoadBean(t, &Comment{IssueID: i2.ID, RefIssueID: pr.Issue.ID, RefCommentID: c1.ID}).(*Comment)
+
+ // Must be ignored
+ c2 := testCreateComment(t, 1, 2, pr.Issue.ID, fmt.Sprintf("mentions #%d", i2.Index))
+ AssertExistsAndLoadBean(t, &Comment{IssueID: i2.ID, RefIssueID: pr.Issue.ID, RefCommentID: c2.ID})
+
+ // Must be superseded by c4/r4
+ c3 := testCreateComment(t, 1, 2, pr.Issue.ID, fmt.Sprintf("reopens #%d", i3.Index))
+ AssertExistsAndLoadBean(t, &Comment{IssueID: i3.ID, RefIssueID: pr.Issue.ID, RefCommentID: c3.ID})
+
+ c4 := testCreateComment(t, 1, 2, pr.Issue.ID, fmt.Sprintf("closes #%d", i3.Index))
+ r4 := AssertExistsAndLoadBean(t, &Comment{IssueID: i3.ID, RefIssueID: pr.Issue.ID, RefCommentID: c4.ID}).(*Comment)
+
+ refs, err := pr.ResolveCrossReferences()
+ assert.NoError(t, err)
+ assert.Len(t, refs, 3)
+ assert.Equal(t, rp.ID, refs[0].ID, "bad ref rp: %+v", refs[0])
+ assert.Equal(t, r1.ID, refs[1].ID, "bad ref r1: %+v", refs[1])
+ assert.Equal(t, r4.ID, refs[2].ID, "bad ref r4: %+v", refs[2])
+}
+
+func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispull bool) *Issue {
+ r := AssertExistsAndLoadBean(t, &Repository{ID: repo}).(*Repository)
+ d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User)
+ i := &Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: ispull}
+
+ sess := x.NewSession()
+ defer sess.Close()
+ assert.NoError(t, sess.Begin())
+ _, err := sess.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").Where("repo_id=?", repo).Insert(i)
+ assert.NoError(t, err)
+ i, err = getIssueByID(sess, i.ID)
+ assert.NoError(t, err)
+ assert.NoError(t, i.addCrossReferences(sess, d))
+ assert.NoError(t, sess.Commit())
+ return i
+}
+
+func testCreatePR(t *testing.T, repo, doer int64, title, content string) *PullRequest {
+ r := AssertExistsAndLoadBean(t, &Repository{ID: repo}).(*Repository)
+ d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User)
+ i := &Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: true}
+ pr := &PullRequest{HeadRepoID: repo, BaseRepoID: repo, HeadBranch: "head", BaseBranch: "base"}
+ assert.NoError(t, NewPullRequest(r, i, nil, nil, pr, nil))
+ pr.Issue = i
+ return pr
+}
+
+func testCreateComment(t *testing.T, repo, doer, issue int64, content string) *Comment {
+ d := AssertExistsAndLoadBean(t, &User{ID: doer}).(*User)
+ i := AssertExistsAndLoadBean(t, &Issue{ID: issue}).(*Issue)
+ c := &Comment{Type: CommentTypeComment, PosterID: doer, Poster: d, IssueID: issue, Issue: i, Content: content}
+
+ sess := x.NewSession()
+ defer sess.Close()
+ assert.NoError(t, sess.Begin())
+ _, err := sess.Insert(c)
+ assert.NoError(t, err)
+ assert.NoError(t, c.addCrossReferences(sess, d))
+ assert.NoError(t, sess.Commit())
+ return c
+}
diff --git a/modules/markup/html.go b/modules/markup/html.go
index 1ff7a41cbb0..924d0089a55 100644
--- a/modules/markup/html.go
+++ b/modules/markup/html.go
@@ -659,8 +659,16 @@ func issueIndexPatternProcessor(ctx *postProcessCtx, node *html.Node) {
return
}
- // Decorate action keywords
- keyword := createKeyword(node.Data[ref.ActionLocation.Start:ref.ActionLocation.End])
+ // Decorate action keywords if actionable
+ var keyword *html.Node
+ if references.IsXrefActionable(ref.Action) {
+ keyword = createKeyword(node.Data[ref.ActionLocation.Start:ref.ActionLocation.End])
+ } else {
+ keyword = &html.Node{
+ Type: html.TextNode,
+ Data: node.Data[ref.ActionLocation.Start:ref.ActionLocation.End],
+ }
+ }
spaces := &html.Node{
Type: html.TextNode,
Data: node.Data[ref.ActionLocation.End:ref.RefLocation.Start],
diff --git a/modules/references/references.go b/modules/references/references.go
index af0fe1aa0df..17e9ec2c910 100644
--- a/modules/references/references.go
+++ b/modules/references/references.go
@@ -350,3 +350,8 @@ func findActionKeywords(content []byte, start int) (XRefAction, *RefSpan) {
}
return XRefActionNone, nil
}
+
+// IsXrefActionable returns true if the xref action is actionable (i.e. produces a result when resolved)
+func IsXrefActionable(a XRefAction) bool {
+ return a == XRefActionCloses || a == XRefActionReopens
+}
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 883b9222f53..4d22aa2fd90 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -876,10 +876,13 @@ issues.create_comment = Comment
issues.closed_at = `closed %[2]s`
issues.reopened_at = `reopened %[2]s`
issues.commit_ref_at = `referenced this issue from a commit %[2]s`
-issues.ref_issue_at = `referenced this issue %[1]s`
-issues.ref_pull_at = `referenced this pull request %[1]s`
-issues.ref_issue_ext_at = `referenced this issue from %[1]s %[2]s`
-issues.ref_pull_ext_at = `referenced this pull request from %[1]s %[2]s`
+issues.ref_issue_from = `referenced this issue %[4]s %[2]s`
+issues.ref_pull_from = `referenced this pull request %[4]s %[2]s`
+issues.ref_closing_from = `referenced a pull request %[4]s that will close this issue %[2]s`
+issues.ref_reopening_from = `referenced a pull request %[4]s that will reopen this issue %[2]s`
+issues.ref_closed_from = `closed this issue %[4]s %[2]s`
+issues.ref_reopened_from = `reopened this issue %[4]s %[2]s`
+issues.ref_from = `from %[1]s`
issues.poster = Poster
issues.collaborator = Collaborator
issues.owner = Owner
diff --git a/services/pull/merge.go b/services/pull/merge.go
index 7dc3c07338a..8ae5f029e8f 100644
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -21,8 +21,10 @@ import (
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
+ "code.gitea.io/gitea/modules/references"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"
+ issue_service "code.gitea.io/gitea/services/issue"
"github.com/mcuadros/go-version"
)
@@ -447,6 +449,26 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
notification.NotifyIssueChangeStatus(doer, pr.Issue, true)
+ // Resolve cross references
+ refs, err := pr.ResolveCrossReferences()
+ if err != nil {
+ log.Error("ResolveCrossReferences: %v", err)
+ return nil
+ }
+
+ for _, ref := range refs {
+ if err = ref.LoadIssue(); err != nil {
+ return err
+ }
+ if err = ref.Issue.LoadRepo(); err != nil {
+ return err
+ }
+ close := (ref.RefAction == references.XRefActionCloses)
+ if err = issue_service.ChangeStatus(ref.Issue, doer, close); err != nil {
+ return err
+ }
+ }
+
return nil
}
diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl
index 5a3d4026c6d..86860fe8f44 100644
--- a/templates/repo/issue/view_content/comments.tmpl
+++ b/templates/repo/issue/view_content/comments.tmpl
@@ -88,7 +88,20 @@
{{.Poster.GetDisplayName}} {{$.i18n.Tr "repo.issues.closed_at" .EventTag $createdStr | Safe}}
- {{else if or (eq .Type 3) (eq .Type 5) (eq .Type 6)}}
+ {{else if eq .Type 3 5 6}}
+ {{ $refFrom:= "" }}
+ {{if ne .RefRepoID .Issue.RepoID}}
+ {{ $refFrom = $.i18n.Tr "repo.issues.ref_from" .RefRepo.FullName }}
+ {{end}}
+ {{ $refTr := "repo.issues.ref_issue_from" }}
+ {{if .Issue.IsPull}}
+ {{ $refTr = "repo.issues.ref_pull_from" }}
+ {{else if eq .RefAction 1 }}
+ {{ $refTr = "repo.issues.ref_closing_from" }}
+ {{else if eq .RefAction 2 }}
+ {{ $refTr = "repo.issues.ref_reopening_from" }}
+ {{end}}
+ {{ $createdStr:= TimeSinceUnix .CreatedUnix $.Lang }}