Add reviewers selection to new pull request (#32403)

Users could add reviewers when creating new PRs.

---------

Co-authored-by: splitt3r <splitt3r@users.noreply.github.com>
Co-authored-by: Sebastian Sauer <sauer.sebastian@gmail.com>
Co-authored-by: bb-ben <70356237+bboerben@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

1 files changed, 123 insertions, 63 deletions

diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go
index c4fc535446..7fa8d428d3 100644
--- a/routers/web/repo/issue.go
+++ b/routers/web/repo/issue.go
@@ -654,34 +654,66 @@ func retrieveProjects(ctx *context.Context, repo *repo_model.Repository) {
 
 // repoReviewerSelection items to bee shown
 type repoReviewerSelection struct {
-	IsTeam    bool
-	Team      *organization.Team
-	User      *user_model.User
-	Review    *issues_model.Review
-	CanChange bool
-	Checked   bool
-	ItemID    int64
+	IsTeam         bool
+	Team           *organization.Team
+	User           *user_model.User
+	Review         *issues_model.Review
+	CanBeDismissed bool
+	CanChange      bool
+	Requested      bool
+	ItemID         int64
 }
 
-// RetrieveRepoReviewers find all reviewers of a repository
+type issueSidebarReviewersData struct {
+	Repository           *repo_model.Repository
+	RepoOwnerName        string
+	RepoLink             string
+	IssueID              int64
+	CanChooseReviewer    bool
+	OriginalReviews      issues_model.ReviewList
+	TeamReviewers        []*repoReviewerSelection
+	Reviewers            []*repoReviewerSelection
+	CurrentPullReviewers []*repoReviewerSelection
+}
+
+// RetrieveRepoReviewers find all reviewers of a repository. If issue is nil, it means the doer is creating a new PR.
 func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, issue *issues_model.Issue, canChooseReviewer bool) {
-	ctx.Data["CanChooseReviewer"] = canChooseReviewer
+	data := &issueSidebarReviewersData{}
+	data.RepoLink = ctx.Repo.RepoLink
+	data.Repository = repo
+	data.RepoOwnerName = repo.OwnerName
+	data.CanChooseReviewer = canChooseReviewer
 
-	originalAuthorReviews, err := issues_model.GetReviewersFromOriginalAuthorsByIssueID(ctx, issue.ID)
-	if err != nil {
-		ctx.ServerError("GetReviewersFromOriginalAuthorsByIssueID", err)
-		return
-	}
-	ctx.Data["OriginalReviews"] = originalAuthorReviews
+	var posterID int64
+	var isClosed bool
+	var reviews issues_model.ReviewList
 
-	reviews, err := issues_model.GetReviewsByIssueID(ctx, issue.ID)
-	if err != nil {
-		ctx.ServerError("GetReviewersByIssueID", err)
-		return
-	}
+	if issue == nil {
+		posterID = ctx.Doer.ID
+	} else {
+		posterID = issue.PosterID
+		if issue.OriginalAuthorID > 0 {
+			posterID = 0 // for migrated PRs, no poster ID
+		}
 
-	if len(reviews) == 0 && !canChooseReviewer {
-		return
+		data.IssueID = issue.ID
+		isClosed = issue.IsClosed || issue.PullRequest.HasMerged
+
+		originalAuthorReviews, err := issues_model.GetReviewersFromOriginalAuthorsByIssueID(ctx, issue.ID)
+		if err != nil {
+			ctx.ServerError("GetReviewersFromOriginalAuthorsByIssueID", err)
+			return
+		}
+		data.OriginalReviews = originalAuthorReviews
+
+		reviews, err = issues_model.GetReviewsByIssueID(ctx, issue.ID)
+		if err != nil {
+			ctx.ServerError("GetReviewersByIssueID", err)
+			return
+		}
+		if len(reviews) == 0 && !canChooseReviewer {
+			return
+		}
 	}
 
 	var (
@@ -693,11 +725,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 	)
 
 	if canChooseReviewer {
-		posterID := issue.PosterID
-		if issue.OriginalAuthorID > 0 {
-			posterID = 0
-		}
-
+		var err error
 		reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
 		if err != nil {
 			ctx.ServerError("GetReviewers", err)
@@ -723,9 +751,9 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 
 	for _, review := range reviews {
 		tmp := &repoReviewerSelection{
-			Checked: review.Type == issues_model.ReviewTypeRequest,
-			Review:  review,
-			ItemID:  review.ReviewerID,
+			Requested: review.Type == issues_model.ReviewTypeRequest,
+			Review:    review,
+			ItemID:    review.ReviewerID,
 		}
 		if review.ReviewerTeamID > 0 {
 			tmp.IsTeam = true
@@ -756,7 +784,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 		currentPullReviewers := make([]*repoReviewerSelection, 0, len(pullReviews))
 		for _, item := range pullReviews {
 			if item.Review.ReviewerID > 0 {
-				if err = item.Review.LoadReviewer(ctx); err != nil {
+				if err := item.Review.LoadReviewer(ctx); err != nil {
 					if user_model.IsErrUserNotExist(err) {
 						continue
 					}
@@ -765,7 +793,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 				}
 				item.User = item.Review.Reviewer
 			} else if item.Review.ReviewerTeamID > 0 {
-				if err = item.Review.LoadReviewerTeam(ctx); err != nil {
+				if err := item.Review.LoadReviewerTeam(ctx); err != nil {
 					if organization.IsErrTeamNotExist(err) {
 						continue
 					}
@@ -776,10 +804,11 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 			} else {
 				continue
 			}
-
+			item.CanBeDismissed = ctx.Repo.Permission.IsAdmin() && !isClosed &&
+				(item.Review.Type == issues_model.ReviewTypeApprove || item.Review.Type == issues_model.ReviewTypeReject)
 			currentPullReviewers = append(currentPullReviewers, item)
 		}
-		ctx.Data["PullReviewers"] = currentPullReviewers
+		data.CurrentPullReviewers = currentPullReviewers
 	}
 
 	if canChooseReviewer && reviewersResult != nil {
@@ -807,7 +836,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 			})
 		}
 
-		ctx.Data["Reviewers"] = reviewersResult
+		data.Reviewers = reviewersResult
 	}
 
 	if canChooseReviewer && teamReviewersResult != nil {
@@ -835,8 +864,10 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 			})
 		}
 
-		ctx.Data["TeamReviewers"] = teamReviewersResult
+		data.TeamReviewers = teamReviewersResult
 	}
+
+	ctx.Data["IssueSidebarReviewersData"] = data
 }
 
 // RetrieveRepoMetas find all the meta information of a repository
@@ -1117,7 +1148,14 @@ func DeleteIssue(ctx *context.Context) {
 }
 
 // ValidateRepoMetas check and returns repository's meta information
-func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull bool) ([]int64, []int64, int64, int64) {
+func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull bool) (ret struct {
+	LabelIDs, AssigneeIDs  []int64
+	MilestoneID, ProjectID int64
+
+	Reviewers     []*user_model.User
+	TeamReviewers []*organization.Team
+},
+) {
 	var (
 		repo = ctx.Repo.Repository
 		err  error
@@ -1125,7 +1163,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 
 	labels := RetrieveRepoMetas(ctx, ctx.Repo.Repository, isPull)
 	if ctx.Written() {
-		return nil, nil, 0, 0
+		return ret
 	}
 
 	var labelIDs []int64
@@ -1134,7 +1172,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 	if len(form.LabelIDs) > 0 {
 		labelIDs, err = base.StringsToInt64s(strings.Split(form.LabelIDs, ","))
 		if err != nil {
-			return nil, nil, 0, 0
+			return ret
 		}
 		labelIDMark := make(container.Set[int64])
 		labelIDMark.AddMultiple(labelIDs...)
@@ -1157,11 +1195,11 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 		milestone, err := issues_model.GetMilestoneByRepoID(ctx, ctx.Repo.Repository.ID, milestoneID)
 		if err != nil {
 			ctx.ServerError("GetMilestoneByID", err)
-			return nil, nil, 0, 0
+			return ret
 		}
 		if milestone.RepoID != repo.ID {
 			ctx.ServerError("GetMilestoneByID", err)
-			return nil, nil, 0, 0
+			return ret
 		}
 		ctx.Data["Milestone"] = milestone
 		ctx.Data["milestone_id"] = milestoneID
@@ -1171,11 +1209,11 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 		p, err := project_model.GetProjectByID(ctx, form.ProjectID)
 		if err != nil {
 			ctx.ServerError("GetProjectByID", err)
-			return nil, nil, 0, 0
+			return ret
 		}
 		if p.RepoID != ctx.Repo.Repository.ID && p.OwnerID != ctx.Repo.Repository.OwnerID {
 			ctx.NotFound("", nil)
-			return nil, nil, 0, 0
+			return ret
 		}
 
 		ctx.Data["Project"] = p
@@ -1187,7 +1225,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 	if len(form.AssigneeIDs) > 0 {
 		assigneeIDs, err = base.StringsToInt64s(strings.Split(form.AssigneeIDs, ","))
 		if err != nil {
-			return nil, nil, 0, 0
+			return ret
 		}
 
 		// Check if the passed assignees actually exists and is assignable
@@ -1195,18 +1233,18 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 			assignee, err := user_model.GetUserByID(ctx, aID)
 			if err != nil {
 				ctx.ServerError("GetUserByID", err)
-				return nil, nil, 0, 0
+				return ret
 			}
 
 			valid, err := access_model.CanBeAssigned(ctx, assignee, repo, isPull)
 			if err != nil {
 				ctx.ServerError("CanBeAssigned", err)
-				return nil, nil, 0, 0
+				return ret
 			}
 
 			if !valid {
 				ctx.ServerError("canBeAssigned", repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name})
-				return nil, nil, 0, 0
+				return ret
 			}
 		}
 	}
@@ -1216,7 +1254,39 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 		assigneeIDs = append(assigneeIDs, form.AssigneeID)
 	}
 
-	return labelIDs, assigneeIDs, milestoneID, form.ProjectID
+	// Check reviewers
+	var reviewers []*user_model.User
+	var teamReviewers []*organization.Team
+	if isPull && len(form.ReviewerIDs) > 0 {
+		reviewerIDs, err := base.StringsToInt64s(strings.Split(form.ReviewerIDs, ","))
+		if err != nil {
+			return ret
+		}
+		// Check if the passed reviewers (user/team) actually exist
+		for _, rID := range reviewerIDs {
+			// negative reviewIDs represent team requests
+			if rID < 0 {
+				teamReviewer, err := organization.GetTeamByID(ctx, -rID)
+				if err != nil {
+					ctx.ServerError("GetTeamByID", err)
+					return ret
+				}
+				teamReviewers = append(teamReviewers, teamReviewer)
+				continue
+			}
+
+			reviewer, err := user_model.GetUserByID(ctx, rID)
+			if err != nil {
+				ctx.ServerError("GetUserByID", err)
+				return ret
+			}
+			reviewers = append(reviewers, reviewer)
+		}
+	}
+
+	ret.LabelIDs, ret.AssigneeIDs, ret.MilestoneID, ret.ProjectID = labelIDs, assigneeIDs, milestoneID, form.ProjectID
+	ret.Reviewers, ret.TeamReviewers = reviewers, teamReviewers
+	return ret
 }
 
 // NewIssuePost response for creating new issue
@@ -1234,11 +1304,13 @@ func NewIssuePost(ctx *context.Context) {
 		attachments []string
 	)
 
-	labelIDs, assigneeIDs, milestoneID, projectID := ValidateRepoMetas(ctx, *form, false)
+	validateRet := ValidateRepoMetas(ctx, *form, false)
 	if ctx.Written() {
 		return
 	}
 
+	labelIDs, assigneeIDs, milestoneID, projectID := validateRet.LabelIDs, validateRet.AssigneeIDs, validateRet.MilestoneID, validateRet.ProjectID
+
 	if projectID > 0 {
 		if !ctx.Repo.CanRead(unit.TypeProjects) {
 			// User must also be able to see the project.
@@ -2479,7 +2551,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 				return
 			}
 
-			err = issue_service.IsValidTeamReviewRequest(ctx, team, ctx.Doer, action == "attach", issue)
+			_, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach")
 			if err != nil {
 				if issues_model.IsErrNotValidReviewRequest(err) {
 					log.Warn(
@@ -2490,12 +2562,6 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 					ctx.Status(http.StatusForbidden)
 					return
 				}
-				ctx.ServerError("IsValidTeamReviewRequest", err)
-				return
-			}
-
-			_, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach")
-			if err != nil {
 				ctx.ServerError("TeamReviewRequest", err)
 				return
 			}
@@ -2517,7 +2583,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 			return
 		}
 
-		err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, action == "attach", issue, nil)
+		_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach")
 		if err != nil {
 			if issues_model.IsErrNotValidReviewRequest(err) {
 				log.Warn(
@@ -2528,12 +2594,6 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 				ctx.Status(http.StatusForbidden)
 				return
 			}
-			ctx.ServerError("isValidReviewRequest", err)
-			return
-		}
-
-		_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach")
-		if err != nil {
 			if issues_model.IsErrReviewRequestOnClosedPR(err) {
 				ctx.Status(http.StatusForbidden)
 				return