From dad67cae5455f346f122ab26ec50ac4e029cacf4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 14 Nov 2019 10:57:36 +0800 Subject: [PATCH] Refactor pull request review (#8954) * refactor submit review * remove unnecessary code * remove unused comment * fix lint * remove duplicated actions * remove duplicated actions * fix typo * fix comment content --- models/issue_comment.go | 7 +- models/repo_watch.go | 15 +++ models/review.go | 134 +++++++++++++----------- models/review_test.go | 8 -- modules/notification/action/action.go | 49 +++++++++ routers/repo/pull_review.go | 128 ++++------------------- services/comments/comments.go | 61 ----------- services/pull/review.go | 140 +++++++++++++++++++++++--- 8 files changed, 291 insertions(+), 251 deletions(-) diff --git a/models/issue_comment.go b/models/issue_comment.go index 63f5f6b778..c7e9e7cdfa 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -539,8 +539,10 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err return nil, err } - if err = sendCreateCommentAction(e, opts, comment); err != nil { - return nil, err + if !opts.NoAction { + if err = sendCreateCommentAction(e, opts, comment); err != nil { + return nil, err + } } if err = comment.addCrossReferences(e, opts.Doer); err != nil { @@ -816,6 +818,7 @@ type CreateCommentOptions struct { RefCommentID int64 RefAction references.XRefAction RefIsPull bool + NoAction bool } // CreateComment creates comment of issue or commit. diff --git a/models/repo_watch.go b/models/repo_watch.go index cb864fb46d..2de4f8b320 100644 --- a/models/repo_watch.go +++ b/models/repo_watch.go @@ -216,6 +216,21 @@ func NotifyWatchers(act *Action) error { return notifyWatchers(x, act) } +// NotifyWatchersActions creates batch of actions for every watcher. +func NotifyWatchersActions(acts []*Action) error { + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return err + } + for _, act := range acts { + if err := notifyWatchers(sess, act); err != nil { + return err + } + } + return sess.Commit() +} + func watchIfAuto(e Engine, userID, repoID int64, isWrite bool) error { if !isWrite || !setting.Service.AutoWatchOnChanges { return nil diff --git a/models/review.go b/models/review.go index 89a26d6fdb..441bb40fbb 100644 --- a/models/review.go +++ b/models/review.go @@ -5,14 +5,12 @@ package models import ( - "fmt" + "strings" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" "xorm.io/builder" "xorm.io/core" - "xorm.io/xorm" ) // ReviewType defines the sort of feedback a review gives @@ -86,6 +84,11 @@ func (r *Review) loadReviewer(e Engine) (err error) { return } +// LoadReviewer loads reviewer +func (r *Review) LoadReviewer() error { + return r.loadReviewer(x) +} + func (r *Review) loadAttributes(e Engine) (err error) { if err = r.loadReviewer(e); err != nil { return @@ -101,54 +104,6 @@ func (r *Review) LoadAttributes() error { return r.loadAttributes(x) } -// Publish will send notifications / actions to participants for all code comments; parts are concurrent -func (r *Review) Publish() error { - return r.publish(x) -} - -func (r *Review) publish(e *xorm.Engine) error { - if r.Type == ReviewTypePending || r.Type == ReviewTypeUnknown { - return fmt.Errorf("review cannot be published if type is pending or unknown") - } - if r.Issue == nil { - if err := r.loadIssue(e); err != nil { - return err - } - } - if err := r.Issue.loadRepo(e); err != nil { - return err - } - if len(r.CodeComments) == 0 { - if err := r.loadCodeComments(e); err != nil { - return err - } - } - for _, lines := range r.CodeComments { - for _, comments := range lines { - for _, comment := range comments { - go func(en *xorm.Engine, review *Review, comm *Comment) { - sess := en.NewSession() - defer sess.Close() - opts := &CreateCommentOptions{ - Doer: comm.Poster, - Issue: review.Issue, - Repo: review.Issue.Repo, - Type: comm.Type, - Content: comm.Content, - } - if err := updateCommentInfos(sess, opts, comm); err != nil { - log.Warn("updateCommentInfos: %v", err) - } - if err := sendCreateCommentAction(sess, opts, comm); err != nil { - log.Warn("sendCreateCommentAction: %v", err) - } - }(e, r, comment) - } - } - } - return nil -} - func getReviewByID(e Engine, id int64) (*Review, error) { review := new(Review) if has, err := e.ID(id).Get(review); err != nil { @@ -271,12 +226,79 @@ func GetCurrentReview(reviewer *User, issue *Issue) (*Review, error) { return getCurrentReview(x, reviewer, issue) } -// UpdateReview will update all cols of the given review in db -func UpdateReview(r *Review) error { - if _, err := x.ID(r.ID).AllCols().Update(r); err != nil { - return err +// ContentEmptyErr represents an content empty error +type ContentEmptyErr struct { +} + +func (ContentEmptyErr) Error() string { + return "Review content is empty" +} + +// IsContentEmptyErr returns true if err is a ContentEmptyErr +func IsContentEmptyErr(err error) bool { + _, ok := err.(ContentEmptyErr) + return ok +} + +// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist +func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content string) (*Review, *Comment, error) { + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return nil, nil, err } - return nil + + review, err := getCurrentReview(sess, doer, issue) + if err != nil { + if !IsErrReviewNotExist(err) { + return nil, nil, err + } + + if len(strings.TrimSpace(content)) == 0 { + return nil, nil, ContentEmptyErr{} + } + + // No current review. Create a new one! + review, err = createReview(sess, CreateReviewOptions{ + Type: reviewType, + Issue: issue, + Reviewer: doer, + Content: content, + }) + if err != nil { + return nil, nil, err + } + } else { + if err := review.loadCodeComments(sess); err != nil { + return nil, nil, err + } + if len(review.CodeComments) == 0 && len(strings.TrimSpace(content)) == 0 { + return nil, nil, ContentEmptyErr{} + } + + review.Issue = issue + review.Content = content + review.Type = reviewType + if _, err := sess.ID(review.ID).Cols("content, type").Update(review); err != nil { + return nil, nil, err + } + } + + comm, err := createComment(sess, &CreateCommentOptions{ + Type: CommentTypeReview, + Doer: doer, + Content: review.Content, + Issue: issue, + Repo: issue.Repo, + ReviewID: review.ID, + NoAction: true, + }) + if err != nil || comm == nil { + return nil, nil, err + } + + comm.Review = review + return review, comm, sess.Commit() } // PullReviewersWithType represents the type used to display a review overview diff --git a/models/review_test.go b/models/review_test.go index f8e8086dca..3e7563b434 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -98,14 +98,6 @@ func TestCreateReview(t *testing.T) { AssertExistsAndLoadBean(t, &Review{Content: "New Review"}) } -func TestUpdateReview(t *testing.T) { - assert.NoError(t, PrepareTestDatabase()) - review := AssertExistsAndLoadBean(t, &Review{ID: 1}).(*Review) - review.Content = "Updated Review" - assert.NoError(t, UpdateReview(review)) - AssertExistsAndLoadBean(t, &Review{ID: 1, Content: "Updated Review"}) -} - func TestGetReviewersByPullID(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go index d481bd8c4d..36035b864f 100644 --- a/modules/notification/action/action.go +++ b/modules/notification/action/action.go @@ -6,6 +6,7 @@ package action import ( "fmt" + "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" @@ -117,3 +118,51 @@ func (a *actionNotifier) NotifyForkRepository(doer *models.User, oldRepo, repo * log.Error("notify watchers '%d/%d': %v", doer.ID, repo.ID, err) } } + +func (a *actionNotifier) NotifyPullRequestReview(pr *models.PullRequest, review *models.Review, comment *models.Comment) { + if err := review.LoadReviewer(); err != nil { + log.Error("LoadReviewer '%d/%d': %v", review.ID, review.ReviewerID, err) + return + } + if err := review.LoadCodeComments(); err != nil { + log.Error("LoadCodeComments '%d/%d': %v", review.Reviewer.ID, review.ID, err) + return + } + + var actions = make([]*models.Action, 0, 10) + for _, lines := range review.CodeComments { + for _, comments := range lines { + for _, comm := range comments { + actions = append(actions, &models.Action{ + ActUserID: review.Reviewer.ID, + ActUser: review.Reviewer, + Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]), + OpType: models.ActionCommentIssue, + RepoID: review.Issue.RepoID, + Repo: review.Issue.Repo, + IsPrivate: review.Issue.Repo.IsPrivate, + Comment: comm, + CommentID: comm.ID, + }) + } + } + } + + if strings.TrimSpace(comment.Content) != "" { + actions = append(actions, &models.Action{ + ActUserID: review.Reviewer.ID, + ActUser: review.Reviewer, + Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comment.Content, "\n")[0]), + OpType: models.ActionCommentIssue, + RepoID: review.Issue.RepoID, + Repo: review.Issue.Repo, + IsPrivate: review.Issue.Repo.IsPrivate, + Comment: comment, + CommentID: comment.ID, + }) + } + + if err := models.NotifyWatchersActions(actions); err != nil { + log.Error("notify watchers '%d/%d': %v", review.Reviewer.ID, review.Issue.RepoID, err) + } +} diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index 7d030988fd..b596d2578b 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -11,8 +11,6 @@ import ( "code.gitea.io/gitea/modules/auth" "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/notification" - comment_service "code.gitea.io/gitea/services/comments" pull_service "code.gitea.io/gitea/services/pull" ) @@ -31,64 +29,33 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) { ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) return } - var comment *models.Comment - defer func() { - if comment != nil { - ctx.Redirect(comment.HTMLURL()) - } else { - ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) - } - }() + signedLine := form.Line if form.Side == "previous" { signedLine *= -1 } - review := new(models.Review) - if form.IsReview { - var err error - // Check if the user has already a pending review for this issue - if review, err = models.GetCurrentReview(ctx.User, issue); err != nil { - if !models.IsErrReviewNotExist(err) { - ctx.ServerError("CreateCodeComment", err) - return - } - // No pending review exists - // Create a new pending review for this issue & user - if review, err = pull_service.CreateReview(models.CreateReviewOptions{ - Type: models.ReviewTypePending, - Reviewer: ctx.User, - Issue: issue, - }); err != nil { - ctx.ServerError("CreateCodeComment", err) - return - } - - } - } - if review.ID == 0 { - review.ID = form.Reply - } - //FIXME check if line, commit and treepath exist - comment, err := comment_service.CreateCodeComment( + comment, err := pull_service.CreateCodeComment( ctx.User, - issue.Repo, issue, + signedLine, form.Content, form.TreePath, - signedLine, - review.ID, + form.IsReview, + form.Reply, ) if err != nil { ctx.ServerError("CreateCodeComment", err) return } - // Send no notification if comment is pending - if !form.IsReview || form.Reply != 0 { - notification.NotifyCreateIssueComment(ctx.User, issue.Repo, issue, comment) - } log.Trace("Comment created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID) + + if comment != nil { + ctx.Redirect(comment.HTMLURL()) + } else { + ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) + } } // SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist @@ -105,23 +72,17 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) return } - var review *models.Review - var err error reviewType := form.ReviewType() - switch reviewType { case models.ReviewTypeUnknown: - ctx.ServerError("GetCurrentReview", fmt.Errorf("unknown ReviewType: %s", form.Type)) + ctx.ServerError("ReviewType", fmt.Errorf("unknown ReviewType: %s", form.Type)) return // can not approve/reject your own PR case models.ReviewTypeApprove, models.ReviewTypeReject: - if issue.Poster.ID == ctx.User.ID { - var translated string - if reviewType == models.ReviewTypeApprove { translated = ctx.Tr("repo.issues.review.self.approval") } else { @@ -134,69 +95,16 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { } } - review, err = models.GetCurrentReview(ctx.User, issue) - if err == nil { - review.Issue = issue - if errl := review.LoadCodeComments(); errl != nil { - ctx.ServerError("LoadCodeComments", err) - return - } - } - - if ((err == nil && len(review.CodeComments) == 0) || - (err != nil && models.IsErrReviewNotExist(err))) && - form.HasEmptyContent() { - ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) - return - } - + _, comm, err := pull_service.SubmitReview(ctx.User, issue, reviewType, form.Content) if err != nil { - if !models.IsErrReviewNotExist(err) { - ctx.ServerError("GetCurrentReview", err) - return + if models.IsContentEmptyErr(err) { + ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty")) + ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) + } else { + ctx.ServerError("SubmitReview", err) } - // No current review. Create a new one! - if review, err = pull_service.CreateReview(models.CreateReviewOptions{ - Type: reviewType, - Issue: issue, - Reviewer: ctx.User, - Content: form.Content, - }); err != nil { - ctx.ServerError("CreateReview", err) - return - } - } else { - review.Content = form.Content - review.Type = reviewType - if err = pull_service.UpdateReview(review); err != nil { - ctx.ServerError("UpdateReview", err) - return - } - } - comm, err := models.CreateComment(&models.CreateCommentOptions{ - Type: models.CommentTypeReview, - Doer: ctx.User, - Content: review.Content, - Issue: issue, - Repo: issue.Repo, - ReviewID: review.ID, - }) - if err != nil || comm == nil { - ctx.ServerError("CreateComment", err) return } - if err = review.Publish(); err != nil { - ctx.ServerError("Publish", err) - return - } - - pr, err := issue.GetPullRequest() - if err != nil { - ctx.ServerError("GetPullRequest", err) - return - } - notification.NotifyPullRequestReview(pr, review, comm) ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag())) } diff --git a/services/comments/comments.go b/services/comments/comments.go index ba40bf582d..ed479e7110 100644 --- a/services/comments/comments.go +++ b/services/comments/comments.go @@ -5,15 +5,8 @@ package comments import ( - "bytes" - "fmt" - "strings" - "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/services/gitdiff" ) // CreateIssueComment creates a plain issue comment. @@ -35,60 +28,6 @@ func CreateIssueComment(doer *models.User, repo *models.Repository, issue *model return comment, nil } -// CreateCodeComment creates a plain code comment at the specified line / path -func CreateCodeComment(doer *models.User, repo *models.Repository, issue *models.Issue, content, treePath string, line, reviewID int64) (*models.Comment, error) { - var commitID, patch string - pr, err := models.GetPullRequestByIssueID(issue.ID) - if err != nil { - return nil, fmt.Errorf("GetPullRequestByIssueID: %v", err) - } - if err := pr.GetBaseRepo(); err != nil { - return nil, fmt.Errorf("GetHeadRepo: %v", err) - } - gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) - if err != nil { - return nil, fmt.Errorf("OpenRepository: %v", err) - } - defer gitRepo.Close() - - // FIXME validate treePath - // Get latest commit referencing the commented line - // No need for get commit for base branch changes - if line > 0 { - commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(line)) - if err == nil { - commitID = commit.ID.String() - } else if !strings.Contains(err.Error(), "exit status 128 - fatal: no such path") { - return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) - } - } - - // Only fetch diff if comment is review comment - if reviewID != 0 { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) - if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err) - } - patchBuf := new(bytes.Buffer) - if err := gitdiff.GetRawDiffForFile(gitRepo.Path, pr.MergeBase, headCommitID, gitdiff.RawDiffNormal, treePath, patchBuf); err != nil { - return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, headCommitID, treePath) - } - patch = gitdiff.CutDiffAroundLine(patchBuf, int64((&models.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) - } - return models.CreateComment(&models.CreateCommentOptions{ - Type: models.CommentTypeCode, - Doer: doer, - Repo: repo, - Issue: issue, - Content: content, - LineNum: line, - TreePath: treePath, - CommitSHA: commitID, - ReviewID: reviewID, - Patch: patch, - }) -} - // UpdateComment updates information of comment. func UpdateComment(c *models.Comment, doer *models.User, oldContent string) error { if err := models.UpdateComment(c, doer); err != nil { diff --git a/services/pull/review.go b/services/pull/review.go index 294297f956..880647c6b5 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -6,32 +6,144 @@ package pull import ( + "bytes" + "fmt" + "strings" + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/notification" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/services/gitdiff" ) -// CreateReview creates a new review based on opts -func CreateReview(opts models.CreateReviewOptions) (*models.Review, error) { - review, err := models.CreateReview(opts) +// CreateCodeComment creates a comment on the code line +func CreateCodeComment(doer *models.User, issue *models.Issue, line int64, content string, treePath string, isReview bool, replyReviewID int64) (*models.Comment, error) { + // It's not a review, maybe a reply to a review comment or a single comment. + if !isReview { + if err := issue.LoadRepo(); err != nil { + return nil, err + } + + comment, err := createCodeComment( + doer, + issue.Repo, + issue, + content, + treePath, + line, + replyReviewID, + ) + if err != nil { + return nil, err + } + + notification.NotifyCreateIssueComment(doer, issue.Repo, issue, comment) + + return comment, nil + } + + review, err := models.GetCurrentReview(doer, issue) + if err != nil { + if !models.IsErrReviewNotExist(err) { + return nil, err + } + + review, err = models.CreateReview(models.CreateReviewOptions{ + Type: models.ReviewTypePending, + Reviewer: doer, + Issue: issue, + }) + if err != nil { + return nil, err + } + } + + comment, err := createCodeComment( + doer, + issue.Repo, + issue, + content, + treePath, + line, + review.ID, + ) if err != nil { return nil, err } - if opts.Type != models.ReviewTypePending { - notification.NotifyPullRequestReview(review.Issue.PullRequest, review, nil) - } + // NOTICE: it's a pending review, so the notifications will not be fired until user submit review. - return review, nil + return comment, nil } -// UpdateReview updates a review -func UpdateReview(review *models.Review) error { - err := models.UpdateReview(review) +// createCodeComment creates a plain code comment at the specified line / path +func createCodeComment(doer *models.User, repo *models.Repository, issue *models.Issue, content, treePath string, line, reviewID int64) (*models.Comment, error) { + var commitID, patch string + if err := issue.LoadPullRequest(); err != nil { + return nil, fmt.Errorf("GetPullRequestByIssueID: %v", err) + } + pr := issue.PullRequest + if err := pr.GetBaseRepo(); err != nil { + return nil, fmt.Errorf("GetHeadRepo: %v", err) + } + gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) if err != nil { - return err + return nil, fmt.Errorf("OpenRepository: %v", err) + } + defer gitRepo.Close() + + // FIXME validate treePath + // Get latest commit referencing the commented line + // No need for get commit for base branch changes + if line > 0 { + commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(line)) + if err == nil { + commitID = commit.ID.String() + } else if !strings.Contains(err.Error(), "exit status 128 - fatal: no such path") { + return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) + } } - notification.NotifyPullRequestReview(review.Issue.PullRequest, review, nil) - - return nil + // Only fetch diff if comment is review comment + if reviewID != 0 { + headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) + if err != nil { + return nil, fmt.Errorf("GetRefCommitID[%s]: %v", pr.GetGitRefName(), err) + } + patchBuf := new(bytes.Buffer) + if err := gitdiff.GetRawDiffForFile(gitRepo.Path, pr.MergeBase, headCommitID, gitdiff.RawDiffNormal, treePath, patchBuf); err != nil { + return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, headCommitID, treePath) + } + patch = gitdiff.CutDiffAroundLine(patchBuf, int64((&models.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + } + return models.CreateComment(&models.CreateCommentOptions{ + Type: models.CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: content, + LineNum: line, + TreePath: treePath, + CommitSHA: commitID, + ReviewID: reviewID, + Patch: patch, + NoAction: true, + }) +} + +// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist +func SubmitReview(doer *models.User, issue *models.Issue, reviewType models.ReviewType, content string) (*models.Review, *models.Comment, error) { + review, comm, err := models.SubmitReview(doer, issue, reviewType, content) + if err != nil { + return nil, nil, err + } + + pr, err := issue.GetPullRequest() + if err != nil { + return nil, nil, err + } + notification.NotifyPullRequestReview(pr, review, comm) + + return review, comm, nil }