Skip to content

Commit

Permalink
add request review from specific reviewers feature in pull request (g…
Browse files Browse the repository at this point in the history
…o-gitea#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 <18600385+guillep2k@users.noreply.github.com>
Co-authored-by: Lauris BH <lauris@nix.lv>

Signed-off-by: a1012112796 <1012112796@qq.com>

* new change

* add placeholder string

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

Co-authored-by: Lauris BH <lauris@nix.lv>
  • Loading branch information
2 people authored and Yohann Delafollye committed Jul 31, 2020
1 parent a742aae commit 75ab581
Show file tree
Hide file tree
Showing 24 changed files with 714 additions and 67 deletions.
3 changes: 2 additions & 1 deletion models/branches.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
77 changes: 43 additions & 34 deletions models/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion models/notification_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
58 changes: 58 additions & 0 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
154 changes: 151 additions & 3 deletions models/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -38,6 +40,8 @@ func (rt ReviewType) Icon() string {
return "request-changes"
case ReviewTypeComment:
return "comment"
case ReviewTypeRequest:
return "primitive-dot"
default:
return "comment"
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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()
}
Loading

0 comments on commit 75ab581

Please sign in to comment.