From cfa6db75ae694756dd553d15e1ae7404335186bb Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Sat, 20 Jun 2020 18:16:26 +0800
Subject: [PATCH 01/24] Add team support for review request

Block #11355

Signed-off-by: a1012112796 <1012112796@qq.com>
---
 models/issue_comment.go                       |  20 ++
 models/repo.go                                |  18 +-
 models/review.go                              | 195 +++++++++++++++++-
 routers/repo/issue.go                         |  81 +++++++-
 services/issue/assignee.go                    |  33 ++-
 .../repo/issue/view_content/comments.tmpl     |  18 +-
 templates/repo/issue/view_content/pull.tmpl   |  11 +-
 .../repo/issue/view_content/sidebar.tmpl      |  41 +++-
 8 files changed, 400 insertions(+), 17 deletions(-)

diff --git a/models/issue_comment.go b/models/issue_comment.go
index 452afc79f00df..37f09ca3de211 100644
--- a/models/issue_comment.go
+++ b/models/issue_comment.go
@@ -129,6 +129,7 @@ type Comment struct {
 	AssigneeID       int64
 	RemovedAssignee  bool
 	Assignee         *User `xorm:"-"`
+	TeamAssignee     *Team `xorm:"-"`
 	ResolveDoerID    int64
 	ResolveDoer      *User `xorm:"-"`
 	OldTitle         string
@@ -465,6 +466,25 @@ func (c *Comment) LoadAssigneeUser() error {
 			}
 			c.Assignee = NewGhostUser()
 		}
+	} else {
+		if err = c.LoadIssue(); err != nil {
+			return err
+		}
+
+		if err = c.Issue.LoadRepo(); err != nil {
+			return err
+		}
+
+		if err = c.Issue.Repo.GetOwner(); err != nil {
+			return err
+		}
+
+		if c.Issue.Repo.Owner.IsOrganization() {
+			c.TeamAssignee, err = GetTeamByID(-c.AssigneeID)
+			if err != nil && !IsErrTeamNotExist(err) {
+				return err
+			}
+		}
 	}
 	return nil
 }
diff --git a/models/repo.go b/models/repo.go
index 3b874f3359af6..fc38b7240f683 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -685,9 +685,9 @@ func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_
 	return users, nil
 }
 
-func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, err error) {
+func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, teams []*Team, err error) {
 	if err = repo.getOwner(e); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	if repo.IsPrivate ||
@@ -696,6 +696,18 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []
 	} else {
 		users, err = repo.getReviewersPublic(x, doerID, posterID)
 	}
+
+	if repo.Owner.IsOrganization() {
+		teams, err = GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
+		if err != nil {
+			return nil, nil, err
+		}
+
+		for _, team := range teams {
+			team.ID = -team.ID
+		}
+	}
+
 	return
 }
 
@@ -704,7 +716,7 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []
 // but for public rpo, that return all users that have write access or higher to the repository,
 // and all repo watchers.
 // TODO: may be we should hava a busy choice for users to block review request to them.
-func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, err error) {
+func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, _ []*Team, err error) {
 	return repo.getReviewers(x, doerID, posterID)
 }
 
diff --git a/models/review.go b/models/review.go
index 522fe5886c372..0dafa58d6af56 100644
--- a/models/review.go
+++ b/models/review.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"strings"
 
+	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/timeutil"
 
 	"xorm.io/builder"
@@ -53,6 +54,7 @@ type Review struct {
 	ID               int64 `xorm:"pk autoincr"`
 	Type             ReviewType
 	Reviewer         *User `xorm:"-"`
+	ReviewerTeam     *Team `xorm:"-"`
 	ReviewerID       int64 `xorm:"index"`
 	OriginalAuthor   string
 	OriginalAuthorID int64
@@ -101,6 +103,11 @@ func (r *Review) loadReviewer(e Engine) (err error) {
 	if r.Reviewer != nil || r.ReviewerID == 0 {
 		return nil
 	}
+
+	if r.ReviewerID < 0 {
+		r.ReviewerTeam, err = getTeamByID(e, -r.ReviewerID)
+		return
+	}
 	r.Reviewer, err = getUserByID(e, r.ReviewerID)
 	return
 }
@@ -218,6 +225,30 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
 	return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
 }
 
+// IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals)
+func IsOfficialReviewerTeam(issue *Issue, team *Team) (bool, error) {
+	return isOfficialReviewerTeam(x, issue, team)
+}
+
+func isOfficialReviewerTeam(e Engine, issue *Issue, team *Team) (bool, error) {
+	pr, err := getPullRequestByIssueID(e, issue.ID)
+	if err != nil {
+		return false, err
+	}
+	if err = pr.loadProtectedBranch(e); err != nil {
+		return false, err
+	}
+	if pr.ProtectedBranch == nil {
+		return false, nil
+	}
+
+	if !pr.ProtectedBranch.EnableApprovalsWhitelist {
+		return team.Authorize >= AccessModeWrite, nil
+	}
+
+	return base.Int64sContains(pr.ProtectedBranch.ApprovalsWhitelistTeamIDs, team.ID), nil
+}
+
 func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
 	review := &Review{
 		Type:       opts.Type,
@@ -373,6 +404,29 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 		return nil, nil, err
 	}
 
+	// try to remove team review request if need
+	if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
+		teamReviewRequests := make([]*Review, 0, 10)
+		if err = sess.SQL("SELECT * FROM review WHERE reviewer_id < 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
+			return nil, nil, err
+		}
+
+		if len(teamReviewRequests) > 0 {
+			for _, teamReviewRequest := range teamReviewRequests {
+				ok := false
+				if ok, err = isTeamMember(sess, issue.Repo.OwnerID, -teamReviewRequest.ReviewerID, doer.ID); err != nil {
+					return nil, nil, err
+				}
+
+				if ok {
+					if _, err := sess.Delete(teamReviewRequest); err != nil {
+						return nil, nil, err
+					}
+				}
+			}
+		}
+	}
+
 	comm.Review = review
 	return review, comm, sess.Commit()
 }
@@ -397,7 +451,7 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
 	// Load reviewer and skip if user is deleted
 	for _, review := range reviewsUnfiltered {
 		if err = review.loadReviewer(sess); err != nil {
-			if !IsErrUserNotExist(err) {
+			if !IsErrUserNotExist(err) && !IsErrTeamNotExist(err) {
 				return nil, err
 			}
 		} else {
@@ -482,7 +536,7 @@ func InsertReviews(reviews []*Review) error {
 }
 
 // AddReviewRequest add a review request from one reviewer
-func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Comment, err error) {
+func AddReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err error) {
 	review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
 	if err != nil {
 		return
@@ -549,7 +603,7 @@ func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Commen
 }
 
 //RemoveReviewRequest remove a review request from one reviewer
-func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Comment, err error) {
+func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err error) {
 	review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
 	if err != nil {
 		return
@@ -612,6 +666,141 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
 	return comment, sess.Commit()
 }
 
+// AddTeamReviewRequest add a review request from one team
+func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
+	review, err := GetReviewerByIssueIDAndUserID(issue.ID, -reviewer.ID)
+	if err != nil {
+		return
+	}
+
+	// skip it when reviewer hase been request to review
+	if review != nil && review.Type == ReviewTypeRequest {
+		return nil, nil
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
+	var official bool
+	official, err = isOfficialReviewerTeam(sess, issue, reviewer)
+
+	if err != nil {
+		return nil, err
+	}
+
+	if !official {
+		official, err = isOfficialReviewer(sess, issue, doer)
+
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if official {
+		if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, -reviewer.ID); err != nil {
+			return nil, err
+		}
+	}
+
+	_, err = createReview(sess, CreateReviewOptions{
+		Type:     ReviewTypeRequest,
+		Issue:    issue,
+		Reviewer: &User{ID: -reviewer.ID},
+		Official: official,
+		Stale:    false,
+	})
+
+	if err != nil {
+		return
+	}
+
+	comment, err = createComment(sess, &CreateCommentOptions{
+		Type:            CommentTypeReviewRequest,
+		Doer:            doer,
+		Repo:            issue.Repo,
+		Issue:           issue,
+		RemovedAssignee: false,        // Use RemovedAssignee as !isRequest
+		AssigneeID:      -reviewer.ID, // Use AssigneeID as reviewer ID
+	})
+
+	if err != nil {
+		return nil, err
+	}
+
+	return comment, sess.Commit()
+}
+
+//RemoveTeamReviewRequest remove a review request from one team
+func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
+	review, err := GetReviewerByIssueIDAndUserID(issue.ID, -reviewer.ID)
+	if err != nil {
+		return
+	}
+
+	if review.Type != ReviewTypeRequest {
+		return nil, nil
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
+	_, err = sess.Delete(review)
+	if err != nil {
+		return nil, err
+	}
+
+	var official bool
+	official, err = isOfficialReviewerTeam(sess, issue, reviewer)
+	if err != nil {
+		return
+	}
+
+	if official {
+		// recalculate which is the latest official review from that team
+		var review *Review
+
+		review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
+		if err != nil {
+			return nil, err
+		}
+
+		if review != nil {
+			if _, err := sess.Exec("UPDATE `review` SET official=? WHERE id=?", true, review.ID); err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	if err != nil {
+		return nil, err
+	}
+
+	if doer == nil {
+		return nil, sess.Commit()
+	}
+
+	comment, err = createComment(sess, &CreateCommentOptions{
+		Type:            CommentTypeReviewRequest,
+		Doer:            doer,
+		Repo:            issue.Repo,
+		Issue:           issue,
+		RemovedAssignee: true,         // Use RemovedAssignee as !isRequest
+		AssigneeID:      -reviewer.ID, // Use AssigneeID as reviewer ID
+	})
+
+	if err != nil {
+		return nil, err
+	}
+
+	return comment, sess.Commit()
+}
+
 // MarkConversation Add or remove Conversation mark for a code comment
 func MarkConversation(comment *Comment, doer *User, isResolve bool) (err error) {
 	if comment.Type != CommentTypeCode {
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 181ed59a8c1a8..1e3fdcbde8bc0 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -390,7 +390,7 @@ func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *models.Repos
 // RetrieveRepoReviewers find all reviewers of a repository
 func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issuePosterID int64) {
 	var err error
-	ctx.Data["Reviewers"], err = repo.GetReviewers(ctx.User.ID, issuePosterID)
+	ctx.Data["Reviewers"], ctx.Data["TeamReviewers"], err = repo.GetReviewers(ctx.User.ID, issuePosterID)
 	if err != nil {
 		ctx.ServerError("GetReviewers", err)
 		return
@@ -1395,6 +1395,46 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 	return nil
 }
 
+func isLegalTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bool, issue *models.Issue) error {
+	if doer.IsOrganization() {
+		return fmt.Errorf("Organization can't be doer to add reviewer [user_id: %d, repo_id: %d]", doer.ID, issue.PullRequest.BaseRepo.ID)
+	}
+
+	permDoer, err := models.GetUserRepoPermission(issue.Repo, doer)
+	if err != nil {
+		return err
+	}
+
+	var pemResult bool
+	if isAdd {
+		if issue.Repo.IsPrivate {
+			pemResult = models.HasTeamRepo(reviewer.OrgID, reviewer.ID, issue.RepoID)
+
+			if !pemResult {
+				return fmt.Errorf("Reviewer can't read [team_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
+			}
+		}
+
+		pemResult = permDoer.CanAccessAny(models.AccessModeWrite, models.UnitTypePullRequests)
+		if !pemResult {
+			pemResult, err = models.IsOfficialReviewer(issue, doer)
+			if err != nil {
+				return err
+			}
+			if !pemResult {
+				return fmt.Errorf("Doer can't choose reviewer [user_id: %d, repo_name: %s, issue_id: %d]", doer.ID, issue.Repo.Name, issue.ID)
+			}
+		}
+	} else {
+		pemResult = permDoer.IsAdmin()
+		if !pemResult {
+			return fmt.Errorf("Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
+		}
+	}
+
+	return nil
+}
+
 // updatePullReviewRequest change pull's request reviewers
 func updatePullReviewRequest(ctx *context.Context) {
 	issues := getActionIssues(ctx)
@@ -1413,6 +1453,45 @@ func updatePullReviewRequest(ctx *context.Context) {
 	for _, issue := range issues {
 		if issue.IsPull {
 
+			if reviewID < 0 {
+				if err := issue.LoadRepo(); err != nil {
+					ctx.ServerError("issue.LoadRepo", err)
+					return
+				}
+
+				if err := issue.Repo.GetOwner(); err != nil {
+					ctx.ServerError("issue.Repo.GetOwner", err)
+					return
+				}
+
+				if issue.Repo.Owner.IsOrganization() {
+					team, err := models.GetTeamByID(-reviewID)
+					if err != nil {
+						ctx.ServerError("models.GetTeamByID", err)
+						return
+					}
+
+					if team.OrgID != issue.Repo.OwnerID {
+						ctx.Status(403)
+						return
+					}
+
+					err = isLegalTeamReviewRequest(team, ctx.User, event == "add", issue)
+					if err != nil {
+						ctx.ServerError("isLegalTeamReviewRequest", err)
+						return
+					}
+
+					err = issue_service.TeamReviewRequest(issue, ctx.User, team, event == "add")
+					if err != nil {
+						ctx.ServerError("ReviewRequest", err)
+						return
+					}
+
+					continue
+				}
+			}
+
 			reviewer, err := models.GetUserByID(reviewID)
 			if err != nil {
 				ctx.ServerError("GetUserByID", err)
diff --git a/services/issue/assignee.go b/services/issue/assignee.go
index d63c7bf032e65..ec17f9e7febea 100644
--- a/services/issue/assignee.go
+++ b/services/issue/assignee.go
@@ -52,7 +52,7 @@ func ToggleAssignee(issue *models.Issue, doer *models.User, assigneeID int64) (r
 	return
 }
 
-// ReviewRequest add or remove a review for this PR, and make comment for it.
+// ReviewRequest add or remove a review request from a user for this PR, and make comment for it.
 func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User, isAdd bool) (err error) {
 	var comment *models.Comment
 	if isAdd {
@@ -71,3 +71,34 @@ func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User
 
 	return nil
 }
+
+// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it.
+func TeamReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.Team, isAdd bool) (err error) {
+	var comment *models.Comment
+	if isAdd {
+		comment, err = models.AddTeamReviewRequest(issue, reviewer, doer)
+	} else {
+		comment, err = models.RemoveTeamReviewRequest(issue, reviewer, doer)
+	}
+
+	if err != nil {
+		return
+	}
+
+	if comment != nil && isAdd {
+		// notify all user in this team
+		if err = comment.LoadIssue(); err != nil {
+			return
+		}
+		err = reviewer.GetMembers(&models.SearchMembersOptions{})
+		for _, member := range reviewer.Members {
+			if member.ID == comment.Issue.PosterID {
+				continue
+			}
+			comment.AssigneeID = member.ID
+			notification.NotifyPullReviewRequest(doer, issue, member, isAdd, comment)
+		}
+	}
+
+	return nil
+}
diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl
index b3ef258a6c863..7be149d936cf7 100644
--- a/templates/repo/issue/view_content/comments.tmpl
+++ b/templates/repo/issue/view_content/comments.tmpl
@@ -590,14 +590,22 @@
 			</a>
 			<span class="text grey">
 				<a class="author" href="{{.Poster.HomeLink}}">{{.Poster.GetDisplayName}}</a>
-				{{if .RemovedAssignee}}
-					{{if eq .PosterID .AssigneeID}}
-						{{$.i18n.Tr "repo.issues.review.remove_review_request_self" $createdStr | Safe}}
+				{{if (gt .AssigneeID 0)}}
+					{{if .RemovedAssignee}}
+						{{if eq .PosterID .AssigneeID}}
+							{{$.i18n.Tr "repo.issues.review.remove_review_request_self" $createdStr | Safe}}
+						{{else}}
+							{{$.i18n.Tr "repo.issues.review.remove_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}}
+						{{end}}
 					{{else}}
-						{{$.i18n.Tr "repo.issues.review.remove_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}}
+						{{$.i18n.Tr "repo.issues.review.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}}
 					{{end}}
 				{{else}}
-					{{$.i18n.Tr "repo.issues.review.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}}
+					{{if .RemovedAssignee}}
+						{{$.i18n.Tr "repo.issues.review.remove_review_request" (.TeamAssignee.Name|Escape) $createdStr | Safe}}
+					{{else}}
+						{{$.i18n.Tr "repo.issues.review.add_review_request" (.TeamAssignee.Name|Escape) $createdStr | Safe}}
+					{{end}}
 				{{end}}
 			</span>
 		</div>
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index dc897bd7b9371..37aa2c9a86a0c 100644
--- a/templates/repo/issue/view_content/pull.tmpl
+++ b/templates/repo/issue/view_content/pull.tmpl
@@ -8,10 +8,17 @@
 					<div class="ui divider"></div>
 					<div class="review-item">
 						<div class="review-item-left">
+							{{if (gt .ReviewerID 0)}}
 							<a class="ui avatar image" href="{{.Reviewer.HomeLink}}">
 								<img src="{{.Reviewer.RelAvatarLink}}">
 							</a>
-							<span class="text grey"><a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a>
+							{{end}}
+							<span class="text grey">
+								{{if (gt .ReviewerID 0)}}
+									<a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a>
+								{{else}}
+									<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.ReviewerTeam.Name}}</span>
+								{{end}}
 								{{if eq .Type 1}}
 									{{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}}
 								{{else if eq .Type 2}}
@@ -39,7 +46,7 @@
 
 								{{$canChoose := false}}
 								{{if eq .Type 4}}
-									{{if or (eq .ReviewerID $.SignedUserID) $.Permission.IsAdmin}}
+									{{if or (and (eq .ReviewerID $.SignedUserID) (gt .ReviewerID 0)) $.Permission.IsAdmin}}
 										{{$canChoose = true}}
 									{{end}}
 								{{else}}
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index f0c7f69b5857e..beeeb149da353 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -51,6 +51,39 @@
 						</span>
 					</a>
 				{{end}}
+				{{if .TeamReviewers}}
+					{{range .TeamReviewers}}
+						{{$ReviewerID := .ID}}
+						{{$checked := false}}
+						{{$canChoose := false}}
+						{{$notReviewed := true}}
+
+						{{range $.PullReviewers}}
+							{{if eq .ReviewerID $ReviewerID }}
+								{{$notReviewed = false }}
+								{{if  eq .Type 4 }}
+									{{$checked = true}}
+									{{if $.Permission.IsAdmin}}
+										{{$canChoose = true}}
+									{{end}}
+								{{else}}
+									{{$canChoose = true}}
+								{{end}}
+							{{end}}
+						{{end}}
+
+						{{ if $notReviewed}}
+							{{$canChoose = true}}
+						{{end}}
+
+						<a class="{{if not $canChoose}}ui poping up{{end}} item {{if $checked}} checked {{end}}" href="#" data-id="{{.ID}}" data-id-selector="#review_request_{{.ID}}" data-can-change="{{if not $canChoose}}block{{end}}" {{if not $canChoose}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}} data-is-checked="{{if $checked}}add{{else}}remove{{end}}">
+							<span class="octicon-check {{if not $checked}}invisible{{end}}">{{svg "octicon-check" 16}}</span>
+							<span class="ui text">
+								{{$.Issue.Repo.OwnerName}}/{{.Name}}
+							</span>
+						</a>
+					{{end}}
+				{{end}}
 			</div>
 		</div>
 
@@ -59,7 +92,11 @@
 			<div class="selected">
 				{{range .PullReviewers}}
 					<div class="item" style="margin-bottom: 10px;">
-						<a href="{{.Reviewer.HomeLink}}"><img class="ui avatar image" src="{{.Reviewer.RelAvatarLink}}">&nbsp;{{.Reviewer.GetDisplayName}}</a>
+						{{if (gt .ReviewerID 0)}}
+							<a href="{{.Reviewer.HomeLink}}"><img class="ui avatar image" src="{{.Reviewer.RelAvatarLink}}">&nbsp;{{.Reviewer.GetDisplayName}}</a>
+						{{else}}
+							<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.ReviewerTeam.Name}}</span>
+						{{end}}
 						<span class="ui right type-icon text {{if eq .Type 1}}green
 							{{- else if eq .Type 2}}grey
 							{{- else if eq .Type 3}}red
@@ -68,7 +105,7 @@
 
 							{{$canChoose := false}}
 							{{if eq .Type 4}}
-								{{if or (eq .ReviewerID $.SignedUserID) $.Permission.IsAdmin}}
+								{{if or (and (eq .ReviewerID $.SignedUserID) (gt .ReviewerID 0) ) $.Permission.IsAdmin}}
 									{{$canChoose = true}}
 								{{end}}
 							{{else}}

From 3196c8e0f3e9800bbdc7fd05567d321be0b20046 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Wed, 24 Jun 2020 22:28:20 +0800
Subject: [PATCH 02/24] fix lint

---
 services/issue/assignee.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/services/issue/assignee.go b/services/issue/assignee.go
index ec17f9e7febea..35edec206a2a0 100644
--- a/services/issue/assignee.go
+++ b/services/issue/assignee.go
@@ -90,7 +90,10 @@ func TeamReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.
 		if err = comment.LoadIssue(); err != nil {
 			return
 		}
-		err = reviewer.GetMembers(&models.SearchMembersOptions{})
+		if err = reviewer.GetMembers(&models.SearchMembersOptions{}); err != nil {
+			return
+		}
+
 		for _, member := range reviewer.Members {
 			if member.ID == comment.Issue.PosterID {
 				continue

From 3c02d48d5a1e2b89517b84c36641844c5aff726d Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Sun, 13 Sep 2020 17:26:32 +0800
Subject: [PATCH 03/24] Apply review suggestion

---
 models/repo.go        |  4 ----
 routers/repo/issue.go | 17 +++++++++++++----
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/models/repo.go b/models/repo.go
index a610e3b4495a6..e1009556ea0cf 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -706,10 +706,6 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []
 		if err != nil {
 			return nil, nil, err
 		}
-
-		for _, team := range teams {
-			team.ID = -team.ID
-		}
 	}
 
 	return
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 13c9d9c73abc6..c04b0c3714a53 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -436,12 +436,21 @@ func retrieveProjects(ctx *context.Context, repo *models.Repository) {
 
 // RetrieveRepoReviewers find all reviewers of a repository
 func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issuePosterID int64) {
-	var err error
-	ctx.Data["Reviewers"], ctx.Data["TeamReviewers"], err = repo.GetReviewers(ctx.User.ID, issuePosterID)
+	var (
+		err           error
+		teamReviewers []*models.Team
+	)
+	ctx.Data["Reviewers"], teamReviewers, err = repo.GetReviewers(ctx.User.ID, issuePosterID)
 	if err != nil {
 		ctx.ServerError("GetReviewers", err)
 		return
 	}
+
+	for _, team := range teamReviewers {
+		team.ID = -team.ID
+	}
+
+	ctx.Data["TeamReviewers"] = teamReviewers
 }
 
 // RetrieveRepoMetas find all the meta information of a repository
@@ -1672,13 +1681,13 @@ func updatePullReviewRequest(ctx *context.Context) {
 						return
 					}
 
-					err = isLegalTeamReviewRequest(team, ctx.User, event == "add", issue)
+					err = isLegalTeamReviewRequest(team, ctx.User, action == "attach", issue)
 					if err != nil {
 						ctx.ServerError("isLegalTeamReviewRequest", err)
 						return
 					}
 
-					err = issue_service.TeamReviewRequest(issue, ctx.User, team, event == "add")
+					err = issue_service.TeamReviewRequest(issue, ctx.User, team, action == "attach")
 					if err != nil {
 						ctx.ServerError("ReviewRequest", err)
 						return

From fbef6d77f7d0a3acca0c238e055cd5ed2d63dcb4 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Thu, 17 Sep 2020 17:21:52 +0800
Subject: [PATCH 04/24] Apply suggestion from review @lafriks

---
 models/issue_comment.go                       |  15 +-
 models/migrations/migrations.go               |   2 +
 models/migrations/v152.go                     |  29 ++++
 models/review.go                              | 154 ++++++++++++------
 routers/repo/issue.go                         |   4 +-
 .../repo/issue/view_content/comments.tmpl     |   4 +-
 .../repo/issue/view_content/sidebar.tmpl      |   2 +-
 7 files changed, 151 insertions(+), 59 deletions(-)
 create mode 100644 models/migrations/v152.go

diff --git a/models/issue_comment.go b/models/issue_comment.go
index 77a759913351c..270a10e240141 100644
--- a/models/issue_comment.go
+++ b/models/issue_comment.go
@@ -137,7 +137,8 @@ type Comment struct {
 	AssigneeID       int64
 	RemovedAssignee  bool
 	Assignee         *User `xorm:"-"`
-	TeamAssignee     *Team `xorm:"-"`
+	AssigneeTeamID   int64 `xorm:"NOT NULL DEFAULT 0"`
+	AssigneeTeam     *Team `xorm:"-"`
 	ResolveDoerID    int64
 	ResolveDoer      *User `xorm:"-"`
 	OldTitle         string
@@ -488,11 +489,11 @@ func (c *Comment) UpdateAttachments(uuids []string) error {
 	return sess.Commit()
 }
 
-// LoadAssigneeUser if comment.Type is CommentTypeAssignees, then load assignees
-func (c *Comment) LoadAssigneeUser() error {
+// LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees
+func (c *Comment) LoadAssigneeUserAndTeam() error {
 	var err error
 
-	if c.AssigneeID > 0 {
+	if c.AssigneeID > 0 && c.Assignee == nil {
 		c.Assignee, err = getUserByID(x, c.AssigneeID)
 		if err != nil {
 			if !IsErrUserNotExist(err) {
@@ -500,7 +501,7 @@ func (c *Comment) LoadAssigneeUser() error {
 			}
 			c.Assignee = NewGhostUser()
 		}
-	} else {
+	} else if c.AssigneeTeamID > 0 && c.AssigneeTeam == nil {
 		if err = c.LoadIssue(); err != nil {
 			return err
 		}
@@ -514,7 +515,7 @@ func (c *Comment) LoadAssigneeUser() error {
 		}
 
 		if c.Issue.Repo.Owner.IsOrganization() {
-			c.TeamAssignee, err = GetTeamByID(-c.AssigneeID)
+			c.AssigneeTeam, err = GetTeamByID(c.AssigneeTeamID)
 			if err != nil && !IsErrTeamNotExist(err) {
 				return err
 			}
@@ -705,6 +706,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
 		ProjectID:        opts.ProjectID,
 		RemovedAssignee:  opts.RemovedAssignee,
 		AssigneeID:       opts.AssigneeID,
+		AssigneeTeamID:   opts.AssigneeTeamID,
 		CommitID:         opts.CommitID,
 		CommitSHA:        opts.CommitSHA,
 		Line:             opts.LineNum,
@@ -869,6 +871,7 @@ type CreateCommentOptions struct {
 	OldProjectID     int64
 	ProjectID        int64
 	AssigneeID       int64
+	AssigneeTeamID   int64
 	RemovedAssignee  bool
 	OldTitle         string
 	NewTitle         string
diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 5317cc5743d5d..c197902138aec 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -237,6 +237,8 @@ var migrations = []Migration{
 	NewMigration("add primary key to repo_topic", addPrimaryKeyToRepoTopic),
 	// v151 -> v152
 	NewMigration("set default password algorithm to Argon2", setDefaultPasswordToArgon2),
+	// v152 -> v153
+	NewMigration("add Team review request support", addTeamReviewRequestSupport),
 }
 
 // GetCurrentDBVersion returns the current db version
diff --git a/models/migrations/v152.go b/models/migrations/v152.go
new file mode 100644
index 0000000000000..4ef8898b1ce86
--- /dev/null
+++ b/models/migrations/v152.go
@@ -0,0 +1,29 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"xorm.io/xorm"
+)
+
+func addTeamReviewRequestSupport(x *xorm.Engine) error {
+	type Review struct {
+		ReviewerTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	type Comment struct {
+		AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	if err := x.Sync2(new(Review)); err != nil {
+		return err
+	}
+
+	if err := x.Sync2(new(Comment)); err != nil {
+		return err
+	}
+
+	return nil
+}
diff --git a/models/review.go b/models/review.go
index b51180e5abf86..9c9a56131bee3 100644
--- a/models/review.go
+++ b/models/review.go
@@ -54,8 +54,9 @@ type Review struct {
 	ID               int64 `xorm:"pk autoincr"`
 	Type             ReviewType
 	Reviewer         *User `xorm:"-"`
-	ReviewerTeam     *Team `xorm:"-"`
 	ReviewerID       int64 `xorm:"index"`
+	ReviewerTeamID   int64 `xorm:"NOT NULL DEFAULT 0"`
+	ReviewerTeam     *Team `xorm:"-"`
 	OriginalAuthor   string
 	OriginalAuthorID int64
 	Issue            *Issue `xorm:"-"`
@@ -100,15 +101,19 @@ func (r *Review) loadIssue(e Engine) (err error) {
 }
 
 func (r *Review) loadReviewer(e Engine) (err error) {
-	if r.Reviewer != nil || r.ReviewerID == 0 {
-		return nil
+	if r.ReviewerID == 0 || r.Reviewer != nil {
+		return
 	}
+	r.Reviewer, err = getUserByID(e, r.ReviewerID)
+	return
+}
 
-	if r.ReviewerID < 0 {
-		r.ReviewerTeam, err = getTeamByID(e, -r.ReviewerID)
+func (r *Review) loadReviewerTeam(e Engine) (err error) {
+	if r.ReviewerTeamID == 0 || r.ReviewerTeam != nil {
 		return
 	}
-	r.Reviewer, err = getUserByID(e, r.ReviewerID)
+
+	r.ReviewerTeam, err = getTeamByID(e, r.ReviewerTeamID)
 	return
 }
 
@@ -127,6 +132,9 @@ func (r *Review) loadAttributes(e Engine) (err error) {
 	if err = r.loadReviewer(e); err != nil {
 		return
 	}
+	if err = r.loadReviewerTeam(e); err != nil {
+		return
+	}
 	return
 }
 
@@ -196,13 +204,14 @@ func FindReviews(opts FindReviewOptions) ([]*Review, error) {
 
 // CreateReviewOptions represent the options to create a review. Type, Issue and Reviewer are required.
 type CreateReviewOptions struct {
-	Content  string
-	Type     ReviewType
-	Issue    *Issue
-	Reviewer *User
-	Official bool
-	CommitID string
-	Stale    bool
+	Content      string
+	Type         ReviewType
+	Issue        *Issue
+	Reviewer     *User
+	ReviewerTeam *Team
+	Official     bool
+	CommitID     string
+	Stale        bool
 }
 
 // IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
@@ -251,15 +260,20 @@ func isOfficialReviewerTeam(e Engine, issue *Issue, team *Team) (bool, error) {
 
 func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
 	review := &Review{
-		Type:       opts.Type,
-		Issue:      opts.Issue,
-		IssueID:    opts.Issue.ID,
-		Reviewer:   opts.Reviewer,
-		ReviewerID: opts.Reviewer.ID,
-		Content:    opts.Content,
-		Official:   opts.Official,
-		CommitID:   opts.CommitID,
-		Stale:      opts.Stale,
+		Type:         opts.Type,
+		Issue:        opts.Issue,
+		IssueID:      opts.Issue.ID,
+		Reviewer:     opts.Reviewer,
+		ReviewerTeam: opts.ReviewerTeam,
+		Content:      opts.Content,
+		Official:     opts.Official,
+		CommitID:     opts.CommitID,
+		Stale:        opts.Stale,
+	}
+	if opts.Reviewer != nil {
+		review.ReviewerID = opts.Reviewer.ID
+	} else {
+		review.ReviewerTeamID = opts.ReviewerTeam.ID
 	}
 	if _, err := e.Insert(review); err != nil {
 		return nil, err
@@ -407,14 +421,14 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 	// try to remove team review request if need
 	if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
 		teamReviewRequests := make([]*Review, 0, 10)
-		if err = sess.SQL("SELECT * FROM review WHERE reviewer_id < 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
+		if err = sess.SQL("SELECT * FROM review WHERE reviewer_team_id > 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
 			return nil, nil, err
 		}
 
 		if len(teamReviewRequests) > 0 {
 			for _, teamReviewRequest := range teamReviewRequests {
 				ok := false
-				if ok, err = isTeamMember(sess, issue.Repo.OwnerID, -teamReviewRequest.ReviewerID, doer.ID); err != nil {
+				if ok, err = isTeamMember(sess, issue.Repo.OwnerID, teamReviewRequest.ReviewerTeamID, doer.ID); err != nil {
 					return nil, nil, err
 				}
 
@@ -442,7 +456,7 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
 	}
 
 	// Get latest review of each reviwer, sorted in order they were made
-	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND type in (?, ?, ?) GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
+	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
 		issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
 		Find(&reviewsUnfiltered); err != nil {
 		return nil, err
@@ -451,10 +465,32 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
 	// Load reviewer and skip if user is deleted
 	for _, review := range reviewsUnfiltered {
 		if err = review.loadReviewer(sess); err != nil {
-			if !IsErrUserNotExist(err) && !IsErrTeamNotExist(err) {
+			if !IsErrUserNotExist(err) {
+				return nil, err
+			}
+		} else {
+			reviews = append(reviews, review)
+		}
+	}
+
+	teamReviewRequests := make([]*Review, 0, 5)
+	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
+		issueID).
+		Find(&teamReviewRequests); err != nil {
+		return nil, err
+	}
+
+	if len(teamReviewRequests) == 0 {
+		return reviews, nil
+	}
+
+	for _, review := range teamReviewRequests {
+		if err = review.loadReviewerTeam(sess); err != nil {
+			if !IsErrTeamNotExist(err) {
 				return nil, err
 			}
 		} else {
+			review.ReviewerID = -review.ReviewerTeamID
 			reviews = append(reviews, review)
 		}
 	}
@@ -479,6 +515,28 @@ func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Rev
 	return
 }
 
+// GetTeamReviewerByIssueIDAndTeamID get the latest review requst of reviewer team for a pull request
+func GetTeamReviewerByIssueIDAndTeamID(issueID, teamID int64) (review *Review, err error) {
+	return getTeamReviewerByIssueIDAndTeamID(x, issueID, teamID)
+}
+
+func getTeamReviewerByIssueIDAndTeamID(e Engine, issueID, teamID int64) (review *Review, err error) {
+	review = new(Review)
+
+	has := false
+	if has, err = e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = ?)",
+		issueID, teamID).
+		Get(review); err != nil {
+		return nil, err
+	}
+
+	if !has {
+		return nil, ErrReviewNotExist{0}
+	}
+
+	return
+}
+
 // MarkReviewsAsStale marks existing reviews as stale
 func MarkReviewsAsStale(issueID int64) (err error) {
 	_, err = x.Exec("UPDATE `review` SET stale=? WHERE issue_id=?", true, issueID)
@@ -668,13 +726,13 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment,
 
 // AddTeamReviewRequest add a review request from one team
 func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
-	review, err := GetReviewerByIssueIDAndUserID(issue.ID, -reviewer.ID)
-	if err != nil {
+	review, err := GetTeamReviewerByIssueIDAndTeamID(issue.ID, reviewer.ID)
+	if err != nil && !IsErrReviewNotExist(err) {
 		return
 	}
 
-	// skip it when reviewer hase been request to review
-	if review != nil && review.Type == ReviewTypeRequest {
+	// skip it when reviewer team hase been request to review
+	if review != nil {
 		return nil, nil
 	}
 
@@ -699,31 +757,31 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Co
 		}
 	}
 
-	if official {
-		if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, -reviewer.ID); err != nil {
-			return nil, err
-		}
-	}
-
 	_, err = createReview(sess, CreateReviewOptions{
-		Type:     ReviewTypeRequest,
-		Issue:    issue,
-		Reviewer: &User{ID: -reviewer.ID},
-		Official: official,
-		Stale:    false,
+		Type:         ReviewTypeRequest,
+		Issue:        issue,
+		ReviewerTeam: reviewer,
+		Official:     official,
+		Stale:        false,
 	})
 
 	if err != nil {
 		return
 	}
 
+	if official {
+		if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id = ?", false, issue.ID, reviewer.ID); err != nil {
+			return nil, err
+		}
+	}
+
 	comment, err = createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
-		RemovedAssignee: false,        // Use RemovedAssignee as !isRequest
-		AssigneeID:      -reviewer.ID, // Use AssigneeID as reviewer ID
+		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
+		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
 	})
 
 	if err != nil {
@@ -735,12 +793,12 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Co
 
 //RemoveTeamReviewRequest remove a review request from one team
 func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
-	review, err := GetReviewerByIssueIDAndUserID(issue.ID, -reviewer.ID)
-	if err != nil {
+	review, err := GetTeamReviewerByIssueIDAndTeamID(issue.ID, reviewer.ID)
+	if err != nil && !IsErrReviewNotExist(err) {
 		return
 	}
 
-	if review.Type != ReviewTypeRequest {
+	if review == nil {
 		return nil, nil
 	}
 
@@ -790,8 +848,8 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
-		RemovedAssignee: true,         // Use RemovedAssignee as !isRequest
-		AssigneeID:      -reviewer.ID, // Use AssigneeID as reviewer ID
+		RemovedAssignee: true,        // Use RemovedAssignee as !isRequest
+		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
 	})
 
 	if err != nil {
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index c04b0c3714a53..11f60985104e5 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -1138,8 +1138,8 @@ func ViewIssue(ctx *context.Context) {
 			}
 
 		} else if comment.Type == models.CommentTypeAssignees || comment.Type == models.CommentTypeReviewRequest {
-			if err = comment.LoadAssigneeUser(); err != nil {
-				ctx.ServerError("LoadAssigneeUser", err)
+			if err = comment.LoadAssigneeUserAndTeam(); err != nil {
+				ctx.ServerError("LoadAssigneeUserAndTeam", err)
 				return
 			}
 		} else if comment.Type == models.CommentTypeRemoveDependency || comment.Type == models.CommentTypeAddDependency {
diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl
index 41b1599e5eb4e..b3403592e74b9 100644
--- a/templates/repo/issue/view_content/comments.tmpl
+++ b/templates/repo/issue/view_content/comments.tmpl
@@ -605,9 +605,9 @@
 					{{end}}
 				{{else}}
 					{{if .RemovedAssignee}}
-						{{$.i18n.Tr "repo.issues.review.remove_review_request" (.TeamAssignee.Name|Escape) $createdStr | Safe}}
+						{{$.i18n.Tr "repo.issues.review.remove_review_request" (.AssigneeTeam.Name|Escape) $createdStr | Safe}}
 					{{else}}
-						{{$.i18n.Tr "repo.issues.review.add_review_request" (.TeamAssignee.Name|Escape) $createdStr | Safe}}
+						{{$.i18n.Tr "repo.issues.review.add_review_request" (.AssigneeTeam.Name|Escape) $createdStr | Safe}}
 					{{end}}
 				{{end}}
 			</span>
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index 2dd68fe662ca0..ce27a96b5de1b 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -76,7 +76,7 @@
 							{{$canChoose = true}}
 						{{end}}
 
-						<a class="{{if not $canChoose}}ui poping up{{end}} item {{if $checked}} checked {{end}}" href="#" data-id="{{.ID}}" data-id-selector="#review_request_{{.ID}}" data-can-change="{{if not $canChoose}}block{{end}}" {{if not $canChoose}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}} data-is-checked="{{if $checked}}add{{else}}remove{{end}}">
+						<a class="{{if not $canChoose}}ui poping up{{end}} item {{if $checked}} checked {{end}} {{if not $canChoose}}ban-change{{end}}" href="#" data-id="{{.ID}}" data-id-selector="#review_request_{{.ID}}" {{if not $canChoose}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
 							<span class="octicon-check {{if not $checked}}invisible{{end}}">{{svg "octicon-check" 16}}</span>
 							<span class="ui text">
 								{{$.Issue.Repo.OwnerName}}/{{.Name}}

From 66a0f478e71decd6e0ef01c6ead1503f2ffdd661 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Thu, 17 Sep 2020 17:29:31 +0800
Subject: [PATCH 05/24] fix lint

---
 models/migrations/v152.go | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/models/migrations/v152.go b/models/migrations/v152.go
index 4ef8898b1ce86..a73a591256770 100644
--- a/models/migrations/v152.go
+++ b/models/migrations/v152.go
@@ -21,9 +21,6 @@ func addTeamReviewRequestSupport(x *xorm.Engine) error {
 		return err
 	}
 
-	if err := x.Sync2(new(Comment)); err != nil {
-		return err
-	}
-
-	return nil
+	err := x.Sync2(new(Comment))
+	return err
 }

From f36a5ec99aad5158684c53271c8a6cbfd96968b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com>
Date: Mon, 28 Sep 2020 17:42:54 +0800
Subject: [PATCH 06/24] Update models/migrations/v153.go

Co-authored-by: Lauris BH <lauris@nix.lv>
---
 models/migrations/v153.go | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/models/migrations/v153.go b/models/migrations/v153.go
index a73a591256770..1e5ae9f7da47b 100644
--- a/models/migrations/v153.go
+++ b/models/migrations/v153.go
@@ -21,6 +21,5 @@ func addTeamReviewRequestSupport(x *xorm.Engine) error {
 		return err
 	}
 
-	err := x.Sync2(new(Comment))
-	return err
+	return x.Sync2(new(Comment))
 }

From 26eb8c41e1b65d285cbbcc19e5fc065e6cf3c8a5 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 29 Sep 2020 13:08:46 +0800
Subject: [PATCH 07/24] Apply suggestion from code review @lafriks

* Fix getReviewers() return
* Simplify ui logic

* Fix some bugs about Original author
---
 models/repo.go                                |  18 +-
 models/review.go                              |  49 ++---
 routers/repo/issue.go                         | 208 ++++++++++++++++--
 templates/repo/issue/view_content/pull.tmpl   |  55 ++---
 .../repo/issue/view_content/sidebar.tmpl      | 108 +++------
 web_src/js/index.js                           |   2 +-
 6 files changed, 263 insertions(+), 177 deletions(-)

diff --git a/models/repo.go b/models/repo.go
index ee51f7907a70e..db55c7bbf9242 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -729,26 +729,36 @@ func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_
 	return users, nil
 }
 
-func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, teams []*Team, err error) {
-	if err = repo.getOwner(e); err != nil {
+func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, []*Team, error) {
+	if err := repo.getOwner(e); err != nil {
 		return nil, nil, err
 	}
 
+	var (
+		users []*User
+		err   error
+	)
+
 	if repo.IsPrivate ||
 		(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
 		users, err = repo.getReviewersPrivate(x, doerID, posterID)
 	} else {
 		users, err = repo.getReviewersPublic(x, doerID, posterID)
 	}
+	if err != nil {
+		return nil, nil, err
+	}
 
 	if repo.Owner.IsOrganization() {
-		teams, err = GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
+		teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
 		if err != nil {
 			return nil, nil, err
 		}
+
+		return users, teams, nil
 	}
 
-	return
+	return users, nil, nil
 }
 
 // GetReviewers get all users can be requested to review
diff --git a/models/review.go b/models/review.go
index 9c9a56131bee3..93f49ceb6604b 100644
--- a/models/review.go
+++ b/models/review.go
@@ -122,6 +122,11 @@ func (r *Review) LoadReviewer() error {
 	return r.loadReviewer(x)
 }
 
+// LoadReviewerTeam loads reviewer team
+func (r *Review) LoadReviewerTeam() error {
+	return r.loadReviewerTeam(x)
+}
+
 func (r *Review) loadAttributes(e Engine) (err error) {
 	if err = r.loadIssue(e); err != nil {
 		return
@@ -446,53 +451,25 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 }
 
 // GetReviewersByIssueID gets the latest review of each reviewer for a pull request
-func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
-	reviewsUnfiltered := []*Review{}
-
-	sess := x.NewSession()
-	defer sess.Close()
-	if err := sess.Begin(); err != nil {
-		return nil, err
-	}
+func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
+	reviews := make([]*Review, 0, 10)
 
 	// Get latest review of each reviwer, sorted in order they were made
-	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
+	if err := x.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
 		issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
-		Find(&reviewsUnfiltered); err != nil {
+		Find(&reviews); err != nil {
 		return nil, err
 	}
 
-	// Load reviewer and skip if user is deleted
-	for _, review := range reviewsUnfiltered {
-		if err = review.loadReviewer(sess); err != nil {
-			if !IsErrUserNotExist(err) {
-				return nil, err
-			}
-		} else {
-			reviews = append(reviews, review)
-		}
-	}
-
 	teamReviewRequests := make([]*Review, 0, 5)
-	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
+	if err := x.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 AND original_author_id = 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
 		issueID).
 		Find(&teamReviewRequests); err != nil {
 		return nil, err
 	}
 
-	if len(teamReviewRequests) == 0 {
-		return reviews, nil
-	}
-
-	for _, review := range teamReviewRequests {
-		if err = review.loadReviewerTeam(sess); err != nil {
-			if !IsErrTeamNotExist(err) {
-				return nil, err
-			}
-		} else {
-			review.ReviewerID = -review.ReviewerTeamID
-			reviews = append(reviews, review)
-		}
+	if len(teamReviewRequests) > 0 {
+		reviews = append(reviews, teamReviewRequests...)
 	}
 
 	return reviews, nil
@@ -506,7 +483,7 @@ func GetReviewerByIssueIDAndUserID(issueID, userID int64) (review *Review, err e
 func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Review, err error) {
 	review = new(Review)
 
-	if _, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND type in (?, ?, ?))",
+	if _, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
 		issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
 		Get(review); err != nil {
 		return nil, err
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index e8aaad2a448f9..45db5b617d196 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -434,23 +434,195 @@ func retrieveProjects(ctx *context.Context, repo *models.Repository) {
 	}
 }
 
+// ReviewerChoseItem items to bee shown
+type ReviewerChoseItem struct {
+	IsTeam    bool
+	Team      *models.Team
+	User      *models.User
+	Review    *models.Review
+	CanChange bool
+	Checked   bool
+	ItemID    int64
+}
+
 // RetrieveRepoReviewers find all reviewers of a repository
-func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issuePosterID int64) {
+func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue *models.Issue, canChooseReviewer bool) {
+	ctx.Data["CanChooseReviewer"] = canChooseReviewer
+
+	reviews, err := models.GetReviewersByIssueID(issue.ID)
+	if err != nil {
+		ctx.ServerError("GetReviewersByIssueID", err)
+		return
+	}
+
 	var (
-		err           error
-		teamReviewers []*models.Team
+		pullReviews         []*ReviewerChoseItem
+		reviewersResult     []*ReviewerChoseItem
+		teamReviewersResult []*ReviewerChoseItem
+		teamReviewers       []*models.Team
+		reviewers           []*models.User
 	)
-	ctx.Data["Reviewers"], teamReviewers, err = repo.GetReviewers(ctx.User.ID, issuePosterID)
-	if err != nil {
-		ctx.ServerError("GetReviewers", err)
+
+	if canChooseReviewer {
+		posterID := issue.PosterID
+		if issue.OriginalAuthorID > 0 {
+			posterID = 0
+		}
+		reviewers, teamReviewers, err = repo.GetReviewers(ctx.User.ID, posterID)
+		if err != nil {
+			ctx.ServerError("GetReviewers", err)
+			return
+		}
+		if len(reviewers) > 0 {
+			reviewersResult = make([]*ReviewerChoseItem, 0, len(reviewers))
+		}
+		if len(teamReviewers) > 0 {
+			teamReviewersResult = make([]*ReviewerChoseItem, 0, len(teamReviewers))
+		}
+	}
+
+	if len(reviews) != 0 {
+		pullReviews = make([]*ReviewerChoseItem, 0, len(reviews))
+
+		for _, review := range reviews {
+			tmp := &ReviewerChoseItem{
+				Checked: review.Type == models.ReviewTypeRequest,
+				Review:  review,
+				ItemID:  review.ReviewerID,
+			}
+			if review.ReviewerTeamID > 0 {
+				tmp.IsTeam = true
+				tmp.ItemID = -review.ReviewerTeamID
+			}
+			if ctx.User != nil && ctx.User.ID == review.ReviewerID && review.Type == models.ReviewTypeRequest {
+				// all user can refuse the review request to them
+				tmp.CanChange = true
+			} else if (canChooseReviewer || (ctx.User != nil && ctx.User.ID == issue.PosterID)) && review.Type != models.ReviewTypeRequest &&
+				ctx.User.ID != review.ReviewerID {
+				// manager and official reviewers call re-requst review from other reviewer
+				tmp.CanChange = true
+			} else if ctx.User != nil && ctx.Repo.IsAdmin() && ctx.User.ID != review.ReviewerID {
+				// only admin can dissims review request to other reviewers
+				tmp.CanChange = true
+			}
+
+			pullReviews = append(pullReviews, tmp)
+
+			if canChooseReviewer {
+				if tmp.IsTeam {
+					teamReviewersResult = append(teamReviewersResult, tmp)
+				} else {
+					reviewersResult = append(reviewersResult, tmp)
+				}
+			}
+		}
+	} else {
+		if !canChooseReviewer {
+			return
+		}
+	}
+
+	if !canChooseReviewer {
+		exitsReviers := make([]*ReviewerChoseItem, 0, len(pullReviews))
+		for _, item := range pullReviews {
+			if item.Review.ReviewerID > 0 {
+				if err = item.Review.LoadReviewer(); err != nil {
+					if models.IsErrUserNotExist(err) {
+						continue
+					} else {
+						ctx.ServerError("LoadReviewer", err)
+						return
+					}
+				}
+				item.User = item.Review.Reviewer
+			} else if item.Review.ReviewerTeamID > 0 {
+				if err = item.Review.LoadReviewerTeam(); err != nil {
+					if models.IsErrTeamNotExist(err) {
+						continue
+					} else {
+						ctx.ServerError("LoadReviewerTeam", err)
+						return
+					}
+				}
+				item.Team = item.Review.ReviewerTeam
+			} else {
+				continue
+			}
+
+			exitsReviers = append(exitsReviers, item)
+		}
+		ctx.Data["PullReviewers"] = exitsReviers
 		return
 	}
 
-	for _, team := range teamReviewers {
-		team.ID = -team.ID
+	if reviewersResult != nil {
+		exitLen := len(reviewersResult)
+		for _, reviewer := range reviewers {
+			exist := false
+			for index, tmp := range reviewersResult {
+				if index >= exitLen {
+					break
+				}
+				if tmp.ItemID == reviewer.ID {
+					tmp.User = reviewer
+					exist = true
+				}
+			}
+			if !exist {
+				reviewersResult = append(reviewersResult, &ReviewerChoseItem{
+					IsTeam:    false,
+					CanChange: true,
+					User:      reviewer,
+					ItemID:    reviewer.ID,
+				})
+			}
+		}
+
+		ctx.Data["Reviewers"] = reviewersResult
 	}
 
-	ctx.Data["TeamReviewers"] = teamReviewers
+	if teamReviewersResult != nil {
+		exitLen := len(teamReviewersResult)
+		for _, team := range teamReviewers {
+			exist := false
+			for index, tmp := range teamReviewersResult {
+				if index >= exitLen {
+					break
+				}
+				if tmp.ItemID == -team.ID {
+					tmp.Team = team
+					exist = true
+				}
+			}
+			if !exist {
+				teamReviewersResult = append(teamReviewersResult, &ReviewerChoseItem{
+					IsTeam:    true,
+					CanChange: true,
+					Team:      team,
+					ItemID:    -team.ID,
+				})
+			}
+		}
+
+		ctx.Data["TeamReviewers"] = teamReviewersResult
+	}
+
+	if len(pullReviews) > 0 {
+		exitsReviers := make([]*ReviewerChoseItem, 0, len(pullReviews))
+		for _, item := range pullReviews {
+			if ctx.User != nil && item.ItemID == ctx.User.ID {
+				if err = item.Review.LoadReviewer(); err != nil {
+					ctx.ServerError("LoadReviewer", err)
+					return
+				}
+				item.User = item.Review.Reviewer
+			}
+			if item.Team != nil || item.User != nil {
+				exitsReviers = append(exitsReviers, item)
+			}
+		}
+		ctx.Data["PullReviewers"] = exitsReviers
+	}
 }
 
 // RetrieveRepoMetas find all the meta information of a repository
@@ -988,13 +1160,7 @@ func ViewIssue(ctx *context.Context) {
 			}
 		}
 
-		if canChooseReviewer {
-			RetrieveRepoReviewers(ctx, repo, issue.PosterID)
-			ctx.Data["CanChooseReviewer"] = true
-		} else {
-			ctx.Data["CanChooseReviewer"] = false
-		}
-
+		RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer)
 		if ctx.Written() {
 			return
 		}
@@ -1286,12 +1452,6 @@ func ViewIssue(ctx *context.Context) {
 			pull.HeadRepo != nil &&
 			git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) &&
 			(!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"])
-
-		ctx.Data["PullReviewers"], err = models.GetReviewersByIssueID(issue.ID)
-		if err != nil {
-			ctx.ServerError("GetReviewersByIssueID", err)
-			return
-		}
 	}
 
 	// Get Dependencies
@@ -1563,7 +1723,7 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 			return fmt.Errorf("Reviewer can't read [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
 		}
 
-		if doer.ID == issue.PosterID && lastreview != nil && lastreview.Type != models.ReviewTypeRequest {
+		if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != models.ReviewTypeRequest {
 			return nil
 		}
 
@@ -1582,7 +1742,7 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 			return fmt.Errorf("doer can't be reviewer [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
 		}
 
-		if reviewer.ID == issue.PosterID {
+		if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 {
 			return fmt.Errorf("poster of pr can't be reviewer [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
 		}
 	} else {
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index be1462037efe8..b9be5327870fb 100644
--- a/templates/repo/issue/view_content/pull.tmpl
+++ b/templates/repo/issue/view_content/pull.tmpl
@@ -1,31 +1,31 @@
-{{if gt (len .PullReviewers) 0}}
+{{if .PullReviewers }}
 	<div class="comment box">
 		<div class="content">
 			<div class="ui segment">
 				<h4>{{$.i18n.Tr "repo.issues.review.reviewers"}}</h4>
 				{{range .PullReviewers}}
-					{{ $createdStr:= TimeSinceUnix .UpdatedUnix $.Lang }}
+					{{ $createdStr:= TimeSinceUnix .Review.UpdatedUnix $.Lang }}
 					<div class="ui divider"></div>
 					<div class="review-item">
 						<div class="review-item-left">
-							{{if (gt .ReviewerID 0)}}
-							<a class="ui avatar image" href="{{.Reviewer.HomeLink}}">
-								<img src="{{.Reviewer.RelAvatarLink}}">
-							</a>
+							{{if .User}}
+								<a class="ui avatar image" href="{{.User.HomeLink}}">
+									<img src="{{.User.RelAvatarLink}}">
+								</a>
 							{{end}}
 							<span class="text grey">
-								{{if (gt .ReviewerID 0)}}
-									<a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a>
-								{{else}}
-									<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.ReviewerTeam.Name}}</span>
+								{{if .User}}
+									<a href="{{.User.HomeLink}}">{{.User.Name}}</a>
+								{{else if .Team}}
+									<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
 								{{end}}
-								{{if eq .Type 1}}
+								{{if eq .Review.Type 1}}
 									{{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}}
-								{{else if eq .Type 2}}
+								{{else if eq .Review.Type 2}}
 									{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
-								{{else if eq .Type 3}}
+								{{else if eq .Review.Type 3}}
 									{{$.i18n.Tr "repo.issues.review.reject" $createdStr | Safe}}
-								{{else if eq .Type 4}}
+								{{else if eq .Review.Type 4}}
 									{{$.i18n.Tr "repo.issues.review.wait" $createdStr | Safe}}
 								{{else}}
 									{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
@@ -33,34 +33,23 @@
 							</span>
 						</div>
 						<div class="review-item-right">
-							{{if .Stale}}
+							{{if .Review.Stale}}
 							<span class="ui poping up type-icon text grey" data-content="{{$.i18n.Tr "repo.issues.is_stale"}}">
 								<i class="octicon icon fa-hourglass-end"></i>
 							</span>
 							{{end}}
-							<span class="type-icon text {{if eq .Type 1}}green
-								{{- else if eq .Type 2}}grey
-								{{- else if eq .Type 3}}red
-								{{- else if eq .Type 4}}yellow
+							<span class="type-icon text {{if eq .Review.Type 1}}green
+								{{- else if eq .Review.Type 2}}grey
+								{{- else if eq .Review.Type 3}}red
+								{{- else if eq .Review.Type 4}}yellow
 								{{else}}grey{{end}}">
 
-								{{$canChoose := false}}
-								{{if eq .Type 4}}
-									{{if or (and (eq .ReviewerID $.SignedUserID) (gt .ReviewerID 0)) $.Permission.IsAdmin}}
-										{{$canChoose = true}}
-									{{end}}
-								{{else}}
-									{{if and (or $.IsIssuePoster $.CanChooseReviewer) (not (eq $.SignedUserID .ReviewerID))}}
-										{{$canChoose = true}}
-									{{end}}
-								{{end}}
-
-								{{if $canChoose }}
-									<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if  eq .Type 4}}true{{else}}false{{end}}" data-issue-id="{{$.Issue.ID}}" data-content="{{ if eq .Type 4 }} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}"  data-id="{{.ReviewerID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
+								{{if .CanChange }}
+									<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if .Checked}}true{{else}}false{{end}}" data-issue-id="{{$.Issue.ID}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}"  data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
 										{{svg "octicon-sync"}}
 									</a>
 								{{end}}
-								{{svg (printf "octicon-%s" .Type.Icon)}}
+								{{svg (printf "octicon-%s" .Review.Type.Icon)}}
 							</span>
 						</div>
 					</div>
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index ce27a96b5de1b..683427d7586cb 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -5,7 +5,7 @@
 		{{if .Issue.IsPull }}
 
 		<input id="reviewer_id" name="reviewer_id" type="hidden" value="{{.reviewer_id}}">
-		<div class="ui {{if or (not .CanChooseReviewer) .Repository.IsArchived}}disabled{{end}} floating jump select-reviewers-modify dropdown">
+		<div class="ui {{if or (not .Reviewers) (not .CanChooseReviewer) .Repository.IsArchived}}disabled{{end}} floating jump select-reviewers-modify dropdown">
 			<span class="text">
 				<strong>{{.i18n.Tr "repo.issues.review.reviewers"}}</strong>
 				{{if and .CanChooseReviewer (not .Repository.IsArchived)}}
@@ -20,68 +20,29 @@
 						<input type="text" placeholder="{{.i18n.Tr "repo.issues.filter_reviewers"}}">
 					</div>
 				{{end}}
-				{{range .Reviewers}}
-					{{$ReviewerID := .ID}}
-					{{$checked := false}}
-					{{$canChoose := false}}
-					{{$notReviewed := true}}
-
-					{{range $.PullReviewers}}
-						{{if eq .ReviewerID $ReviewerID }}
-							{{$notReviewed = false }}
-							{{if  eq .Type 4 }}
-								{{$checked = true}}
-								{{if or (eq $ReviewerID $.SignedUserID) $.Permission.IsAdmin}}
-									{{$canChoose = true}}
-								{{end}}
-							{{else}}
-								{{$canChoose = true}}
-							{{end}}
+				{{if .Reviewers}}
+					{{range .Reviewers}}
+						{{if .User}}
+							<a class="{{if not .CanChange}}ui poping up{{end}} item {{if .Checked}} checked {{end}} {{if not .CanChange}}ban-change{{end}}" href="#" data-id="{{.ItemID}}" data-id-selector="#review_request_{{.ItemID}}" {{if not .CanChange}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
+								<span class="octicon-check {{if not .Checked}}invisible{{end}}">{{svg "octicon-check"}}</span>
+								<span class="text">
+									<img class="ui avatar image" src="{{.User.RelAvatarLink}}"> {{.User.GetDisplayName}}
+								</span>
+							</a>
 						{{end}}
 					{{end}}
-
-					{{ if $notReviewed}}
-						{{$canChoose = true}}
-					{{end}}
-
-					<a class="{{if not $canChoose}}ui poping up{{end}} item {{if $checked}} checked {{end}} {{if not $canChoose}}ban-change{{end}}" href="#" data-id="{{.ID}}" data-id-selector="#review_request_{{.ID}}" {{if not $canChoose}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
-						<span class="octicon-check {{if not $checked}}invisible{{end}}">{{svg "octicon-check"}}</span>
-						<span class="text">
-							<img class="ui avatar image" src="{{.RelAvatarLink}}"> {{.GetDisplayName}}
-						</span>
-					</a>
 				{{end}}
 				{{if .TeamReviewers}}
+					<div class="ui divider"></div>
 					{{range .TeamReviewers}}
-						{{$ReviewerID := .ID}}
-						{{$checked := false}}
-						{{$canChoose := false}}
-						{{$notReviewed := true}}
-
-						{{range $.PullReviewers}}
-							{{if eq .ReviewerID $ReviewerID }}
-								{{$notReviewed = false }}
-								{{if  eq .Type 4 }}
-									{{$checked = true}}
-									{{if $.Permission.IsAdmin}}
-										{{$canChoose = true}}
-									{{end}}
-								{{else}}
-									{{$canChoose = true}}
-								{{end}}
-							{{end}}
+						{{if .Team}}
+							<a class="{{if not .CanChange}}ui poping up{{end}} item {{if .Checked}} checked {{end}} {{if not .CanChange}}ban-change{{end}}" href="#" data-id="{{.ItemID}}" data-id-selector="#review_request_team_{{.Team.ID}}" {{if not .CanChange}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
+								<span class="octicon-check {{if not .Checked}}invisible{{end}}">{{svg "octicon-check" 16}}</span>
+								<span class="ui text">
+									{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}
+								</span>
+							</a>
 						{{end}}
-
-						{{ if $notReviewed}}
-							{{$canChoose = true}}
-						{{end}}
-
-						<a class="{{if not $canChoose}}ui poping up{{end}} item {{if $checked}} checked {{end}} {{if not $canChoose}}ban-change{{end}}" href="#" data-id="{{.ID}}" data-id-selector="#review_request_{{.ID}}" {{if not $canChoose}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
-							<span class="octicon-check {{if not $checked}}invisible{{end}}">{{svg "octicon-check" 16}}</span>
-							<span class="ui text">
-								{{$.Issue.Repo.OwnerName}}/{{.Name}}
-							</span>
-						</a>
 					{{end}}
 				{{end}}
 			</div>
@@ -92,34 +53,23 @@
 			<div class="selected">
 				{{range .PullReviewers}}
 					<div class="item" style="margin-bottom: 10px;">
-						{{if (gt .ReviewerID 0)}}
-							<a href="{{.Reviewer.HomeLink}}"><img class="ui avatar image" src="{{.Reviewer.RelAvatarLink}}">&nbsp;{{.Reviewer.GetDisplayName}}</a>
-						{{else}}
-							<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.ReviewerTeam.Name}}</span>
+						{{if .User}}
+							<a href="{{.User.HomeLink}}"><img class="ui avatar image" src="{{.User.RelAvatarLink}}">&nbsp;{{.User.GetDisplayName}}</a>
+						{{else if .Team}}
+							<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
 						{{end}}
-						<span class="ui right type-icon text {{if eq .Type 1}}green
-							{{- else if eq .Type 2}}grey
-							{{- else if eq .Type 3}}red
-							{{- else if eq .Type 4}}yellow
+						<span class="ui right type-icon text {{if eq .Review.Type 1}}green
+							{{- else if eq .Review.Type 2}}grey
+							{{- else if eq .Review.Type 3}}red
+							{{- else if eq .Review.Type 4}}yellow
 							{{- else}}grey{{end}} right ">
 
-							{{$canChoose := false}}
-							{{if eq .Type 4}}
-								{{if or (and (eq .ReviewerID $.SignedUserID) (gt .ReviewerID 0) ) $.Permission.IsAdmin}}
-									{{$canChoose = true}}
-								{{end}}
-							{{else}}
-								{{if and (or $.IsIssuePoster $.CanChooseReviewer) (not (eq $.SignedUserID .ReviewerID))}}
-									{{$canChoose = true}}
-								{{end}}
-							{{end}}
-
-							{{if $canChoose}}
-								<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if  eq .Type 4}}true{{else}}false{{end}}" data-content="{{ if eq .Type 4 }} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-issue-id="{{$.Issue.ID}}"  data-id="{{.ReviewerID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
+							{{if .CanChange}}
+								<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if .Checked}}true{{else}}false{{end}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-issue-id="{{$.Issue.ID}}"  data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
 									{{svg "octicon-sync"}}
 								</a>
 							{{end}}
-							{{svg (printf "octicon-%s" .Type.Icon)}}
+							{{svg (printf "octicon-%s" .Review.Type.Icon)}}
 						</span>
 					</div>
 				{{end}}
diff --git a/web_src/js/index.js b/web_src/js/index.js
index 415db385b39e6..e89b8a9111a3f 100644
--- a/web_src/js/index.js
+++ b/web_src/js/index.js
@@ -668,7 +668,7 @@ function initIssueComments() {
     event.preventDefault();
     updateIssuesMeta(
       url,
-      isChecked === 'true' ? 'attach' : 'detach',
+      isChecked === 'true' ? 'detach' : 'attach',
       issueId,
       id,
     ).then(reload);

From 515d09d5406148066429405fb1a14d53dba8c4ae Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 29 Sep 2020 13:18:59 +0800
Subject: [PATCH 08/24] fix lint

---
 routers/repo/issue.go | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 45db5b617d196..0d82c63c36f40 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -516,10 +516,8 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 				}
 			}
 		}
-	} else {
-		if !canChooseReviewer {
-			return
-		}
+	} else if !canChooseReviewer {
+		return
 	}
 
 	if !canChooseReviewer {

From 3f8fee58332a1d3568888ea0ee0a18dc959c5b4a Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 29 Sep 2020 13:36:51 +0800
Subject: [PATCH 09/24] fix test

---
 models/fixtures/review.yml | 4 ++++
 models/review_test.go      | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml
index 35d3dee2e6b06..3db0b473532a4 100644
--- a/models/fixtures/review.yml
+++ b/models/fixtures/review.yml
@@ -44,6 +44,7 @@
   reviewer_id: 2
   issue_id: 3
   content: "New review 3"
+  original_author_id: 0
   updated_unix: 946684811
   created_unix: 946684811
 -
@@ -52,6 +53,7 @@
   reviewer_id: 3
   issue_id: 3
   content: "New review 4"
+  original_author_id: 0
   updated_unix: 946684812
   created_unix: 946684812
 -
@@ -59,6 +61,7 @@
   type: 1
   reviewer_id: 4
   issue_id: 3
+  original_author_id: 0
   content: "New review 5"
   commit_id: 8091a55037cd59e47293aca02981b5a67076b364
   stale: true
@@ -72,6 +75,7 @@
   content: "New review 3 rejected"
   updated_unix: 946684814
   created_unix: 946684814
+  original_author_id: 0
 
 -
   id: 10
diff --git a/models/review_test.go b/models/review_test.go
index 7103962ce9a98..702e216824041 100644
--- a/models/review_test.go
+++ b/models/review_test.go
@@ -130,6 +130,9 @@ func TestGetReviewersByIssueID(t *testing.T) {
 		})
 
 	allReviews, err := GetReviewersByIssueID(issue.ID)
+	for _, reviewer := range allReviews {
+		assert.NoError(t, reviewer.LoadReviewer())
+	}
 	assert.NoError(t, err)
 	if assert.Len(t, allReviews, 3) {
 		for i, review := range allReviews {

From 39f0b94b379a353a0fffc831515238d3d5f623b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com>
Date: Mon, 5 Oct 2020 23:17:53 +0800
Subject: [PATCH 10/24] Update models/repo.go

Co-authored-by: Lauris BH <lauris@nix.lv>
---
 models/repo.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/models/repo.go b/models/repo.go
index db55c7bbf9242..7c129d108c1ab 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -766,7 +766,7 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User,
 // but for public rpo, that return all users that have write access or higher to the repository,
 // and all repo watchers.
 // TODO: may be we should hava a busy choice for users to block review request to them.
-func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, _ []*Team, err error) {
+func (repo *Repository) GetReviewers(doerID, posterID int64) ([]*User, []*Team, error) {
 	return repo.getReviewers(x, doerID, posterID)
 }
 

From 94ee9d5dd3f0853b8af4b51c0938992e38e66dad Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Mon, 5 Oct 2020 23:24:44 +0800
Subject: [PATCH 11/24] Apply suggestion from code review

Co-authored-by: Lauris BH <lauris@nix.lv>
---
 routers/repo/issue.go | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 0d82c63c36f40..c6b57cdab1492 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -434,8 +434,8 @@ func retrieveProjects(ctx *context.Context, repo *models.Repository) {
 	}
 }
 
-// ReviewerChoseItem items to bee shown
-type ReviewerChoseItem struct {
+// repoReviewerSelection items to bee shown
+type repoReviewerSelection struct {
 	IsTeam    bool
 	Team      *models.Team
 	User      *models.User
@@ -456,9 +456,9 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 	}
 
 	var (
-		pullReviews         []*ReviewerChoseItem
-		reviewersResult     []*ReviewerChoseItem
-		teamReviewersResult []*ReviewerChoseItem
+		pullReviews         []*repoReviewerSelection
+		reviewersResult     []*repoReviewerSelection
+		teamReviewersResult []*repoReviewerSelection
 		teamReviewers       []*models.Team
 		reviewers           []*models.User
 	)
@@ -474,18 +474,18 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 			return
 		}
 		if len(reviewers) > 0 {
-			reviewersResult = make([]*ReviewerChoseItem, 0, len(reviewers))
+			reviewersResult = make([]*repoReviewerSelection, 0, len(reviewers))
 		}
 		if len(teamReviewers) > 0 {
-			teamReviewersResult = make([]*ReviewerChoseItem, 0, len(teamReviewers))
+			teamReviewersResult = make([]*repoReviewerSelection, 0, len(teamReviewers))
 		}
 	}
 
 	if len(reviews) != 0 {
-		pullReviews = make([]*ReviewerChoseItem, 0, len(reviews))
+		pullReviews = make([]*repoReviewerSelection, 0, len(reviews))
 
 		for _, review := range reviews {
-			tmp := &ReviewerChoseItem{
+			tmp := &repoReviewerSelection{
 				Checked: review.Type == models.ReviewTypeRequest,
 				Review:  review,
 				ItemID:  review.ReviewerID,
@@ -521,7 +521,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 	}
 
 	if !canChooseReviewer {
-		exitsReviers := make([]*ReviewerChoseItem, 0, len(pullReviews))
+		exitsReviers := make([]*repoReviewerSelection, 0, len(pullReviews))
 		for _, item := range pullReviews {
 			if item.Review.ReviewerID > 0 {
 				if err = item.Review.LoadReviewer(); err != nil {
@@ -567,7 +567,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 				}
 			}
 			if !exist {
-				reviewersResult = append(reviewersResult, &ReviewerChoseItem{
+				reviewersResult = append(reviewersResult, &repoReviewerSelection{
 					IsTeam:    false,
 					CanChange: true,
 					User:      reviewer,
@@ -593,7 +593,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 				}
 			}
 			if !exist {
-				teamReviewersResult = append(teamReviewersResult, &ReviewerChoseItem{
+				teamReviewersResult = append(teamReviewersResult, &repoReviewerSelection{
 					IsTeam:    true,
 					CanChange: true,
 					Team:      team,
@@ -606,7 +606,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 	}
 
 	if len(pullReviews) > 0 {
-		exitsReviers := make([]*ReviewerChoseItem, 0, len(pullReviews))
+		exitsReviers := make([]*repoReviewerSelection, 0, len(pullReviews))
 		for _, item := range pullReviews {
 			if ctx.User != nil && item.ItemID == ctx.User.ID {
 				if err = item.Review.LoadReviewer(); err != nil {

From 7d75af09cc5851efb74ae2f36d991a637a57b5b8 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Tue, 6 Oct 2020 10:51:58 +0800
Subject: [PATCH 12/24] splite get reviewers and add some test

---
 models/repo.go        | 36 ++++++++++++++++++++++--------------
 models/repo_test.go   | 31 +++++++++++++++++++++++++++++++
 routers/repo/issue.go |  7 ++++++-
 3 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/models/repo.go b/models/repo.go
index 0aef08c49cf68..aacf8d6f9daf3 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -729,9 +729,9 @@ func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_
 	return users, nil
 }
 
-func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, []*Team, error) {
+func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, error) {
 	if err := repo.getOwner(e); err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	var (
@@ -746,19 +746,10 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User,
 		users, err = repo.getReviewersPublic(x, doerID, posterID)
 	}
 	if err != nil {
-		return nil, nil, err
-	}
-
-	if repo.Owner.IsOrganization() {
-		teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
-		if err != nil {
-			return nil, nil, err
-		}
-
-		return users, teams, nil
+		return nil, err
 	}
 
-	return users, nil, nil
+	return users, nil
 }
 
 // GetReviewers get all users can be requested to review
@@ -766,10 +757,27 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User,
 // but for public rpo, that return all users that have write access or higher to the repository,
 // and all repo watchers.
 // TODO: may be we should hava a busy choice for users to block review request to them.
-func (repo *Repository) GetReviewers(doerID, posterID int64) ([]*User, []*Team, error) {
+func (repo *Repository) GetReviewers(doerID, posterID int64) ([]*User, error) {
 	return repo.getReviewers(x, doerID, posterID)
 }
 
+// GetReviewerTeams get all teams can be requested to review
+func (repo *Repository) GetReviewerTeams() ([]*Team, error) {
+	if err := repo.GetOwner(); err != nil {
+		return nil, err
+	}
+	if !repo.Owner.IsOrganization() {
+		return nil, nil
+	}
+
+	teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
+	if err != nil {
+		return nil, err
+	}
+
+	return teams, err
+}
+
 // GetMilestoneByID returns the milestone belongs to repository by given ID.
 func (repo *Repository) GetMilestoneByID(milestoneID int64) (*Milestone, error) {
 	return GetMilestoneByRepoID(repo.ID, milestoneID)
diff --git a/models/repo_test.go b/models/repo_test.go
index 045f94670bde0..a366772d5c257 100644
--- a/models/repo_test.go
+++ b/models/repo_test.go
@@ -193,3 +193,34 @@ func TestDoctorUserStarNum(t *testing.T) {
 
 	assert.NoError(t, DoctorUserStarNum())
 }
+
+func TestRepoGetReviewers(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	// test public repo
+	repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
+
+	reviewers, err := repo1.GetReviewers(2, 2)
+	assert.NoError(t, err)
+	assert.Equal(t, 4, len(reviewers))
+
+	// test private repo
+	repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
+	reviewers, err = repo2.GetReviewers(2, 2)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(reviewers))
+}
+
+func TestRepoGetReviewerTeams(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
+	teams, err := repo2.GetReviewerTeams()
+	assert.NoError(t, err)
+	assert.Empty(t, teams)
+
+	repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
+	teams, err = repo3.GetReviewerTeams()
+	assert.NoError(t, err)
+	assert.Equal(t, 2, len(teams))
+}
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 8ff6fd692db65..6d7a9bd536763 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -469,11 +469,16 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 		if issue.OriginalAuthorID > 0 {
 			posterID = 0
 		}
-		reviewers, teamReviewers, err = repo.GetReviewers(ctx.User.ID, posterID)
+		reviewers, err = repo.GetReviewers(ctx.User.ID, posterID)
 		if err != nil {
 			ctx.ServerError("GetReviewers", err)
 			return
 		}
+		teamReviewers, err = repo.GetReviewerTeams()
+		if err != nil {
+			ctx.ServerError("GetReviewerTeams", err)
+			return
+		}
 		if len(reviewers) > 0 {
 			reviewersResult = make([]*repoReviewerSelection, 0, len(reviewers))
 		}

From 49ddcc7a1cd64147c408eb392ad7cd249b7fc715 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Wed, 7 Oct 2020 10:39:59 +0800
Subject: [PATCH 13/24] fix code style

---
 models/review.go        | 155 ++++++++++++++++------------------------
 services/pull/review.go |   5 +-
 2 files changed, 65 insertions(+), 95 deletions(-)

diff --git a/models/review.go b/models/review.go
index 93f49ceb6604b..98b3b7e59ec47 100644
--- a/models/review.go
+++ b/models/review.go
@@ -361,14 +361,13 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 			if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
 				return nil, nil, err
 			}
-			official, err = isOfficialReviewer(sess, issue, doer)
-			if err != nil {
+			if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
 				return nil, nil, err
 			}
 		}
 
 		// No current review. Create a new one!
-		review, err = createReview(sess, CreateReviewOptions{
+		if review, err = createReview(sess, CreateReviewOptions{
 			Type:     reviewType,
 			Issue:    issue,
 			Reviewer: doer,
@@ -376,8 +375,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 			Official: official,
 			CommitID: commitID,
 			Stale:    stale,
-		})
-		if err != nil {
+		}); err != nil {
 			return nil, nil, err
 		}
 	} else {
@@ -393,8 +391,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 			if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
 				return nil, nil, err
 			}
-			official, err = isOfficialReviewer(sess, issue, doer)
-			if err != nil {
+			if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
 				return nil, nil, err
 			}
 		}
@@ -571,10 +568,16 @@ func InsertReviews(reviews []*Review) error {
 }
 
 // AddReviewRequest add a review request from one reviewer
-func AddReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err error) {
-	review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
+func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
+	review, err := getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
 	if err != nil {
-		return
+		return nil, err
 	}
 
 	// skip it when reviewer hase been request to review
@@ -582,23 +585,13 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err
 		return nil, nil
 	}
 
-	sess := x.NewSession()
-	defer sess.Close()
-	if err := sess.Begin(); err != nil {
-		return nil, err
-	}
-
 	var official bool
-	official, err = isOfficialReviewer(sess, issue, reviewer)
-
-	if err != nil {
+	if official, err = isOfficialReviewer(sess, issue, reviewer); err != nil {
 		return nil, err
 	}
 
 	if !official {
-		official, err = isOfficialReviewer(sess, issue, doer)
-
-		if err != nil {
+		if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
 			return nil, err
 		}
 	}
@@ -609,28 +602,25 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err
 		}
 	}
 
-	_, err = createReview(sess, CreateReviewOptions{
+	if _, err = createReview(sess, CreateReviewOptions{
 		Type:     ReviewTypeRequest,
 		Issue:    issue,
 		Reviewer: reviewer,
 		Official: official,
 		Stale:    false,
-	})
-
-	if err != nil {
-		return
+	}); err != nil {
+		return nil, err
 	}
 
-	comment, err = createComment(sess, &CreateCommentOptions{
+	var comment *Comment
+	if comment, err = createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
-	})
-
-	if err != nil {
+	}); err != nil {
 		return nil, err
 	}
 
@@ -638,31 +628,29 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err
 }
 
 //RemoveReviewRequest remove a review request from one reviewer
-func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err error) {
-	review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
-	if err != nil {
-		return
-	}
-
-	if review.Type != ReviewTypeRequest {
-		return nil, nil
-	}
-
+func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err := sess.Begin(); err != nil {
 		return nil, err
 	}
 
-	_, err = sess.Delete(review)
+	review, err := getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
 	if err != nil {
 		return nil, err
 	}
 
+	if review.Type != ReviewTypeRequest {
+		return nil, nil
+	}
+
+	if _, err = sess.Delete(review); err != nil {
+		return nil, err
+	}
+
 	var official bool
-	official, err = isOfficialReviewer(sess, issue, reviewer)
-	if err != nil {
-		return
+	if official, err = isOfficialReviewer(sess, issue, reviewer); err != nil {
+		return nil, err
 	}
 
 	if official {
@@ -681,10 +669,7 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment,
 		}
 	}
 
-	if err != nil {
-		return nil, err
-	}
-
+	var comment *Comment
 	comment, err = createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
@@ -702,48 +687,42 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment,
 }
 
 // AddTeamReviewRequest add a review request from one team
-func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
+func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, error) {
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
 	review, err := GetTeamReviewerByIssueIDAndTeamID(issue.ID, reviewer.ID)
 	if err != nil && !IsErrReviewNotExist(err) {
-		return
+		return nil, err
 	}
 
-	// skip it when reviewer team hase been request to review
+	// This team already has been requested to review - therefore skip this.
 	if review != nil {
 		return nil, nil
 	}
 
-	sess := x.NewSession()
-	defer sess.Close()
-	if err := sess.Begin(); err != nil {
-		return nil, err
-	}
-
 	var official bool
-	official, err = isOfficialReviewerTeam(sess, issue, reviewer)
-
-	if err != nil {
-		return nil, err
+	if official, err = isOfficialReviewerTeam(sess, issue, reviewer); err != nil {
+		return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
 	}
 
 	if !official {
-		official, err = isOfficialReviewer(sess, issue, doer)
-
-		if err != nil {
-			return nil, err
+		if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
+			return nil, fmt.Errorf("isOfficialReviewer(): %v", err)
 		}
 	}
 
-	_, err = createReview(sess, CreateReviewOptions{
+	if _, err = createReview(sess, CreateReviewOptions{
 		Type:         ReviewTypeRequest,
 		Issue:        issue,
 		ReviewerTeam: reviewer,
 		Official:     official,
 		Stale:        false,
-	})
-
-	if err != nil {
-		return
+	}); err != nil {
+		return nil, err
 	}
 
 	if official {
@@ -752,27 +731,26 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Co
 		}
 	}
 
-	comment, err = createComment(sess, &CreateCommentOptions{
+	var comment *Comment
+	if comment, err = createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
-	})
-
-	if err != nil {
-		return nil, err
+	}); err != nil {
+		return nil, fmt.Errorf("createComment(): %v", err)
 	}
 
 	return comment, sess.Commit()
 }
 
 //RemoveTeamReviewRequest remove a review request from one team
-func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
+func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, error) {
 	review, err := GetTeamReviewerByIssueIDAndTeamID(issue.ID, reviewer.ID)
 	if err != nil && !IsErrReviewNotExist(err) {
-		return
+		return nil, err
 	}
 
 	if review == nil {
@@ -785,15 +763,13 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment
 		return nil, err
 	}
 
-	_, err = sess.Delete(review)
-	if err != nil {
+	if _, err = sess.Delete(review); err != nil {
 		return nil, err
 	}
 
 	var official bool
-	official, err = isOfficialReviewerTeam(sess, issue, reviewer)
-	if err != nil {
-		return
+	if official, err = isOfficialReviewerTeam(sess, issue, reviewer); err != nil {
+		return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
 	}
 
 	if official {
@@ -812,25 +788,20 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment
 		}
 	}
 
-	if err != nil {
-		return nil, err
-	}
-
 	if doer == nil {
 		return nil, sess.Commit()
 	}
 
-	comment, err = createComment(sess, &CreateCommentOptions{
+	var comment *Comment
+	if comment, err = createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
 		RemovedAssignee: true,        // Use RemovedAssignee as !isRequest
 		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
-	})
-
-	if err != nil {
-		return nil, err
+	}); err != nil {
+		return nil, fmt.Errorf("createComment(): %v", err)
 	}
 
 	return comment, sess.Commit()
diff --git a/services/pull/review.go b/services/pull/review.go
index 09ab3ff567ea3..99afdd73c2150 100644
--- a/services/pull/review.go
+++ b/services/pull/review.go
@@ -68,14 +68,13 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models
 			return nil, err
 		}
 
-		review, err = models.CreateReview(models.CreateReviewOptions{
+		if review, err = models.CreateReview(models.CreateReviewOptions{
 			Type:     models.ReviewTypePending,
 			Reviewer: doer,
 			Issue:    issue,
 			Official: false,
 			CommitID: latestCommitID,
-		})
-		if err != nil {
+		}); err != nil {
 			return nil, err
 		}
 	}

From 10378454f5a2ddf780c765c894eebc306eb0f46b Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Fri, 9 Oct 2020 10:27:38 +0800
Subject: [PATCH 14/24] Apply suggestions from code review

---
 models/review.go | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/models/review.go b/models/review.go
index 98b3b7e59ec47..8e2f8ff7ee10a 100644
--- a/models/review.go
+++ b/models/review.go
@@ -278,6 +278,9 @@ func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
 	if opts.Reviewer != nil {
 		review.ReviewerID = opts.Reviewer.ID
 	} else {
+		if review.Type != ReviewTypeRequest {
+			review.Type = ReviewTypeRequest
+		}
 		review.ReviewerTeamID = opts.ReviewerTeam.ID
 	}
 	if _, err := e.Insert(review); err != nil {
@@ -451,15 +454,18 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
 	reviews := make([]*Review, 0, 10)
 
+	sess := x.NewSession()
+	defer sess.Close()
+
 	// Get latest review of each reviwer, sorted in order they were made
-	if err := x.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
+	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
 		issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
 		Find(&reviews); err != nil {
 		return nil, err
 	}
 
 	teamReviewRequests := make([]*Review, 0, 5)
-	if err := x.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 AND original_author_id = 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
+	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 AND original_author_id = 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
 		issueID).
 		Find(&teamReviewRequests); err != nil {
 		return nil, err
@@ -694,7 +700,7 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
 		return nil, err
 	}
 
-	review, err := GetTeamReviewerByIssueIDAndTeamID(issue.ID, reviewer.ID)
+	review, err := getTeamReviewerByIssueIDAndTeamID(sess, issue.ID, reviewer.ID)
 	if err != nil && !IsErrReviewNotExist(err) {
 		return nil, err
 	}
@@ -748,7 +754,13 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
 
 //RemoveTeamReviewRequest remove a review request from one team
 func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, error) {
-	review, err := GetTeamReviewerByIssueIDAndTeamID(issue.ID, reviewer.ID)
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
+
+	review, err := getTeamReviewerByIssueIDAndTeamID(sess, issue.ID, reviewer.ID)
 	if err != nil && !IsErrReviewNotExist(err) {
 		return nil, err
 	}
@@ -757,12 +769,6 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment
 		return nil, nil
 	}
 
-	sess := x.NewSession()
-	defer sess.Close()
-	if err := sess.Begin(); err != nil {
-		return nil, err
-	}
-
 	if _, err = sess.Delete(review); err != nil {
 		return nil, err
 	}

From c25e7693ff6436ee2749e6340e4c41149349f03e Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Sun, 11 Oct 2020 13:33:48 +0800
Subject: [PATCH 15/24] Apply suggestions from code review

* Rename some function
* Return 403 for bad request
---
 routers/repo/issue.go | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 6d7a9bd536763..df41b4134f851 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -1698,7 +1698,7 @@ func UpdateIssueAssignee(ctx *context.Context) {
 	})
 }
 
-func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models.Issue) error {
+func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models.Issue) error {
 	if reviewer.IsOrganization() {
 		return fmt.Errorf("Organization can't be added as reviewer [user_id: %d, repo_id: %d]", reviewer.ID, issue.PullRequest.BaseRepo.ID)
 	}
@@ -1764,7 +1764,7 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 	return nil
 }
 
-func isLegalTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bool, issue *models.Issue) error {
+func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bool, issue *models.Issue) error {
 	if doer.IsOrganization() {
 		return fmt.Errorf("Organization can't be doer to add reviewer [user_id: %d, repo_id: %d]", doer.ID, issue.PullRequest.BaseRepo.ID)
 	}
@@ -1846,9 +1846,9 @@ func updatePullReviewRequest(ctx *context.Context) {
 						return
 					}
 
-					err = isLegalTeamReviewRequest(team, ctx.User, action == "attach", issue)
+					err = isValidTeamReviewRequest(team, ctx.User, action == "attach", issue)
 					if err != nil {
-						ctx.ServerError("isLegalTeamReviewRequest", err)
+						ctx.Status(403)
 						return
 					}
 
@@ -1868,9 +1868,9 @@ func updatePullReviewRequest(ctx *context.Context) {
 				return
 			}
 
-			err = isLegalReviewRequest(reviewer, ctx.User, action == "attach", issue)
+			err = isValidReviewRequest(reviewer, ctx.User, action == "attach", issue)
 			if err != nil {
-				ctx.ServerError("isLegalRequestReview", err)
+				ctx.Status(403)
 				return
 			}
 

From ef394de0e70877c0a6d8a86d522848cf09cb8a10 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Sun, 11 Oct 2020 15:49:01 +0100
Subject: [PATCH 16/24] Remove getPrivateReviewers and getPublicReviewer

These functions are not reusable and therefore should be folded back in
to their calling function.
---
 models/repo.go | 74 +++++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 46 deletions(-)

diff --git a/models/repo.go b/models/repo.go
index aacf8d6f9daf3..f505412e033bf 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -694,67 +694,49 @@ func (repo *Repository) GetAssignees() (_ []*User, err error) {
 	return repo.getAssignees(x)
 }
 
-func (repo *Repository) getReviewersPrivate(e Engine, doerID, posterID int64) (users []*User, err error) {
-	users = make([]*User, 0, 20)
-
-	if err = e.
-		SQL("SELECT * FROM `user` WHERE id in (SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?)) ORDER BY name",
-			repo.ID, AccessModeRead,
-			doerID, posterID).
-		Find(&users); err != nil {
-		return nil, err
-	}
-
-	return users, nil
-}
-
-func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_ []*User, err error) {
-
-	users := make([]*User, 0)
-
-	const SQLCmd = "SELECT * FROM `user` WHERE id IN ( " +
-		"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) " +
-		"UNION " +
-		"SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) " +
-		") ORDER BY name"
-
-	if err = e.
-		SQL(SQLCmd,
-			repo.ID, AccessModeRead, doerID, posterID,
-			repo.ID, doerID, posterID, RepoWatchModeNormal, RepoWatchModeAuto).
-		Find(&users); err != nil {
-		return nil, err
-	}
-
-	return users, nil
-}
-
 func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, error) {
+	// Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries
 	if err := repo.getOwner(e); err != nil {
 		return nil, err
 	}
 
-	var (
-		users []*User
-		err   error
-	)
+	var users []*User
 
 	if repo.IsPrivate ||
 		(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
-		users, err = repo.getReviewersPrivate(x, doerID, posterID)
-	} else {
-		users, err = repo.getReviewersPublic(x, doerID, posterID)
+		// This a private repository:
+		// Anyone who can read the repository is a requestable reviewer
+		if err := e.
+			SQL("SELECT * FROM `user` WHERE id in (SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?)) ORDER BY name",
+				repo.ID, AccessModeRead,
+				doerID, posterID).
+			Find(&users); err != nil {
+			return nil, err
+		}
+
+		return users, nil
 	}
-	if err != nil {
+
+	// This is a "public" repository:
+	// Any user that has write access or who is a watcher can be requested to review
+	if err := e.
+		SQL("SELECT * FROM `user` WHERE id IN ( "+
+			"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) "+
+			"UNION "+
+			"SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) "+
+			") ORDER BY name",
+			repo.ID, AccessModeRead, doerID, posterID,
+			repo.ID, doerID, posterID, RepoWatchModeNormal, RepoWatchModeAuto).
+		Find(&users); err != nil {
 		return nil, err
 	}
 
 	return users, nil
 }
 
-// GetReviewers get all users can be requested to review
-// for private rpo , that return all users that have read access or higher to the repository.
-// but for public rpo, that return all users that have write access or higher to the repository,
+// GetReviewers get all users can be requested to review:
+// * for private repositories this returns all users that have read access or higher to the repository.
+// * for public repositories this returns all users that have write access or higher to the repository,
 // and all repo watchers.
 // TODO: may be we should hava a busy choice for users to block review request to them.
 func (repo *Repository) GetReviewers(doerID, posterID int64) ([]*User, error) {

From 877c38b254061cc70c428fa231dc49219d0c28f2 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Sun, 11 Oct 2020 20:40:17 +0100
Subject: [PATCH 17/24] Simplifications

---
 models/review.go           | 126 +++++++++++++++++--------------------
 services/issue/assignee.go |  31 ++++-----
 2 files changed, 75 insertions(+), 82 deletions(-)

diff --git a/models/review.go b/models/review.go
index 8e2f8ff7ee10a..d05bd0c22c039 100644
--- a/models/review.go
+++ b/models/review.go
@@ -219,12 +219,12 @@ type CreateReviewOptions struct {
 	Stale        bool
 }
 
-// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
-func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) {
-	return isOfficialReviewer(x, issue, reviewer)
+// IsOfficialReviewer check if at least one of the provided reviewers can make official reviews in issue (counts towards required approvals)
+func IsOfficialReviewer(issue *Issue, reviewers ...*User) (bool, error) {
+	return isOfficialReviewer(x, issue, reviewers...)
 }
 
-func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
+func isOfficialReviewer(e Engine, issue *Issue, reviewers ...*User) (bool, error) {
 	pr, err := getPullRequestByIssueID(e, issue.ID)
 	if err != nil {
 		return false, err
@@ -236,7 +236,14 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
 		return false, nil
 	}
 
-	return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
+	for _, reviewer := range reviewers {
+		official, err := pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
+		if official || err != nil {
+			return official, err
+		}
+	}
+
+	return false, nil
 }
 
 // IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals)
@@ -426,22 +433,20 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
 	// try to remove team review request if need
 	if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
 		teamReviewRequests := make([]*Review, 0, 10)
-		if err = sess.SQL("SELECT * FROM review WHERE reviewer_team_id > 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
+		if err := sess.SQL("SELECT * FROM review WHERE reviewer_team_id > 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
 			return nil, nil, err
 		}
 
-		if len(teamReviewRequests) > 0 {
-			for _, teamReviewRequest := range teamReviewRequests {
-				ok := false
-				if ok, err = isTeamMember(sess, issue.Repo.OwnerID, teamReviewRequest.ReviewerTeamID, doer.ID); err != nil {
-					return nil, nil, err
-				}
-
-				if ok {
-					if _, err := sess.Delete(teamReviewRequest); err != nil {
-						return nil, nil, err
-					}
-				}
+		for _, teamReviewRequest := range teamReviewRequests {
+			ok, err := isTeamMember(sess, issue.Repo.OwnerID, teamReviewRequest.ReviewerTeamID, doer.ID)
+			if err != nil {
+				return nil, nil, err
+			} else if !ok {
+				continue
+			}
+
+			if _, err := sess.Delete(teamReviewRequest); err != nil {
+				return nil, nil, err
 			}
 		}
 	}
@@ -456,6 +461,9 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
 
 	sess := x.NewSession()
 	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
 
 	// Get latest review of each reviwer, sorted in order they were made
 	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
@@ -479,12 +487,12 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
 }
 
 // GetReviewerByIssueIDAndUserID get the latest review of reviewer for a pull request
-func GetReviewerByIssueIDAndUserID(issueID, userID int64) (review *Review, err error) {
-	return getReviewerByIssueIDAndUserID(x, issueID, userID)
+func GetReviewerByIssueIDAndUserID(issueID, userID int64) (*Review, error) {
+	return getReviewByIssueIDAndUserID(x, issueID, userID)
 }
 
-func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Review, err error) {
-	review = new(Review)
+func getReviewByIssueIDAndUserID(e Engine, issueID, userID int64) (*Review, error) {
+	var review *Review
 
 	if _, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
 		issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
@@ -492,7 +500,7 @@ func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Rev
 		return nil, err
 	}
 
-	return
+	return review, nil
 }
 
 // GetTeamReviewerByIssueIDAndTeamID get the latest review requst of reviewer team for a pull request
@@ -581,7 +589,7 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		return nil, err
 	}
 
-	review, err := getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
+	review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
 	if err != nil {
 		return nil, err
 	}
@@ -591,18 +599,10 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		return nil, nil
 	}
 
-	var official bool
-	if official, err = isOfficialReviewer(sess, issue, reviewer); err != nil {
+	official, err := isOfficialReviewer(sess, issue, reviewer, doer)
+	if err != nil {
 		return nil, err
-	}
-
-	if !official {
-		if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
-			return nil, err
-		}
-	}
-
-	if official {
+	} else if official {
 		if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, reviewer.ID); err != nil {
 			return nil, err
 		}
@@ -618,15 +618,15 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		return nil, err
 	}
 
-	var comment *Comment
-	if comment, err = createComment(sess, &CreateCommentOptions{
+	comment, err := createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
-	}); err != nil {
+	})
+	if err != nil {
 		return nil, err
 	}
 
@@ -641,7 +641,7 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		return nil, err
 	}
 
-	review, err := getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
+	review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
 	if err != nil {
 		return nil, err
 	}
@@ -654,16 +654,12 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		return nil, err
 	}
 
-	var official bool
-	if official, err = isOfficialReviewer(sess, issue, reviewer); err != nil {
+	official, err := isOfficialReviewer(sess, issue, reviewer)
+	if err != nil {
 		return nil, err
-	}
-
-	if official {
-		// recalculate which is the latest official review from that user
-		var review *Review
-
-		review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
+	} else if official {
+		// recalculate the latest official review for reviewer
+		review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
 		if err != nil {
 			return nil, err
 		}
@@ -675,8 +671,7 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		}
 	}
 
-	var comment *Comment
-	comment, err = createComment(sess, &CreateCommentOptions{
+	comment, err := createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
@@ -684,7 +679,6 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		RemovedAssignee: true,        // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
 	})
-
 	if err != nil {
 		return nil, err
 	}
@@ -710,12 +704,10 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
 		return nil, nil
 	}
 
-	var official bool
-	if official, err = isOfficialReviewerTeam(sess, issue, reviewer); err != nil {
+	official, err := isOfficialReviewerTeam(sess, issue, reviewer)
+	if err != nil {
 		return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
-	}
-
-	if !official {
+	} else if !official {
 		if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
 			return nil, fmt.Errorf("isOfficialReviewer(): %v", err)
 		}
@@ -732,20 +724,20 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
 	}
 
 	if official {
-		if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id = ?", false, issue.ID, reviewer.ID); err != nil {
+		if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id=?", false, issue.ID, reviewer.ID); err != nil {
 			return nil, err
 		}
 	}
 
-	var comment *Comment
-	if comment, err = createComment(sess, &CreateCommentOptions{
+	comment, err := createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
-	}); err != nil {
+	})
+	if err != nil {
 		return nil, fmt.Errorf("createComment(): %v", err)
 	}
 
@@ -773,16 +765,14 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment
 		return nil, err
 	}
 
-	var official bool
-	if official, err = isOfficialReviewerTeam(sess, issue, reviewer); err != nil {
+	official, err := isOfficialReviewerTeam(sess, issue, reviewer)
+	if err != nil {
 		return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
 	}
 
 	if official {
 		// recalculate which is the latest official review from that team
-		var review *Review
-
-		review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
+		review, err := getReviewByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
 		if err != nil {
 			return nil, err
 		}
@@ -798,15 +788,15 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment
 		return nil, sess.Commit()
 	}
 
-	var comment *Comment
-	if comment, err = createComment(sess, &CreateCommentOptions{
+	comment, err := createComment(sess, &CreateCommentOptions{
 		Type:            CommentTypeReviewRequest,
 		Doer:            doer,
 		Repo:            issue.Repo,
 		Issue:           issue,
 		RemovedAssignee: true,        // Use RemovedAssignee as !isRequest
 		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
-	}); err != nil {
+	})
+	if err != nil {
 		return nil, fmt.Errorf("createComment(): %v", err)
 	}
 
diff --git a/services/issue/assignee.go b/services/issue/assignee.go
index 35edec206a2a0..f48e55e53c996 100644
--- a/services/issue/assignee.go
+++ b/services/issue/assignee.go
@@ -85,22 +85,25 @@ func TeamReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.
 		return
 	}
 
-	if comment != nil && isAdd {
-		// notify all user in this team
-		if err = comment.LoadIssue(); err != nil {
-			return
-		}
-		if err = reviewer.GetMembers(&models.SearchMembersOptions{}); err != nil {
-			return
-		}
+	if comment == nil || !isAdd {
+		return
+	}
 
-		for _, member := range reviewer.Members {
-			if member.ID == comment.Issue.PosterID {
-				continue
-			}
-			comment.AssigneeID = member.ID
-			notification.NotifyPullReviewRequest(doer, issue, member, isAdd, comment)
+	// notify all user in this team
+	if err = comment.LoadIssue(); err != nil {
+		return
+	}
+
+	if err = reviewer.GetMembers(&models.SearchMembersOptions{}); err != nil {
+		return
+	}
+
+	for _, member := range reviewer.Members {
+		if member.ID == comment.Issue.PosterID {
+			continue
 		}
+		comment.AssigneeID = member.ID
+		notification.NotifyPullReviewRequest(doer, issue, member, isAdd, comment)
 	}
 
 	return nil

From 170b57f64d5c5631a96f4ea17265883d598a68a2 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Sun, 11 Oct 2020 20:51:49 +0100
Subject: [PATCH 18/24] Simplify and improve logging

---
 routers/repo/issue.go | 311 +++++++++++++++++++++---------------------
 1 file changed, 159 insertions(+), 152 deletions(-)

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index df41b4134f851..2431ca80fb605 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -456,6 +456,10 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 		return
 	}
 
+	if len(reviews) == 0 && !canChooseReviewer {
+		return
+	}
+
 	var (
 		pullReviews         []*repoReviewerSelection
 		reviewersResult     []*repoReviewerSelection
@@ -469,164 +473,150 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 		if issue.OriginalAuthorID > 0 {
 			posterID = 0
 		}
+
 		reviewers, err = repo.GetReviewers(ctx.User.ID, posterID)
 		if err != nil {
 			ctx.ServerError("GetReviewers", err)
 			return
 		}
+
 		teamReviewers, err = repo.GetReviewerTeams()
 		if err != nil {
 			ctx.ServerError("GetReviewerTeams", err)
 			return
 		}
+
 		if len(reviewers) > 0 {
 			reviewersResult = make([]*repoReviewerSelection, 0, len(reviewers))
 		}
+
 		if len(teamReviewers) > 0 {
 			teamReviewersResult = make([]*repoReviewerSelection, 0, len(teamReviewers))
 		}
 	}
 
-	if len(reviews) != 0 {
-		pullReviews = make([]*repoReviewerSelection, 0, len(reviews))
+	pullReviews = make([]*repoReviewerSelection, 0, len(reviews))
 
-		for _, review := range reviews {
-			tmp := &repoReviewerSelection{
-				Checked: review.Type == models.ReviewTypeRequest,
-				Review:  review,
-				ItemID:  review.ReviewerID,
-			}
-			if review.ReviewerTeamID > 0 {
-				tmp.IsTeam = true
-				tmp.ItemID = -review.ReviewerTeamID
-			}
-			if ctx.User != nil && ctx.User.ID == review.ReviewerID && review.Type == models.ReviewTypeRequest {
-				// all user can refuse the review request to them
-				tmp.CanChange = true
-			} else if (canChooseReviewer || (ctx.User != nil && ctx.User.ID == issue.PosterID)) && review.Type != models.ReviewTypeRequest &&
-				ctx.User.ID != review.ReviewerID {
-				// manager and official reviewers call re-requst review from other reviewer
-				tmp.CanChange = true
-			} else if ctx.User != nil && ctx.Repo.IsAdmin() && ctx.User.ID != review.ReviewerID {
-				// only admin can dissims review request to other reviewers
-				tmp.CanChange = true
-			}
+	for _, review := range reviews {
+		tmp := &repoReviewerSelection{
+			Checked: review.Type == models.ReviewTypeRequest,
+			Review:  review,
+			ItemID:  review.ReviewerID,
+		}
+		if review.ReviewerTeamID > 0 {
+			tmp.IsTeam = true
+			tmp.ItemID = -review.ReviewerTeamID
+		}
 
-			pullReviews = append(pullReviews, tmp)
+		if ctx.Repo.IsAdmin() {
+			// Admin can dismiss or re-request any review requests
+			tmp.CanChange = true
+		} else if ctx.User != nil && ctx.User.ID == review.ReviewerID && review.Type == models.ReviewTypeRequest {
+			// A user can refuse review requests
+			tmp.CanChange = true
+		} else if (canChooseReviewer || (ctx.User != nil && ctx.User.ID == issue.PosterID)) && review.Type != models.ReviewTypeRequest &&
+			ctx.User.ID != review.ReviewerID {
+			// manager and official reviewers call re-requst review from other reviewer
+			tmp.CanChange = true
+		}
 
-			if canChooseReviewer {
-				if tmp.IsTeam {
-					teamReviewersResult = append(teamReviewersResult, tmp)
-				} else {
-					reviewersResult = append(reviewersResult, tmp)
-				}
+		pullReviews = append(pullReviews, tmp)
+
+		if canChooseReviewer {
+			if tmp.IsTeam {
+				teamReviewersResult = append(teamReviewersResult, tmp)
+			} else {
+				reviewersResult = append(reviewersResult, tmp)
 			}
 		}
-	} else if !canChooseReviewer {
-		return
 	}
 
-	if !canChooseReviewer {
-		exitsReviers := make([]*repoReviewerSelection, 0, len(pullReviews))
+	if len(pullReviews) > 0 {
+		// Drop all non-existing users and teams from the reviews
+		currentPullReviewers := make([]*repoReviewerSelection, 0, len(pullReviews))
 		for _, item := range pullReviews {
 			if item.Review.ReviewerID > 0 {
 				if err = item.Review.LoadReviewer(); err != nil {
 					if models.IsErrUserNotExist(err) {
 						continue
-					} else {
-						ctx.ServerError("LoadReviewer", err)
-						return
 					}
+					ctx.ServerError("LoadReviewer", err)
+					return
 				}
 				item.User = item.Review.Reviewer
 			} else if item.Review.ReviewerTeamID > 0 {
 				if err = item.Review.LoadReviewerTeam(); err != nil {
 					if models.IsErrTeamNotExist(err) {
 						continue
-					} else {
-						ctx.ServerError("LoadReviewerTeam", err)
-						return
 					}
+					ctx.ServerError("LoadReviewerTeam", err)
+					return
 				}
 				item.Team = item.Review.ReviewerTeam
 			} else {
 				continue
 			}
 
-			exitsReviers = append(exitsReviers, item)
+			currentPullReviewers = append(currentPullReviewers, item)
 		}
-		ctx.Data["PullReviewers"] = exitsReviers
-		return
+		ctx.Data["PullReviewers"] = currentPullReviewers
 	}
 
-	if reviewersResult != nil {
-		exitLen := len(reviewersResult)
+	if canChooseReviewer && reviewersResult != nil {
+		preadded := len(reviewersResult)
 		for _, reviewer := range reviewers {
-			exist := false
-			for index, tmp := range reviewersResult {
-				if index >= exitLen {
-					break
-				}
+			found := false
+		reviewAddLoop:
+			for _, tmp := range reviewersResult[:preadded] {
 				if tmp.ItemID == reviewer.ID {
 					tmp.User = reviewer
-					exist = true
+					found = true
+					break reviewAddLoop
 				}
 			}
-			if !exist {
-				reviewersResult = append(reviewersResult, &repoReviewerSelection{
-					IsTeam:    false,
-					CanChange: true,
-					User:      reviewer,
-					ItemID:    reviewer.ID,
-				})
+
+			if found {
+				continue
 			}
+
+			reviewersResult = append(reviewersResult, &repoReviewerSelection{
+				IsTeam:    false,
+				CanChange: true,
+				User:      reviewer,
+				ItemID:    reviewer.ID,
+			})
 		}
 
 		ctx.Data["Reviewers"] = reviewersResult
 	}
 
-	if teamReviewersResult != nil {
-		exitLen := len(teamReviewersResult)
+	if canChooseReviewer && teamReviewersResult != nil {
+		preadded := len(teamReviewersResult)
 		for _, team := range teamReviewers {
-			exist := false
-			for index, tmp := range teamReviewersResult {
-				if index >= exitLen {
-					break
-				}
+			found := false
+		teamReviewAddLoop:
+			for _, tmp := range teamReviewersResult[:preadded] {
 				if tmp.ItemID == -team.ID {
 					tmp.Team = team
-					exist = true
+					found = true
+					break teamReviewAddLoop
 				}
 			}
-			if !exist {
-				teamReviewersResult = append(teamReviewersResult, &repoReviewerSelection{
-					IsTeam:    true,
-					CanChange: true,
-					Team:      team,
-					ItemID:    -team.ID,
-				})
+
+			if found {
+				continue
 			}
+
+			teamReviewersResult = append(teamReviewersResult, &repoReviewerSelection{
+				IsTeam:    true,
+				CanChange: true,
+				Team:      team,
+				ItemID:    -team.ID,
+			})
 		}
 
 		ctx.Data["TeamReviewers"] = teamReviewersResult
 	}
-
-	if len(pullReviews) > 0 {
-		exitsReviers := make([]*repoReviewerSelection, 0, len(pullReviews))
-		for _, item := range pullReviews {
-			if ctx.User != nil && item.ItemID == ctx.User.ID {
-				if err = item.Review.LoadReviewer(); err != nil {
-					ctx.ServerError("LoadReviewer", err)
-					return
-				}
-				item.User = item.Review.Reviewer
-			}
-			if item.Team != nil || item.User != nil {
-				exitsReviers = append(exitsReviers, item)
-			}
-		}
-		ctx.Data["PullReviewers"] = exitsReviers
-	}
 }
 
 // RetrieveRepoMetas find all the meta information of a repository
@@ -1769,43 +1759,43 @@ func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bo
 		return fmt.Errorf("Organization can't be doer to add reviewer [user_id: %d, repo_id: %d]", doer.ID, issue.PullRequest.BaseRepo.ID)
 	}
 
-	permDoer, err := models.GetUserRepoPermission(issue.Repo, doer)
+	permission, err := models.GetUserRepoPermission(issue.Repo, doer)
 	if err != nil {
+		log.Error("Unable to GetUserRepoPermission for %-v in %-v#%d", doer, issue.Repo, issue.Index)
 		return err
 	}
 
-	var pemResult bool
 	if isAdd {
 		if issue.Repo.IsPrivate {
-			pemResult = models.HasTeamRepo(reviewer.OrgID, reviewer.ID, issue.RepoID)
+			hasTeam := models.HasTeamRepo(reviewer.OrgID, reviewer.ID, issue.RepoID)
 
-			if !pemResult {
-				return fmt.Errorf("Reviewer can't read [team_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
+			if !hasTeam {
+				return fmt.Errorf("Reviewing team can't read repo [team_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
 			}
 		}
 
-		pemResult = permDoer.CanAccessAny(models.AccessModeWrite, models.UnitTypePullRequests)
-		if !pemResult {
-			pemResult, err = models.IsOfficialReviewer(issue, doer)
+		doerCanWrite := permission.CanAccessAny(models.AccessModeWrite, models.UnitTypePullRequests)
+		if !doerCanWrite {
+			official, err := models.IsOfficialReviewer(issue, doer)
 			if err != nil {
+				log.Error("Unable to Check if IsOfficialReviewer for %-v in %-v#%d", doer, issue.Repo, issue.Index)
 				return err
 			}
-			if !pemResult {
+			if !official {
 				return fmt.Errorf("Doer can't choose reviewer [user_id: %d, repo_name: %s, issue_id: %d]", doer.ID, issue.Repo.Name, issue.ID)
 			}
 		}
 	} else {
-		pemResult = permDoer.IsAdmin()
-		if !pemResult {
-			return fmt.Errorf("Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
+		if !permission.IsAdmin() {
+			return fmt.Errorf("Only admin users can remove team requests. Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
 		}
 	}
 
 	return nil
 }
 
-// updatePullReviewRequest change pull's request reviewers
-func updatePullReviewRequest(ctx *context.Context) {
+// UpdatePullReviewRequest add or remove review request
+func UpdatePullReviewRequest(ctx *context.Context) {
 	issues := getActionIssues(ctx)
 	if ctx.Written() {
 		return
@@ -1821,68 +1811,90 @@ func updatePullReviewRequest(ctx *context.Context) {
 	}
 
 	for _, issue := range issues {
-		if issue.IsPull {
-
-			if reviewID < 0 {
-				if err := issue.LoadRepo(); err != nil {
-					ctx.ServerError("issue.LoadRepo", err)
-					return
-				}
-
-				if err := issue.Repo.GetOwner(); err != nil {
-					ctx.ServerError("issue.Repo.GetOwner", err)
-					return
-				}
-
-				if issue.Repo.Owner.IsOrganization() {
-					team, err := models.GetTeamByID(-reviewID)
-					if err != nil {
-						ctx.ServerError("models.GetTeamByID", err)
-						return
-					}
-
-					if team.OrgID != issue.Repo.OwnerID {
-						ctx.Status(403)
-						return
-					}
-
-					err = isValidTeamReviewRequest(team, ctx.User, action == "attach", issue)
-					if err != nil {
-						ctx.Status(403)
-						return
-					}
+		if err := issue.LoadRepo(); err != nil {
+			ctx.ServerError("issue.LoadRepo", err)
+			return
+		}
 
-					err = issue_service.TeamReviewRequest(issue, ctx.User, team, action == "attach")
-					if err != nil {
-						ctx.ServerError("ReviewRequest", err)
-						return
-					}
+		if !issue.IsPull {
+			log.Warn(
+				"UpdatePullReviewRequest: refusing to add review request for non-PR issue %-v#%d",
+				issue.Repo, issue.Index,
+			)
+			ctx.Status(403)
+			return
+		}
+		if reviewID < 0 {
+			// negative reviewIDs represent team requests
+			if err := issue.Repo.GetOwner(); err != nil {
+				ctx.ServerError("issue.Repo.GetOwner", err)
+				return
+			}
 
-					continue
-				}
+			if !issue.Repo.Owner.IsOrganization() {
+				log.Warn(
+					"UpdatePullReviewRequest: refusing to add team review request for %s#%d owned by non organization UID[%d]",
+					issue.Repo.FullName(), issue.Index, issue.Repo.ID,
+				)
+				ctx.Status(403)
+				return
 			}
 
-			reviewer, err := models.GetUserByID(reviewID)
+			team, err := models.GetTeamByID(-reviewID)
 			if err != nil {
-				ctx.ServerError("GetUserByID", err)
+				ctx.ServerError("models.GetTeamByID", err)
 				return
 			}
 
-			err = isValidReviewRequest(reviewer, ctx.User, action == "attach", issue)
+			if team.OrgID != issue.Repo.OwnerID {
+				log.Warn(
+					"UpdatePullReviewRequest: refusing to add team review request for UID[%d] team %s to %s#%d owned by UID[%d]",
+					team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID)
+				ctx.Status(403)
+				return
+			}
+
+			err = isValidTeamReviewRequest(team, ctx.User, action == "attach", issue)
 			if err != nil {
+				log.Warn(
+					"UpdatePullReviewRequest: refusing to add invalid team review request for UID[%d] team %s to %s#%d owned by UID[%d]: Error: %v",
+					team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID,
+					err,
+				)
 				ctx.Status(403)
 				return
 			}
 
-			err = issue_service.ReviewRequest(issue, ctx.User, reviewer, action == "attach")
+			err = issue_service.TeamReviewRequest(issue, ctx.User, team, action == "attach")
 			if err != nil {
-				ctx.ServerError("ReviewRequest", err)
+				ctx.ServerError("TeamReviewRequest", err)
 				return
 			}
-		} else {
+			continue
+		}
+
+		reviewer, err := models.GetUserByID(reviewID)
+		if err != nil {
+			ctx.ServerError("GetUserByID", err)
+			return
+		}
+
+		err = isValidReviewRequest(reviewer, ctx.User, action == "attach", issue)
+		if err != nil {
+			log.Warn(
+				"UpdatePullReviewRequest: refusing to add invalid review request for %-v to %-v#%d: Error: %v",
+				reviewer, issue.Repo, issue.Index,
+				err,
+			)
 			ctx.Status(403)
 			return
 		}
+
+		err = issue_service.ReviewRequest(issue, ctx.User, reviewer, action == "attach")
+		if err != nil {
+			ctx.ServerError("ReviewRequest", err)
+			return
+		}
 	}
 
 	ctx.JSON(200, map[string]interface{}{
@@ -1890,11 +1902,6 @@ func updatePullReviewRequest(ctx *context.Context) {
 	})
 }
 
-// UpdatePullReviewRequest add or remove review request
-func UpdatePullReviewRequest(ctx *context.Context) {
-	updatePullReviewRequest(ctx)
-}
-
 // UpdateIssueStatus change issue's status
 func UpdateIssueStatus(ctx *context.Context) {
 	issues := getActionIssues(ctx)

From 75a1ae6ce917c08eae05e43187e0a1b7ee45af64 Mon Sep 17 00:00:00 2001
From: zeripath <art27@cantab.net>
Date: Sun, 11 Oct 2020 19:51:13 +0100
Subject: [PATCH 19/24] Log the underlying panic in runMigrateTask (#13096)

If there is a panic during runMigrateTask we should capture and log the underlying
panic error.

This PR ensures that the panic is logged and captured as part of the task message.

Fix #13095

Signed-off-by: Andrew Thornton <art27@cantab.net>
---
 modules/task/migrate.go | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/modules/task/migrate.go b/modules/task/migrate.go
index 9d6c8bf733213..d25decaa00115 100644
--- a/modules/task/migrate.go
+++ b/modules/task/migrate.go
@@ -5,7 +5,6 @@
 package task
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"strings"
@@ -39,10 +38,8 @@ func handleCreateError(owner *models.User, err error, name string) error {
 func runMigrateTask(t *models.Task) (err error) {
 	defer func() {
 		if e := recover(); e != nil {
-			var buf bytes.Buffer
-			fmt.Fprintf(&buf, "Handler crashed with error: %v", log.Stack(2))
-
-			err = errors.New(buf.String())
+			err = fmt.Errorf("PANIC whilst trying to do migrate task: %v\nStacktrace: %v", err, log.Stack(2))
+			log.Critical("PANIC during runMigrateTask[%d] by DoerID[%d] to RepoID[%d] for OwnerID[%d]: %v", t.ID, t.DoerID, t.RepoID, t.OwnerID, err)
 		}
 
 		if err == nil {
@@ -52,14 +49,14 @@ func runMigrateTask(t *models.Task) (err error) {
 				return
 			}
 
-			log.Error("FinishMigrateTask failed: %s", err.Error())
+			log.Error("FinishMigrateTask[%d] by DoerID[%d] to RepoID[%d] for OwnerID[%d] failed: %v", t.ID, t.DoerID, t.RepoID, t.OwnerID, err)
 		}
 
 		t.EndTime = timeutil.TimeStampNow()
 		t.Status = structs.TaskStatusFailed
 		t.Errors = err.Error()
 		if err := t.UpdateCols("status", "errors", "end_time"); err != nil {
-			log.Error("Task UpdateCols failed: %s", err.Error())
+			log.Error("Task UpdateCols failed: %v", err)
 		}
 
 		if t.Repo != nil {

From 3035a37c418702280e0bb2ded4af680fdf17bc20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com>
Date: Mon, 12 Oct 2020 15:42:18 +0800
Subject: [PATCH 20/24] Update routers/repo/issue.go

Co-authored-by: zeripath <art27@cantab.net>
---
 routers/repo/issue.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 2431ca80fb605..7cabe2c31173e 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -516,7 +516,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue
 			tmp.CanChange = true
 		} else if (canChooseReviewer || (ctx.User != nil && ctx.User.ID == issue.PosterID)) && review.Type != models.ReviewTypeRequest &&
 			ctx.User.ID != review.ReviewerID {
-			// manager and official reviewers call re-requst review from other reviewer
+			// The poster of the PR, a manager, or official reviewers can re-request review from other reviewers
 			tmp.CanChange = true
 		}
 

From 386cb7570f402866d698cf5ccfbe56004b022f1d Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Mon, 12 Oct 2020 16:01:15 +0800
Subject: [PATCH 21/24] fix lint and return 403 for reviewer not exist

---
 routers/repo/issue.go | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 7cabe2c31173e..9f2874ab98604 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -1785,10 +1785,8 @@ func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bo
 				return fmt.Errorf("Doer can't choose reviewer [user_id: %d, repo_name: %s, issue_id: %d]", doer.ID, issue.Repo.Name, issue.ID)
 			}
 		}
-	} else {
-		if !permission.IsAdmin() {
-			return fmt.Errorf("Only admin users can remove team requests. Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
-		}
+	} else if !permission.IsAdmin() {
+		return fmt.Errorf("Only admin users can remove team requests. Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
 	}
 
 	return nil
@@ -1875,6 +1873,15 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 
 		reviewer, err := models.GetUserByID(reviewID)
 		if err != nil {
+			if models.IsErrUserNotExist(err) {
+				log.Warn(
+					"UpdatePullReviewRequest: requested reviewer [%d] for %-v to %-v#%d is not exist: Error: %v",
+					reviewID, issue.Repo, issue.Index,
+					err,
+				)
+				ctx.Status(403)
+				return
+			}
 			ctx.ServerError("GetUserByID", err)
 			return
 		}

From b13cad085bac7be4f1406d48475d450b9a713115 Mon Sep 17 00:00:00 2001
From: a1012112796 <1012112796@qq.com>
Date: Mon, 12 Oct 2020 21:02:37 +0800
Subject: [PATCH 22/24] fix bugs

---
 models/error.go                               |  20 ++++
 models/review.go                              |  25 +++--
 routers/repo/issue.go                         | 104 +++++++++++++-----
 templates/repo/issue/view_content/pull.tmpl   |   2 +-
 .../repo/issue/view_content/sidebar.tmpl      |   2 +-
 web_src/js/index.js                           |   5 +-
 6 files changed, 118 insertions(+), 40 deletions(-)

diff --git a/models/error.go b/models/error.go
index 1cab19aafdb31..9fbf9cac723ee 100644
--- a/models/error.go
+++ b/models/error.go
@@ -1994,6 +1994,26 @@ func (err ErrReviewNotExist) Error() string {
 	return fmt.Sprintf("review does not exist [id: %d]", err.ID)
 }
 
+// ErrNotValidReviewRequest an not allowed review request modify
+type ErrNotValidReviewRequest struct {
+	Reason string
+	UserID int64
+	RepoID int64
+}
+
+// IsErrNotValidReviewRequest checks if an error is a ErrNotValidReviewRequest.
+func IsErrNotValidReviewRequest(err error) bool {
+	_, ok := err.(ErrReviewNotExist)
+	return ok
+}
+
+func (err ErrNotValidReviewRequest) Error() string {
+	return fmt.Sprintf("%s [user_id: %d, repo_id: %d]",
+		err.Reason,
+		err.UserID,
+		err.RepoID)
+}
+
 //  ________      _____          __  .__
 //  \_____  \    /  _  \  __ ___/  |_|  |__
 //   /   |   \  /  /_\  \|  |  \   __\  |  \
diff --git a/models/review.go b/models/review.go
index d05bd0c22c039..8f756f2e25603 100644
--- a/models/review.go
+++ b/models/review.go
@@ -486,20 +486,25 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
 	return reviews, nil
 }
 
-// GetReviewerByIssueIDAndUserID get the latest review of reviewer for a pull request
-func GetReviewerByIssueIDAndUserID(issueID, userID int64) (*Review, error) {
+// GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request
+func GetReviewByIssueIDAndUserID(issueID, userID int64) (*Review, error) {
 	return getReviewByIssueIDAndUserID(x, issueID, userID)
 }
 
 func getReviewByIssueIDAndUserID(e Engine, issueID, userID int64) (*Review, error) {
-	var review *Review
+	review := new(Review)
 
-	if _, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
+	has, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
 		issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
-		Get(review); err != nil {
+		Get(review)
+	if err != nil {
 		return nil, err
 	}
 
+	if !has {
+		return nil, ErrReviewNotExist{}
+	}
+
 	return review, nil
 }
 
@@ -590,7 +595,7 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 	}
 
 	review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
-	if err != nil {
+	if err != nil && !IsErrReviewNotExist(err) {
 		return nil, err
 	}
 
@@ -642,11 +647,11 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 	}
 
 	review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
-	if err != nil {
+	if err != nil && !IsErrReviewNotExist(err) {
 		return nil, err
 	}
 
-	if review.Type != ReviewTypeRequest {
+	if review == nil && review.Type != ReviewTypeRequest {
 		return nil, nil
 	}
 
@@ -660,7 +665,7 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 	} else if official {
 		// recalculate the latest official review for reviewer
 		review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
-		if err != nil {
+		if err != nil && !IsErrReviewNotExist(err) {
 			return nil, err
 		}
 
@@ -773,7 +778,7 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment
 	if official {
 		// recalculate which is the latest official review from that team
 		review, err := getReviewByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
-		if err != nil {
+		if err != nil && !IsErrReviewNotExist(err) {
 			return nil, err
 		}
 
diff --git a/routers/repo/issue.go b/routers/repo/issue.go
index 9f2874ab98604..ef10651aa9e04 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -1690,10 +1690,18 @@ func UpdateIssueAssignee(ctx *context.Context) {
 
 func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models.Issue) error {
 	if reviewer.IsOrganization() {
-		return fmt.Errorf("Organization can't be added as reviewer [user_id: %d, repo_id: %d]", reviewer.ID, issue.PullRequest.BaseRepo.ID)
+		return models.ErrNotValidReviewRequest{
+			Reason: "Organization can't be added as reviewer",
+			UserID: doer.ID,
+			RepoID: issue.Repo.ID,
+		}
 	}
 	if doer.IsOrganization() {
-		return fmt.Errorf("Organization can't be doer to add reviewer [user_id: %d, repo_id: %d]", doer.ID, issue.PullRequest.BaseRepo.ID)
+		return models.ErrNotValidReviewRequest{
+			Reason: "Organization can't be doer to add reviewer",
+			UserID: doer.ID,
+			RepoID: issue.Repo.ID,
+		}
 	}
 
 	permReviewer, err := models.GetUserRepoPermission(issue.Repo, reviewer)
@@ -1706,8 +1714,8 @@ func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 		return err
 	}
 
-	lastreview, err := models.GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
-	if err != nil {
+	lastreview, err := models.GetReviewByIssueIDAndUserID(issue.ID, reviewer.ID)
+	if err != nil && !models.IsErrReviewNotExist(err) {
 		return err
 	}
 
@@ -1715,7 +1723,11 @@ func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 	if isAdd {
 		pemResult = permReviewer.CanAccessAny(models.AccessModeRead, models.UnitTypePullRequests)
 		if !pemResult {
-			return fmt.Errorf("Reviewer can't read [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
+			return models.ErrNotValidReviewRequest{
+				Reason: "Reviewer can't read",
+				UserID: doer.ID,
+				RepoID: issue.Repo.ID,
+			}
 		}
 
 		if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != models.ReviewTypeRequest {
@@ -1729,25 +1741,41 @@ func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 				return err
 			}
 			if !pemResult {
-				return fmt.Errorf("Doer can't choose reviewer [user_id: %d, repo_name: %s, issue_id: %d]", doer.ID, issue.Repo.Name, issue.ID)
+				return models.ErrNotValidReviewRequest{
+					Reason: "Doer can't choose reviewer",
+					UserID: doer.ID,
+					RepoID: issue.Repo.ID,
+				}
 			}
 		}
 
 		if doer.ID == reviewer.ID {
-			return fmt.Errorf("doer can't be reviewer [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
+			return models.ErrNotValidReviewRequest{
+				Reason: "doer can't be reviewer",
+				UserID: doer.ID,
+				RepoID: issue.Repo.ID,
+			}
 		}
 
 		if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 {
-			return fmt.Errorf("poster of pr can't be reviewer [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
+			return models.ErrNotValidReviewRequest{
+				Reason: "poster of pr can't be reviewer",
+				UserID: doer.ID,
+				RepoID: issue.Repo.ID,
+			}
 		}
 	} else {
-		if lastreview.Type == models.ReviewTypeRequest && lastreview.ReviewerID == doer.ID {
+		if lastreview != nil && lastreview.Type == models.ReviewTypeRequest && lastreview.ReviewerID == doer.ID {
 			return nil
 		}
 
 		pemResult = permDoer.IsAdmin()
 		if !pemResult {
-			return fmt.Errorf("Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
+			return models.ErrNotValidReviewRequest{
+				Reason: "Doer is not admin",
+				UserID: doer.ID,
+				RepoID: issue.Repo.ID,
+			}
 		}
 	}
 
@@ -1756,7 +1784,11 @@ func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
 
 func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bool, issue *models.Issue) error {
 	if doer.IsOrganization() {
-		return fmt.Errorf("Organization can't be doer to add reviewer [user_id: %d, repo_id: %d]", doer.ID, issue.PullRequest.BaseRepo.ID)
+		return models.ErrNotValidReviewRequest{
+			Reason: "Organization can't be doer to add reviewer",
+			UserID: doer.ID,
+			RepoID: issue.Repo.ID,
+		}
 	}
 
 	permission, err := models.GetUserRepoPermission(issue.Repo, doer)
@@ -1770,7 +1802,11 @@ func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bo
 			hasTeam := models.HasTeamRepo(reviewer.OrgID, reviewer.ID, issue.RepoID)
 
 			if !hasTeam {
-				return fmt.Errorf("Reviewing team can't read repo [team_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name)
+				return models.ErrNotValidReviewRequest{
+					Reason: "Reviewing team can't read repo",
+					UserID: doer.ID,
+					RepoID: issue.Repo.ID,
+				}
 			}
 		}
 
@@ -1782,11 +1818,19 @@ func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bo
 				return err
 			}
 			if !official {
-				return fmt.Errorf("Doer can't choose reviewer [user_id: %d, repo_name: %s, issue_id: %d]", doer.ID, issue.Repo.Name, issue.ID)
+				return models.ErrNotValidReviewRequest{
+					Reason: "Doer can't choose reviewer",
+					UserID: doer.ID,
+					RepoID: issue.Repo.ID,
+				}
 			}
 		}
 	} else if !permission.IsAdmin() {
-		return fmt.Errorf("Only admin users can remove team requests. Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name)
+		return models.ErrNotValidReviewRequest{
+			Reason: "Only admin users can remove team requests. Doer is not admin",
+			UserID: doer.ID,
+			RepoID: issue.Repo.ID,
+		}
 	}
 
 	return nil
@@ -1854,12 +1898,16 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 
 			err = isValidTeamReviewRequest(team, ctx.User, action == "attach", issue)
 			if err != nil {
-				log.Warn(
-					"UpdatePullReviewRequest: refusing to add invalid team review request for UID[%d] team %s to %s#%d owned by UID[%d]: Error: %v",
-					team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID,
-					err,
-				)
-				ctx.Status(403)
+				if models.IsErrNotValidReviewRequest(err) {
+					log.Warn(
+						"UpdatePullReviewRequest: refusing to add invalid team review request for UID[%d] team %s to %s#%d owned by UID[%d]: Error: %v",
+						team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID,
+						err,
+					)
+					ctx.Status(403)
+					return
+				}
+				ctx.ServerError("isValidTeamReviewRequest", err)
 				return
 			}
 
@@ -1888,12 +1936,16 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 
 		err = isValidReviewRequest(reviewer, ctx.User, action == "attach", issue)
 		if err != nil {
-			log.Warn(
-				"UpdatePullReviewRequest: refusing to add invalid review request for %-v to %-v#%d: Error: %v",
-				reviewer, issue.Repo, issue.Index,
-				err,
-			)
-			ctx.Status(403)
+			if models.IsErrNotValidReviewRequest(err) {
+				log.Warn(
+					"UpdatePullReviewRequest: refusing to add invalid review request for %-v to %-v#%d: Error: %v",
+					reviewer, issue.Repo, issue.Index,
+					err,
+				)
+				ctx.Status(403)
+				return
+			}
+			ctx.ServerError("isValidReviewRequest", err)
 			return
 		}
 
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index b9be5327870fb..b21d53419ff49 100644
--- a/templates/repo/issue/view_content/pull.tmpl
+++ b/templates/repo/issue/view_content/pull.tmpl
@@ -45,7 +45,7 @@
 								{{else}}grey{{end}}">
 
 								{{if .CanChange }}
-									<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if .Checked}}true{{else}}false{{end}}" data-issue-id="{{$.Issue.ID}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}"  data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
+									<a href="#" class="ui poping up icon re-request-review {{if .Checked}}checked{{end}}" data-issue-id="{{$.Issue.ID}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}"  data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
 										{{svg "octicon-sync"}}
 									</a>
 								{{end}}
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index 683427d7586cb..ce90063677608 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -65,7 +65,7 @@
 							{{- else}}grey{{end}} right ">
 
 							{{if .CanChange}}
-								<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if .Checked}}true{{else}}false{{end}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-issue-id="{{$.Issue.ID}}"  data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
+								<a href="#" class="ui poping up icon re-request-review {{if .Checked}}checked{{end}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-issue-id="{{$.Issue.ID}}"  data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
 									{{svg "octicon-sync"}}
 								</a>
 							{{end}}
diff --git a/web_src/js/index.js b/web_src/js/index.js
index f87e9f417c1d7..9fafe62d3ed9f 100644
--- a/web_src/js/index.js
+++ b/web_src/js/index.js
@@ -663,15 +663,16 @@ function initIssueComments() {
     const url = $(this).data('update-url');
     const issueId = $(this).data('issue-id');
     const id = $(this).data('id');
-    const isChecked = $(this).data('is-checked');
+    const isChecked = $(this).hasClass('checked');
 
     event.preventDefault();
     updateIssuesMeta(
       url,
-      isChecked === 'true' ? 'detach' : 'attach',
+      isChecked ? 'detach' : 'attach',
       issueId,
       id,
     ).then(reload);
+    return false;
   });
 
   $(document).on('click', (event) => {

From 0b909f9243280af18cb378af7930822db58deb57 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com>
Date: Mon, 12 Oct 2020 21:20:41 +0800
Subject: [PATCH 23/24] Update models/review.go

---
 models/review.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/models/review.go b/models/review.go
index 8f756f2e25603..2c38176ef4523 100644
--- a/models/review.go
+++ b/models/review.go
@@ -651,7 +651,7 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		return nil, err
 	}
 
-	if review == nil && review.Type != ReviewTypeRequest {
+	if review == nil || review.Type != ReviewTypeRequest {
 		return nil, nil
 	}
 

From e5d3c885096435e7decd705b141e190b89e2ae45 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Mon, 12 Oct 2020 19:11:29 +0100
Subject: [PATCH 24/24] Add octicon-people to the team reviews

Signed-off-by: Andrew Thornton <art27@cantab.net>
---
 templates/repo/issue/view_content/sidebar.tmpl | 6 +++---
 web_src/less/_repository.less                  | 6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index ce90063677608..a1dbc7ef7f526 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -38,8 +38,8 @@
 						{{if .Team}}
 							<a class="{{if not .CanChange}}ui poping up{{end}} item {{if .Checked}} checked {{end}} {{if not .CanChange}}ban-change{{end}}" href="#" data-id="{{.ItemID}}" data-id-selector="#review_request_team_{{.Team.ID}}" {{if not .CanChange}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
 								<span class="octicon-check {{if not .Checked}}invisible{{end}}">{{svg "octicon-check" 16}}</span>
-								<span class="ui text">
-									{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}
+								<span class="text">
+									{{svg "octicon-people" 16 "ml-4 mr-2"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}
 								</span>
 							</a>
 						{{end}}
@@ -56,7 +56,7 @@
 						{{if .User}}
 							<a href="{{.User.HomeLink}}"><img class="ui avatar image" src="{{.User.RelAvatarLink}}">&nbsp;{{.User.GetDisplayName}}</a>
 						{{else if .Team}}
-							<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
+							<span class="text">{{svg "octicon-people" 16 "teamavatar"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
 						{{end}}
 						<span class="ui right type-icon text {{if eq .Review.Type 1}}green
 							{{- else if eq .Review.Type 2}}grey
diff --git a/web_src/less/_repository.less b/web_src/less/_repository.less
index a6e3be68e7164..ca1a87b557abc 100644
--- a/web_src/less/_repository.less
+++ b/web_src/less/_repository.less
@@ -101,6 +101,12 @@
         line-height: 2em;
       }
 
+      &.assignees .teamavatar {
+        margin-top: .125rem;
+        margin-left: 6.75px;
+        margin-right: 8.75px;
+      }
+
       .hide {
         display: none !important;
       }