[RFC] Update issues via pull requests

models/action.go

@@ -52,14 +52,43 @@ const (
 	ActionMirrorSyncDelete                        // 20
 )
 
+// KeywordMaskType represents the bitmask of types of keywords found in a message.
+type KeywordMaskType int
+
+// Possible bitmask types for keywords that can be found.
+const (
+	KeywordReference KeywordMaskType = 1 << iota // 1 = 1 << 0
+	KeywordReopen                                // 2 = 1 << 1
+	KeywordClose                                 // 4 = 1 << 2
+)
+
+// IssueKeywordsToFind represents a pairing of a pattern to use to find keywords in message and the keywords bitmask value.
+type IssueKeywordsToFind struct {
+	Pattern     *regexp.Regexp
+	KeywordMask KeywordMaskType
+}
+
 var (
 	// Same as Github. See
 	// https://help.github.com/articles/closing-issues-via-commit-messages
 	issueCloseKeywords  = []string{"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"}
 	issueReopenKeywords = []string{"reopen", "reopens", "reopened"}
 
-	issueCloseKeywordsPat, issueReopenKeywordsPat *regexp.Regexp
-	issueReferenceKeywordsPat                     *regexp.Regexp
+	// populate with details to find keywords for reference, reopen, close
+	issueKeywordsToFind = []*IssueKeywordsToFind{
+		{
+			Pattern:     regexp.MustCompile(issueRefRegexpStr),
+			KeywordMask: KeywordReference,
+		},
+		{
+			Pattern:     regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)),
+			KeywordMask: KeywordReopen,
+		},
+		{
+			Pattern:     regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)),
+			KeywordMask: KeywordClose,
+		},
+	}
 )
 
 const issueRefRegexpStr = `(?:\S+/\S=)?#\d+`
@@ -68,12 +97,6 @@ func assembleKeywordsPattern(words []string) string {
 	return fmt.Sprintf(`(?i)(?:%s) %s`, strings.Join(words, "|"), issueRefRegexpStr)
 }
 
-func init() {
-	issueCloseKeywordsPat = regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords))
-	issueReopenKeywordsPat = regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords))
-	issueReferenceKeywordsPat = regexp.MustCompile(issueRefRegexpStr)
-}
-
 // Action represents user operation type and other information to
 // repository. It implemented interface base.Actioner so that can be
 // used in template render.
@@ -438,74 +461,129 @@ func getIssueFromRef(repo *Repository, ref string) (*Issue, error) {
 	return issue, nil
 }
 
-// UpdateIssuesCommit checks if issues are manipulated by commit message.
-func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) error {
-	// Commits are appended in the reverse order.
-	for i := len(commits) - 1; i >= 0; i-- {
-		c := commits[i]
-
-		refMarked := make(map[int64]bool)
-		for _, ref := range issueReferenceKeywordsPat.FindAllString(c.Message, -1) {
+// findIssueReferencesInString iterates over the keywords to find in a message and accumulates the findings into refs
+func findIssueReferencesInString(message string, repo *Repository) (map[int64]KeywordMaskType, error) {
+	refs := make(map[int64]KeywordMaskType)
+	for _, kwToFind := range issueKeywordsToFind {
+		for _, ref := range kwToFind.Pattern.FindAllString(message, -1) {
 			issue, err := getIssueFromRef(repo, ref)
 			if err != nil {
-				return err
+				return nil, err
 			}
 
-			if issue == nil || refMarked[issue.ID] {
-				continue
+			if issue != nil {
+				refs[issue.ID] |= kwToFind.KeywordMask
 			}
-			refMarked[issue.ID] = true
+		}
+	}
+	return refs, nil
+}
 
-			message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, repo.Link(), c.Sha1, c.Message)
-			if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil {
-				return err
+// changeIssueStatus encapsulates the logic for changing the status of an issue based on what keywords are marked in the keyword mask
+func changeIssueStatus(mask KeywordMaskType, doer *User, issue *Issue) error {
+	// take no action if both KeywordClose and KeywordOpen are set
+	switch mask {
+	case KeywordClose:
+		if err := issue.ChangeStatus(doer, issue.Repo, true); err != nil {
+			// Don't return the error when dependencies are still open as this would cause a push, merge, etc. to fail
+			if IsErrDependenciesLeft(err) {
+				return nil
 			}
+			return err
 		}
+	case KeywordReopen:
+		if err := issue.ChangeStatus(doer, issue.Repo, false); err != nil {
+			return err
+		}
+	}
+	return nil
+}
 
-		refMarked = make(map[int64]bool)
-		// FIXME: can merge this one and next one to a common function.
-		for _, ref := range issueCloseKeywordsPat.FindAllString(c.Message, -1) {
-			issue, err := getIssueFromRef(repo, ref)
+// UpdateIssuesComment checks if issues are manipulated by a regular comment, code comment, or review comment (also issue/pull request title and description)
+func UpdateIssuesComment(doer *User, repo *Repository, commentIssue *Issue, comment *Comment, canOpenClose bool) error {
+	var refString string
+	if comment != nil {
+		if comment.Type != CommentTypeComment && comment.Type != CommentTypeCode && comment.Type != CommentTypeReview {
+			return nil
+		}
+
+		refString = comment.Content
+	} else {
+		refString = commentIssue.Title + ": " + commentIssue.Content
+	}
+
+	refs, err := findIssueReferencesInString(refString, repo)
+	if err != nil {
+		return err
+	}
+
+	for id, mask := range refs {
+		issue, err := GetIssueByID(id)
+		if err != nil {
+			return err
+		}
+		if issue == nil || issue.ID == commentIssue.ID {
+			continue
+		}
+
+		if (mask & KeywordReference) == KeywordReference {
+			if comment != nil && comment.Type == CommentTypeComment {
+				err = CreateCommentRefComment(doer, issue, commentIssue.ID, comment.ID)
+			} else if comment != nil && comment.Type == CommentTypeCode {
+				err = CreateCodeRefComment(doer, issue, commentIssue.ID, comment.ID)
+			} else if comment != nil && comment.Type == CommentTypeReview {
+				err = CreateReviewRefComment(doer, issue, commentIssue.ID, comment.ID)
+			} else if commentIssue.IsPull {
+				err = CreatePullRefComment(doer, issue, commentIssue.ID)
+			} else {
+				err = CreateIssueRefComment(doer, issue, commentIssue.ID)
+			}
 			if err != nil {
 				return err
 			}
+		}
 
-			if issue == nil || refMarked[issue.ID] {
-				continue
+		// only change issue status if this is a pull request in the issue's repo
+		if canOpenClose && comment == nil && commentIssue.IsPull && repo.ID == issue.RepoID {
+			if err = changeIssueStatus(mask&(KeywordReopen|KeywordClose), doer, issue); err != nil {
+				return err
 			}
-			refMarked[issue.ID] = true
+		}
+	}
+	return nil
+}
 
-			if issue.RepoID != repo.ID || issue.IsClosed {
-				continue
-			}
+// UpdateIssuesCommit checks if issues are manipulated by commit message.
+func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, commitsAreMerged bool) error {
+	// Commits are appended in the reverse order.
+	for i := len(commits) - 1; i >= 0; i-- {
+		c := commits[i]
 
-			if err = issue.ChangeStatus(doer, repo, true); err != nil {
-				// Don't return an error when dependencies are open as this would let the push fail
-				if IsErrDependenciesLeft(err) {
-					return nil
-				}
-				return err
-			}
+		refs, err := findIssueReferencesInString(c.Message, repo)
+		if err != nil {
+			return err
 		}
 
-		// It is conflict to have close and reopen at same time, so refsMarked doesn't need to reinit here.
-		for _, ref := range issueReopenKeywordsPat.FindAllString(c.Message, -1) {
-			issue, err := getIssueFromRef(repo, ref)
+		for id, mask := range refs {
+			issue, err := GetIssueByID(id)
 			if err != nil {
 				return err
 			}
-
-			if issue == nil || refMarked[issue.ID] {
+			if issue == nil {
 				continue
 			}
-			refMarked[issue.ID] = true
 
-			if issue.RepoID != repo.ID || !issue.IsClosed {
-				continue
+			if (mask & KeywordReference) == KeywordReference {
+				if err = CreateCommitRefComment(doer, issue, repo.ID, c.Sha1); err != nil {
+					return err
+				}
 			}
 
-			if err = issue.ChangeStatus(doer, repo, false); err != nil {
-				return err
+			// only change issue status if the commit is merged to the issue's repo
+			if commitsAreMerged && repo.ID == issue.RepoID {
+				if err = changeIssueStatus(mask&(KeywordReopen|KeywordClose), doer, issue); err != nil {
+					return err
+				}
 			}
 		}
 	}
@@ -569,8 +647,8 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 			opts.Commits.CompareURL = repo.ComposeCompareURL(opts.OldCommitID, opts.NewCommitID)
 		}
 
-		if err = UpdateIssuesCommit(pusher, repo, opts.Commits.Commits); err != nil {
-			log.Error(4, "updateIssuesCommit: %v", err)
+		if err = UpdateIssuesCommit(pusher, repo, opts.Commits.Commits, true); err != nil {
+			log.Error(4, "UpdateIssuesCommit: %v", err)
 		}
 	}
 
@@ -724,21 +802,91 @@ func TransferRepoAction(doer, oldOwner *User, repo *Repository) error {
 	return transferRepoAction(x, doer, oldOwner, repo)
 }
 
-func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue) error {
-	return notifyWatchers(e, &Action{
+// MergePullRequestAction adds new action for merging pull request (including manually merged pull requests).
+func MergePullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error {
+	if commits != nil {
+		if err := UpdateIssuesCommit(doer, repo, commits.Commits, true); err != nil {
+			log.Error(4, "UpdateIssuesCommit: %v", err)
+		}
+	}
+
+	if err := UpdateIssuesComment(doer, repo, pull, nil, true); err != nil {
+		log.Error(4, "UpdateIssuesComment: %v", err)
+	}
+
+	if err := notifyWatchers(x, &Action{
 		ActUserID: doer.ID,
 		ActUser:   doer,
 		OpType:    ActionMergePullRequest,
-		Content:   fmt.Sprintf("%d|%s", issue.Index, issue.Title),
+		Content:   fmt.Sprintf("%d|%s", pull.Index, pull.Title),
 		RepoID:    repo.ID,
 		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
-	})
+	}); err != nil {
+		return fmt.Errorf("notifyWatchers: %v", err)
+	}
+
+	return nil
 }
 
-// MergePullRequestAction adds new action for merging pull request.
-func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue) error {
-	return mergePullRequestAction(x, actUser, repo, pull)
+// NewPullRequestAction adds new action for creating a new pull request.
+func NewPullRequestAction(doer *User, repo *Repository, pull *Issue, commits *PushCommits) error {
+	if err := UpdateIssuesCommit(doer, repo, commits.Commits, false); err != nil {
+		log.Error(4, "UpdateIssuesCommit: %v", err)
+	}
+
+	if err := UpdateIssuesComment(doer, repo, pull, nil, false); err != nil {
+		log.Error(4, "UpdateIssuesComment: %v", err)
+	}
+
+	if err := NotifyWatchers(&Action{
+		ActUserID: doer.ID,
+		ActUser:   doer,
+		OpType:    ActionCreatePullRequest,
+		Content:   fmt.Sprintf("%d|%s", pull.Index, pull.Title),
+		RepoID:    repo.ID,
+		Repo:      repo,
+		IsPrivate: repo.IsPrivate,
+	}); err != nil {
+		log.Error(4, "NotifyWatchers: %v", err)
+	} else if err := pull.MailParticipants(); err != nil {
+		log.Error(4, "MailParticipants: %v", err)
+	}
+
+	return nil
+}
+
+// CommitPullRequestAction adds new action for pushed commits tracked by a pull request.
+func CommitPullRequestAction(doer *User, repo *Repository, commits *PushCommits) error {
+	if err := UpdateIssuesCommit(doer, repo, commits.Commits, false); err != nil {
+		log.Error(4, "UpdateIssuesCommit: %v", err)
+	}
+
+	// no action added
+
+	return nil
+}
+
+// CreateOrUpdateCommentAction adds new action when creating or updating a comment.
+func CreateOrUpdateCommentAction(doer *User, repo *Repository, issue *Issue, comment *Comment) error {
+	if err := UpdateIssuesComment(doer, repo, issue, comment, false); err != nil {
+		log.Error(4, "UpdateIssuesComment: %v", err)
+	}
+
+	// no action added
+
+	return nil
+}
+
+// CreateOrUpdateIssueAction adds new action when creating a new issue or pull request.
+func CreateOrUpdateIssueAction(doer *User, repo *Repository, issue *Issue) error {
+	if err := UpdateIssuesComment(doer, repo, issue, nil, false); err != nil {
+		log.Error(4, "UpdateIssuesComment: %v", err)
+	}
+
+	// no action added
+
+	return nil
 }
 
 func mirrorSyncAction(e Engine, opType ActionType, repo *Repository, refName string, data []byte) error {