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: 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 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 26 deletions(-) create mode 100644 models/issue_xref_test.go (limited to 'models') diff --git a/models/issue_xref.go b/models/issue_xref.go index 787a0e5119..41937dc675 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 0000000000..4fc6011d78 --- /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 +} -- cgit v1.2.3