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

@@ -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
 		}
@@ -100,6 +98,7 @@ func (issue *Issue) getCrossReferences(e *xorm.Session, ctx *crossReferencesCont
 	var (
 		refRepo   *Repository
 		refIssue  *Issue
+		refAction references.XRefAction
 		err       error
 	)
 
@@ -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,
+				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
+}

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
+}

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],

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
+}

options/locale/locale_en-US.ini

@@ -876,10 +876,13 @@ issues.create_comment = Comment
 issues.closed_at = `closed <a id="%[1]s" href="#%[1]s">%[2]s</a>`
 issues.reopened_at = `reopened <a id="%[1]s" href="#%[1]s">%[2]s</a>`
 issues.commit_ref_at = `referenced this issue from a commit <a id="%[1]s" href="#%[1]s">%[2]s</a>`
-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 = `<a href="%[3]s">referenced this issue %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
+issues.ref_pull_from = `<a href="%[3]s">referenced this pull request %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
+issues.ref_closing_from = `<a href="%[3]s">referenced a pull request %[4]s that will close this issue</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
+issues.ref_reopening_from = `<a href="%[3]s">referenced a pull request %[4]s that will reopen this issue</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
+issues.ref_closed_from = `<a href="%[3]s">closed this issue %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
+issues.ref_reopened_from = `<a href="%[3]s">reopened this issue %[4]s</a> <a id="%[1]s" href="#%[1]s">%[2]s</a>`
+issues.ref_from = `from %[1]s`
 issues.poster = Poster
 issues.collaborator = Collaborator
 issues.owner = Owner

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
 }
 

templates/repo/issue/view_content/comments.tmpl

@@ -88,7 +88,20 @@
 			</a>
 			<span class="text grey"><a href="{{.Poster.HomeLink}}">{{.Poster.GetDisplayName}}</a> {{$.i18n.Tr "repo.issues.closed_at" .EventTag $createdStr | Safe}}</span>
 		</div>
-	{{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 }}
 		<div class="event" id="{{.HashTag}}">
 			<span class="octicon octicon-bookmark"></span>
 			<a class="ui avatar image" href="{{.Poster.HomeLink}}">
@@ -96,19 +109,7 @@
 			</a>
 			{{if eq .RefAction 3}}<del>{{end}}
 			<span class="text grey"><a href="{{.Poster.HomeLink}}">{{.Poster.GetDisplayName}}</a>
-			{{if (eq .RefRepoID .Issue.RepoID)}}
-				{{if .Issue.IsPull}}
-					<a href="{{.RefCommentHTMLURL}}">{{$.i18n.Tr "repo.issues.ref_pull_at" $createdStr | Safe}}</a>
-				{{else}}
-					<a href="{{.RefCommentHTMLURL}}">{{$.i18n.Tr "repo.issues.ref_issue_at" $createdStr | Safe}}</a>
-				{{end}}
-			{{else}}
-				{{if .Issue.IsPull}}
-					<a href="{{.RefCommentHTMLURL}}">{{$.i18n.Tr "repo.issues.ref_pull_ext_at" .RefRepo.FullName $createdStr | Safe}}</a>
-				{{else}}
-					<a href="{{.RefCommentHTMLURL}}">{{$.i18n.Tr "repo.issues.ref_issue_ext_at" .RefRepo.FullName $createdStr | Safe}}</a>
-				{{end}}
-			{{end}}
+				{{$.i18n.Tr $refTr .EventTag $createdStr .RefCommentHTMLURL $refFrom | Safe}}
 			</span>
 			{{if eq .RefAction 3}}</del>{{end}}