add request review from specific reviewers feature in pull request (#10756)

* add request review feature in pull request

add a way to notify specific reviewers to review like github , by add  or delet a  special type
review . The acton is  is similar to Assign ,  so many code reuse the function and items of
Assignee, but the meaning and result is different.

The Permission style is is similar to github, that only writer can add a review request from Reviewers,
but the poster can recall and remove a review request after a reviwer has revied even if he don't have
Write Premission. only manager , the poster and reviewer of a request review can remove it.

The reviewers can be requested to review contain all readers for private repo , for public, contain
all writers and watchers.

The offical Review Request will block merge if Reject can block it.

an other change: add ui otify for Assignees.

Co-authored-by: guillep2k <[email protected]>
Co-authored-by: Lauris BH <[email protected]>

Signed-off-by: a1012112796 <[email protected]>

* new change

* add placeholder string

* do some changes follow #10238 to add review requests num on lists also
change icon for review requests to eye

Co-authored-by: Lauris BH <[email protected]>

Author: a1012112796

models/branches.go

@@ -177,12 +177,13 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest)
 }
 
 // MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews
+// An official ReviewRequest should also block Merge like Reject
 func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool {
 	if !protectBranch.BlockOnRejectedReviews {
 		return false
 	}
 	rejectExist, err := x.Where("issue_id = ?", pr.IssueID).
-		And("type = ?", ReviewTypeReject).
+		And("type in ( ?, ?)", ReviewTypeReject, ReviewTypeRequest).
 		And("official = ?", true).
 		Exist(new(Review))
 	if err != nil {

models/issue_comment.go

@@ -86,6 +86,8 @@ const (
 	CommentTypeChangeTargetBranch
 	// Delete time manual for time tracking
 	CommentTypeDeleteTimeManual
+	// add or remove Request from one
+	CommentTypeReviewRequest
 )
 
 // CommentTag defines comment tag type

models/notification.go

@@ -118,64 +118,73 @@ func GetNotifications(opts FindNotificationOptions) (NotificationList, error) {
 
 // CreateOrUpdateIssueNotifications creates an issue notification
 // for each watcher, or updates it if already exists
-func CreateOrUpdateIssueNotifications(issueID, commentID int64, notificationAuthorID int64) error {
+// receiverID > 0 just send to reciver, else send to all watcher
+func CreateOrUpdateIssueNotifications(issueID, commentID, notificationAuthorID, receiverID int64) error {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err := sess.Begin(); err != nil {
 		return err
 	}
 
-	if err := createOrUpdateIssueNotifications(sess, issueID, commentID, notificationAuthorID); err != nil {
+	if err := createOrUpdateIssueNotifications(sess, issueID, commentID, notificationAuthorID, receiverID); err != nil {
 		return err
 	}
 
 	return sess.Commit()
 }
 
-func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notificationAuthorID int64) error {
+func createOrUpdateIssueNotifications(e Engine, issueID, commentID, notificationAuthorID, receiverID int64) error {
 	// init
-	toNotify := make(map[int64]struct{}, 32)
+	var toNotify map[int64]struct{}
 	notifications, err := getNotificationsByIssueID(e, issueID)
+
 	if err != nil {
 		return err
 	}
+
 	issue, err := getIssueByID(e, issueID)
 	if err != nil {
 		return err
 	}
 
-	issueWatches, err := getIssueWatchersIDs(e, issueID, true)
-	if err != nil {
-		return err
-	}
-	for _, id := range issueWatches {
-		toNotify[id] = struct{}{}
-	}
+	if receiverID > 0 {
+		toNotify = make(map[int64]struct{}, 1)
+		toNotify[receiverID] = struct{}{}
+	} else {
+		toNotify = make(map[int64]struct{}, 32)
+		issueWatches, err := getIssueWatchersIDs(e, issueID, true)
+		if err != nil {
+			return err
+		}
+		for _, id := range issueWatches {
+			toNotify[id] = struct{}{}
+		}
 
-	repoWatches, err := getRepoWatchersIDs(e, issue.RepoID)
-	if err != nil {
-		return err
-	}
-	for _, id := range repoWatches {
-		toNotify[id] = struct{}{}
-	}
-	issueParticipants, err := issue.getParticipantIDsByIssue(e)
-	if err != nil {
-		return err
-	}
-	for _, id := range issueParticipants {
-		toNotify[id] = struct{}{}
-	}
+		repoWatches, err := getRepoWatchersIDs(e, issue.RepoID)
+		if err != nil {
+			return err
+		}
+		for _, id := range repoWatches {
+			toNotify[id] = struct{}{}
+		}
+		issueParticipants, err := issue.getParticipantIDsByIssue(e)
+		if err != nil {
+			return err
+		}
+		for _, id := range issueParticipants {
+			toNotify[id] = struct{}{}
+		}
 
-	// dont notify user who cause notification
-	delete(toNotify, notificationAuthorID)
-	// explicit unwatch on issue
-	issueUnWatches, err := getIssueWatchersIDs(e, issueID, false)
-	if err != nil {
-		return err
-	}
-	for _, id := range issueUnWatches {
-		delete(toNotify, id)
+		// dont notify user who cause notification
+		delete(toNotify, notificationAuthorID)
+		// explicit unwatch on issue
+		issueUnWatches, err := getIssueWatchersIDs(e, issueID, false)
+		if err != nil {
+			return err
+		}
+		for _, id := range issueUnWatches {
+			delete(toNotify, id)
+		}
 	}
 
 	err = issue.loadRepo(e)

models/notification_test.go

@@ -14,7 +14,7 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	issue := AssertExistsAndLoadBean(t, &Issue{ID: 1}).(*Issue)
 
-	assert.NoError(t, CreateOrUpdateIssueNotifications(issue.ID, 0, 2))
+	assert.NoError(t, CreateOrUpdateIssueNotifications(issue.ID, 0, 2, 0))
 
 	// User 9 is inactive, thus notifications for user 1 and 4 are created
 	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification)

models/repo.go

@@ -622,6 +622,64 @@ 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) (users []*User, err error) {
+	if err = repo.getOwner(e); err != nil {
+		return nil, err
+	}
+
+	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)
+	}
+	return
+}
+
+// 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,
+// 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) {
+	return repo.getReviewers(x, doerID, posterID)
+}
+
 // GetMilestoneByID returns the milestone belongs to repository by given ID.
 func (repo *Repository) GetMilestoneByID(milestoneID int64) (*Milestone, error) {
 	return GetMilestoneByRepoID(repo.ID, milestoneID)

models/review.go

@@ -27,6 +27,8 @@ const (
 	ReviewTypeComment
 	// ReviewTypeReject gives feedback blocking merge
 	ReviewTypeReject
+	// ReviewTypeRequest request review from others
+	ReviewTypeRequest
 )
 
 // Icon returns the corresponding icon for the review type
@@ -38,6 +40,8 @@ func (rt ReviewType) Icon() string {
 		return "request-changes"
 	case ReviewTypeComment:
 		return "comment"
+	case ReviewTypeRequest:
+		return "primitive-dot"
 	default:
 		return "comment"
 	}
@@ -369,15 +373,15 @@ 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",
-		issueID, ReviewTypeApprove, ReviewTypeReject).
+	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",
+		issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
 		Find(&reviewsUnfiltered); 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 err = review.loadReviewer(sess); err != nil {
 			if !IsErrUserNotExist(err) {
 				return nil, err
 			}
@@ -389,6 +393,19 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
 	return reviews, nil
 }
 
+// GetReviewerByIssueIDAndUserID get the latest review of reviewer for a pull request
+func GetReviewerByIssueIDAndUserID(issueID, userID int64) (review *Review, err error) {
+	review = new(Review)
+
+	if _, err := x.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND type in (?, ?, ?))",
+		issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
+		Get(review); err != nil {
+		return nil, err
+	}
+
+	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)
@@ -442,3 +459,134 @@ func InsertReviews(reviews []*Review) error {
 
 	return sess.Commit()
 }
+
+// AddRewiewRequest add a review request from one reviewer
+func AddRewiewRequest(issue *Issue, reviewer *User, 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 = isOfficialReviewer(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: reviewer,
+		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()
+}
+
+//RemoveRewiewRequest remove a review request from one reviewer
+func RemoveRewiewRequest(issue *Issue, reviewer *User, 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 = isOfficialReviewer(sess, issue, reviewer)
+	if err != nil {
+		return
+	}
+
+	if official {
+		// recalculate which is the latest official review from that user
+		var review *Review
+
+		review, err = GetReviewerByIssueIDAndUserID(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
+	}
+
+	comment, err = CreateComment(&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()
+}