Nulo/forgejo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Refactor comment (#9330)

Browse source 
* Refactor comment

* fix test

* improve code
This commit is contained in:
Lunny Xiao 2019-12-16 05:57:34 +08:00 committed by techknowlogick
parent c6b3c5bcef
commit 67b316a954
16 changed files with 144 additions and 45 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
models/error.go
View file

@ -1121,6 +1121,38 @@ func (err ErrNewIssueInsert) Error() string {
return err.OriginalError.Error()
}
// ErrIssueWasClosed is used when close a closed issue
type ErrIssueWasClosed struct {
ID int64
Index int64
}
// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed.
func IsErrIssueWasClosed(err error) bool {
_, ok := err.(ErrIssueWasClosed)
return ok
}
func (err ErrIssueWasClosed) Error() string {
return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
}
// ErrPullWasClosed is used close a closed pull request
type ErrPullWasClosed struct {
ID int64
Index int64
}
// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed.
func IsErrPullWasClosed(err error) bool {
_, ok := err.(ErrPullWasClosed)
return ok
}
func (err ErrPullWasClosed) Error() string {
return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index)
}
// ErrForbiddenIssueReaction is used when a forbidden reaction was try to created
type ErrForbiddenIssueReaction struct {
Reaction string

57
models/issue.go
View file

@ -600,16 +600,23 @@ func updateIssueCols(e Engine, issue *Issue, cols ...string) error {
return nil
}
func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (err error) {
func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (*Comment, error) {
// Reload the issue
currentIssue, err := getIssueByID(e, issue.ID)
if err != nil {
return err
return nil, err
}
// Nothing should be performed if current status is same as target status
if currentIssue.IsClosed == isClosed {
return nil
if !issue.IsPull {
return nil, ErrIssueWasClosed{
ID: issue.ID,
}
}
return nil, ErrPullWasClosed{
ID: issue.ID,
}
}
// Check for open dependencies
@ -617,11 +624,11 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er
// only check if dependencies are enabled and we're about to close an issue, otherwise reopening an issue would fail when there are unsatisfied dependencies
noDeps, err := issueNoDependenciesLeft(e, issue)
if err != nil {
return err
return nil, err
}
if !noDeps {
return ErrDependenciesLeft{issue.ID}
return nil, ErrDependenciesLeft{issue.ID}
}
}
@ -633,22 +640,22 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er
}
if err = updateIssueCols(e, issue, "is_closed", "closed_unix"); err != nil {
return err
return nil, err
}
// Update issue count of labels
if err = issue.getLabels(e); err != nil {
return err
return nil, err
}
for idx := range issue.Labels {
if err = updateLabel(e, issue.Labels[idx]); err != nil {
return err
return nil, err
}
}
// Update issue count of milestone
if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {
return err
return nil, err
}
// New action comment
@ -657,43 +664,39 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er
cmtType = CommentTypeReopen
}
var opts = &CreateCommentOptions{
return createCommentWithNoAction(e, &CreateCommentOptions{
Type: cmtType,
Doer: doer,
Repo: issue.Repo,
Issue: issue,
}
comment, err := createCommentWithNoAction(e, opts)
if err != nil {
return err
}
return sendCreateCommentAction(e, opts, comment)
})
}
// ChangeStatus changes issue status to open or closed.
func (issue *Issue) ChangeStatus(doer *User, isClosed bool) (err error) {
func (issue *Issue) ChangeStatus(doer *User, isClosed bool) (*Comment, error) {
sess := x.NewSession()
defer sess.Close()
if err = sess.Begin(); err != nil {
return err
if err := sess.Begin(); err != nil {
return nil, err
}
if err = issue.loadRepo(sess); err != nil {
return err
if err := issue.loadRepo(sess); err != nil {
return nil, err
}
if err = issue.loadPoster(sess); err != nil {
return err
if err := issue.loadPoster(sess); err != nil {
return nil, err
}
if err = issue.changeStatus(sess, doer, isClosed); err != nil {
return err
comment, err := issue.changeStatus(sess, doer, isClosed)
if err != nil {
return nil, err
}
if err = sess.Commit(); err != nil {
return fmt.Errorf("Commit: %v", err)
return nil, fmt.Errorf("Commit: %v", err)
}
return nil
return comment, nil
}
// ChangeTitle changes the title of this issue, as the given user.

4
models/issue_comment.go
View file

@ -750,10 +750,6 @@ func CreateComment(opts *CreateCommentOptions) (comment *Comment, err error) {
return nil, err
}
if err = sendCreateCommentAction(sess, opts, comment); err != nil {
return nil, err
}
if err = sess.Commit(); err != nil {
return nil, err
}

2
models/issue_dependency_test.go
View file

@ -45,7 +45,7 @@ func TestCreateIssueDependency(t *testing.T) {
assert.False(t, left)
// Close #2 and check again
err = issue2.ChangeStatus(user1, true)
_, err = issue2.ChangeStatus(user1, true)
assert.NoError(t, err)
left, err = IssueNoDependenciesLeft(issue1)

3
models/issue_xref_test.go
View file

@ -94,7 +94,8 @@ func TestXRef_ResolveCrossReferences(t *testing.T) {
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))
_, err := i3.ChangeStatus(d, true)
assert.NoError(t, err)
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)

2
models/pull.go
View file

@ -460,7 +460,7 @@ func (pr *PullRequest) SetMerged() (err error) {
return err
}
if err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
return fmt.Errorf("Issue.changeStatus: %v", err)
}
if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {

54
modules/notification/action/action.go
View file

@ -53,6 +53,60 @@ func (a *actionNotifier) NotifyNewIssue(issue *models.Issue) {
}
}
// NotifyIssueChangeStatus notifies close or reopen issue to notifiers
func (a *actionNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, closeOrReopen bool) {
// Compose comment action, could be plain comment, close or reopen issue/pull request.
// This object will be used to notify watchers in the end of function.
act := &models.Action{
ActUserID: doer.ID,
ActUser: doer,
Content: fmt.Sprintf("%d|%s", issue.Index, ""),
RepoID: issue.Repo.ID,
Repo: issue.Repo,
Comment: actionComment,
CommentID: actionComment.ID,
IsPrivate: issue.Repo.IsPrivate,
}
// Check comment type.
if closeOrReopen {
act.OpType = models.ActionCloseIssue
if issue.IsPull {
act.OpType = models.ActionClosePullRequest
}
} else {
act.OpType = models.ActionReopenIssue
if issue.IsPull {
act.OpType = models.ActionReopenPullRequest
}
}
// Notify watchers for whatever action comes in, ignore if no action type.
if err := models.NotifyWatchers(act); err != nil {
log.Error("NotifyWatchers: %v", err)
}
}
// NotifyCreateIssueComment notifies comment on an issue to notifiers
func (a *actionNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
issue *models.Issue, comment *models.Comment) {
act := &models.Action{
OpType: models.ActionCommentIssue,
ActUserID: doer.ID,
ActUser: doer,
Content: fmt.Sprintf("%d|%s", issue.Index, comment.Content),
RepoID: issue.Repo.ID,
Repo: issue.Repo,
Comment: comment,
CommentID: comment.ID,
IsPrivate: issue.Repo.IsPrivate,
}
// Notify watchers for whatever action comes in, ignore if no action type.
if err := models.NotifyWatchers(act); err != nil {
log.Error("NotifyWatchers: %v", err)
}
}
func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest) {
if err := pull.LoadIssue(); err != nil {
log.Error("pull.LoadIssue: %v", err)

2
modules/notification/base/notifier.go
View file

@ -21,7 +21,7 @@ type Notifier interface {
NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string)
NotifyNewIssue(*models.Issue)
NotifyIssueChangeStatus(*models.User, *models.Issue, bool)
NotifyIssueChangeStatus(*models.User, *models.Issue, *models.Comment, bool)
NotifyIssueChangeMilestone(doer *models.User, issue *models.Issue, oldMilestoneID int64)
NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment)
NotifyIssueChangeContent(doer *models.User, issue *models.Issue, oldContent string)

2
modules/notification/base/null.go
View file

@ -31,7 +31,7 @@ func (*NullNotifier) NotifyNewIssue(issue *models.Issue) {
}
// NotifyIssueChangeStatus places a place holder function
func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
}
// NotifyNewPullRequest places a place holder function

2
modules/notification/mail/mail.go
View file

@ -51,7 +51,7 @@ func (m *mailNotifier) NotifyNewIssue(issue *models.Issue) {
}
}
func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
var actionType models.ActionType
if issue.IsPull {
if isClosed {

4
modules/notification/notification.go
View file

@ -53,9 +53,9 @@ func NotifyNewIssue(issue *models.Issue) {
}
// NotifyIssueChangeStatus notifies close or reopen issue to notifiers
func NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, closeOrReopen bool) {
func NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, closeOrReopen bool) {
for _, notifier := range notifiers {
notifier.NotifyIssueChangeStatus(doer, issue, closeOrReopen)
notifier.NotifyIssueChangeStatus(doer, issue, actionComment, closeOrReopen)
}
}

2
modules/notification/ui/ui.go
View file

@ -62,7 +62,7 @@ func (ns *notificationService) NotifyNewIssue(issue *models.Issue) {
}
}
func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
ns.issueQueue <- issueNotificationOpts{
issueID: issue.ID,
notificationAuthorID: doer.ID,

2
modules/notification/webhook/webhook.go
View file

@ -211,7 +211,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model
}
}
func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
mode, _ := models.AccessLevel(issue.Poster, issue.Repo)
var err error
if issue.IsPull {

7
modules/repofiles/action.go
View file

@ -31,7 +31,7 @@ func getIssueFromRef(repo *models.Repository, index int64) (*models.Issue, error
return issue, nil
}
func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, status bool) error {
func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, closed bool) error {
stopTimerIfAvailable := func(doer *models.User, issue *models.Issue) error {
if models.StopwatchExists(doer.ID, issue.ID) {
@ -44,7 +44,8 @@ func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *model
}
issue.Repo = repo
if err := issue.ChangeStatus(doer, status); err != nil {
comment, err := issue.ChangeStatus(doer, closed)
if err != nil {
// Don't return an error when dependencies are open as this would let the push fail
if models.IsErrDependenciesLeft(err) {
return stopTimerIfAvailable(doer, issue)
@ -52,6 +53,8 @@ func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *model
return err
}
notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
return stopTimerIfAvailable(doer, issue)
}

4
services/issue/status.go
View file

@ -11,11 +11,11 @@ import (
// ChangeStatus changes issue status to open or closed.
func ChangeStatus(issue *models.Issue, doer *models.User, isClosed bool) (err error) {
err = issue.ChangeStatus(doer, isClosed)
comment, err := issue.ChangeStatus(doer, isClosed)
if err != nil {
return
}
notification.NotifyIssueChangeStatus(doer, issue, isClosed)
notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed)
return nil
}

10
services/pull/check.go
View file

@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/graceful"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/sync"
"code.gitea.io/gitea/modules/timeutil"
@ -145,6 +146,15 @@ func manuallyMerged(pr *models.PullRequest) bool {
log.Error("PullRequest[%d].setMerged : %v", pr.ID, err)
return false
}
baseGitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
if err != nil {
log.Error("OpenRepository[%s] : %v", pr.BaseRepo.RepoPath(), err)
return false
}
notification.NotifyMergePullRequest(pr, merger, baseGitRepo)
log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String())
return true
}