From 71ff661faa62b8834a7b13518a03638cd98cba79 Mon Sep 17 00:00:00 2001
From: Lunny Xiao <xiaolunwen@gmail.com>
Date: Mon, 25 Nov 2024 21:36:47 -0800
Subject: [PATCH] Add request review check and move to database transaction

---
 models/issues/review.go |  4 ++-
 services/pull/pull.go   | 79 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 70 insertions(+), 13 deletions(-)

diff --git a/models/issues/review.go b/models/issues/review.go
index 8b345e5fd80..69241ff8856 100644
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -46,6 +46,7 @@ func (err ErrReviewNotExist) Unwrap() error {
 type ErrNotValidReviewRequest struct {
 	Reason string
 	UserID int64
+	TeamID int64
 	RepoID int64
 }
 
@@ -56,9 +57,10 @@ func IsErrNotValidReviewRequest(err error) bool {
 }
 
 func (err ErrNotValidReviewRequest) Error() string {
-	return fmt.Sprintf("%s [user_id: %d, repo_id: %d]",
+	return fmt.Sprintf("%s [user_id: %d, team_id: %d, repo_id: %d]",
 		err.Reason,
 		err.UserID,
+		err.TeamID,
 		err.RepoID)
 }
 
diff --git a/services/pull/pull.go b/services/pull/pull.go
index 751ade986cb..9558efadd03 100644
--- a/services/pull/pull.go
+++ b/services/pull/pull.go
@@ -64,6 +64,46 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 		return user_model.ErrBlockedUser
 	}
 
+	// check if reviewers are valid
+	if len(opts.Reviewers) > 0 {
+		allowedUsers, err := GetReviewers(ctx, repo, pr.Issue.PosterID, pr.Issue.PosterID)
+		if err != nil {
+			return err
+		}
+		for _, reviewer := range opts.Reviewers {
+			var found bool
+			for _, allowedUser := range allowedUsers {
+				if allowedUser.ID == reviewer.ID {
+					found = true
+					break
+				}
+			}
+			if !found {
+				return issues_model.ErrNotValidReviewRequest{UserID: reviewer.ID}
+			}
+		}
+	}
+
+	// check if team reviewers are valid
+	if len(opts.TeamReviewers) > 0 {
+		allowedTeams, err := GetReviewerTeams(ctx, repo)
+		if err != nil {
+			return err
+		}
+		for _, teamReviewer := range opts.TeamReviewers {
+			var found bool
+			for _, allowedTeam := range allowedTeams {
+				if allowedTeam.ID == teamReviewer.ID {
+					found = true
+					break
+				}
+			}
+			if !found {
+				return issues_model.ErrNotValidReviewRequest{TeamID: teamReviewer.ID}
+			}
+		}
+	}
+
 	// user should be a collaborator or a member of the organization for base repo
 	if !issue.Poster.IsAdmin {
 		canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID)
@@ -174,7 +214,32 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 			return err
 		}
 
-		if !pr.IsWorkInProgress(ctx) {
+		// if there are reviewers or review teams, we don't need to request code owners review
+		if len(opts.Reviewers)+len(opts.TeamReviewers) > 0 {
+			for _, reviewer := range opts.Reviewers {
+				comment, err := issues_model.AddReviewRequest(ctx, pr.Issue, reviewer, issue.Poster)
+				if err != nil {
+					return err
+				}
+				reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{
+					Comment:  comment,
+					Reviewer: reviewer,
+					IsAdd:    true,
+				})
+			}
+
+			for _, teamReviewer := range opts.TeamReviewers {
+				comment, err := issues_model.AddTeamReviewRequest(ctx, pr.Issue, teamReviewer, issue.Poster)
+				if err != nil {
+					return err
+				}
+				reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{
+					Comment:    comment,
+					ReviewTeam: teamReviewer,
+					IsAdd:      true,
+				})
+			}
+		} else if !pr.IsWorkInProgress(ctx) {
 			reviewNotifiers, err = RequestCodeOwnersReview(ctx, issue, pr)
 			if err != nil {
 				return err
@@ -210,17 +275,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
 		}
 		notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID])
 	}
-	permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster)
-	for _, reviewer := range opts.Reviewers {
-		if _, err = ReviewRequest(ctx, pr, issue.Poster, &permDoer, reviewer, true); err != nil {
-			return err
-		}
-	}
-	for _, teamReviewer := range opts.TeamReviewers {
-		if _, err = TeamReviewRequest(ctx, pr, issue.Poster, teamReviewer, true); err != nil {
-			return err
-		}
-	}
+
 	return nil
 }