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

Author: lunny

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.

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

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

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())
 

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