Skip to content

Commit ff59626

Browse files
committed
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]>
1 parent 3723b06 commit ff59626

File tree

20 files changed

+662
-53
lines changed

20 files changed

+662
-53
lines changed

models/branches.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,12 +177,13 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest)
177177
}
178178

179179
// MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews
180+
// An official ReviewRequest should also block Merge like Reject
180181
func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool {
181182
if !protectBranch.BlockOnRejectedReviews {
182183
return false
183184
}
184185
rejectExist, err := x.Where("issue_id = ?", pr.IssueID).
185-
And("type = ?", ReviewTypeReject).
186+
And("type in ( ?, ?)", ReviewTypeReject, ReviewTypeRequest).
186187
And("official = ?", true).
187188
Exist(new(Review))
188189
if err != nil {

models/issue_comment.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ const (
8686
CommentTypeChangeTargetBranch
8787
// Delete time manual for time tracking
8888
CommentTypeDeleteTimeManual
89+
// add or remove Request from one
90+
CommentTypeReviewRequest
8991
)
9092

9193
// CommentTag defines comment tag type

models/notification.go

Lines changed: 43 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -118,64 +118,73 @@ func GetNotifications(opts FindNotificationOptions) (NotificationList, error) {
118118

119119
// CreateOrUpdateIssueNotifications creates an issue notification
120120
// for each watcher, or updates it if already exists
121-
func CreateOrUpdateIssueNotifications(issueID, commentID int64, notificationAuthorID int64) error {
121+
// receiverID > 0 just send to reciver, else send to all watcher
122+
func CreateOrUpdateIssueNotifications(issueID, commentID, notificationAuthorID, receiverID int64) error {
122123
sess := x.NewSession()
123124
defer sess.Close()
124125
if err := sess.Begin(); err != nil {
125126
return err
126127
}
127128

128-
if err := createOrUpdateIssueNotifications(sess, issueID, commentID, notificationAuthorID); err != nil {
129+
if err := createOrUpdateIssueNotifications(sess, issueID, commentID, notificationAuthorID, receiverID); err != nil {
129130
return err
130131
}
131132

132133
return sess.Commit()
133134
}
134135

135-
func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notificationAuthorID int64) error {
136+
func createOrUpdateIssueNotifications(e Engine, issueID, commentID, notificationAuthorID, receiverID int64) error {
136137
// init
137-
toNotify := make(map[int64]struct{}, 32)
138+
var toNotify map[int64]struct{}
138139
notifications, err := getNotificationsByIssueID(e, issueID)
140+
139141
if err != nil {
140142
return err
141143
}
144+
142145
issue, err := getIssueByID(e, issueID)
143146
if err != nil {
144147
return err
145148
}
146149

147-
issueWatches, err := getIssueWatchersIDs(e, issueID, true)
148-
if err != nil {
149-
return err
150-
}
151-
for _, id := range issueWatches {
152-
toNotify[id] = struct{}{}
153-
}
150+
if receiverID > 0 {
151+
toNotify = make(map[int64]struct{}, 1)
152+
toNotify[receiverID] = struct{}{}
153+
} else {
154+
toNotify = make(map[int64]struct{}, 32)
155+
issueWatches, err := getIssueWatchersIDs(e, issueID, true)
156+
if err != nil {
157+
return err
158+
}
159+
for _, id := range issueWatches {
160+
toNotify[id] = struct{}{}
161+
}
154162

155-
repoWatches, err := getRepoWatchersIDs(e, issue.RepoID)
156-
if err != nil {
157-
return err
158-
}
159-
for _, id := range repoWatches {
160-
toNotify[id] = struct{}{}
161-
}
162-
issueParticipants, err := issue.getParticipantIDsByIssue(e)
163-
if err != nil {
164-
return err
165-
}
166-
for _, id := range issueParticipants {
167-
toNotify[id] = struct{}{}
168-
}
163+
repoWatches, err := getRepoWatchersIDs(e, issue.RepoID)
164+
if err != nil {
165+
return err
166+
}
167+
for _, id := range repoWatches {
168+
toNotify[id] = struct{}{}
169+
}
170+
issueParticipants, err := issue.getParticipantIDsByIssue(e)
171+
if err != nil {
172+
return err
173+
}
174+
for _, id := range issueParticipants {
175+
toNotify[id] = struct{}{}
176+
}
169177

170-
// dont notify user who cause notification
171-
delete(toNotify, notificationAuthorID)
172-
// explicit unwatch on issue
173-
issueUnWatches, err := getIssueWatchersIDs(e, issueID, false)
174-
if err != nil {
175-
return err
176-
}
177-
for _, id := range issueUnWatches {
178-
delete(toNotify, id)
178+
// dont notify user who cause notification
179+
delete(toNotify, notificationAuthorID)
180+
// explicit unwatch on issue
181+
issueUnWatches, err := getIssueWatchersIDs(e, issueID, false)
182+
if err != nil {
183+
return err
184+
}
185+
for _, id := range issueUnWatches {
186+
delete(toNotify, id)
187+
}
179188
}
180189

181190
err = issue.loadRepo(e)

models/notification_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {
1414
assert.NoError(t, PrepareTestDatabase())
1515
issue := AssertExistsAndLoadBean(t, &Issue{ID: 1}).(*Issue)
1616

17-
assert.NoError(t, CreateOrUpdateIssueNotifications(issue.ID, 0, 2))
17+
assert.NoError(t, CreateOrUpdateIssueNotifications(issue.ID, 0, 2, 0))
1818

1919
// User 9 is inactive, thus notifications for user 1 and 4 are created
2020
notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification)

models/repo.go

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,89 @@ func (repo *Repository) GetAssignees() (_ []*User, err error) {
622622
return repo.getAssignees(x)
623623
}
624624

625+
func (repo *Repository) getReviewersPrivate(e Engine, doerID, posterID int64) (users []*User, err error) {
626+
users = make([]*User, 0, 20)
627+
628+
if err = e.
629+
SQL("SELECT * FROM user WHERE id in (SELECT user_id FROM access WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?))",
630+
repo.ID, AccessModeRead,
631+
doerID, posterID).
632+
Find(&users); err != nil {
633+
return nil, err
634+
}
635+
636+
return users, nil
637+
}
638+
639+
func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (users []*User, err error) {
640+
641+
userIDs := make([]int64, 0, 20)
642+
643+
if err = e.
644+
SQL("SELECT user_id FROM access WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?)",
645+
repo.ID, AccessModeRead,
646+
doerID, posterID).
647+
Find(&userIDs); err != nil {
648+
return nil, err
649+
}
650+
651+
getNum := len(userIDs)
652+
653+
watcherIDs := make([]int64, 0)
654+
655+
if err = e.SQL("SELECT user_id FROM watch WHERE repo_id = ? AND user_id NOT IN ( ?, ?)",
656+
repo.ID, doerID, posterID).Find(&watcherIDs); err != nil {
657+
return nil, err
658+
}
659+
660+
for _, watcherID := range watcherIDs {
661+
isHave := false
662+
for index, userID := range userIDs {
663+
if index >= getNum {
664+
break
665+
}
666+
if userID == watcherID {
667+
isHave = true
668+
break
669+
}
670+
}
671+
672+
if !isHave {
673+
userIDs = append(userIDs, watcherID)
674+
}
675+
}
676+
677+
users = make([]*User, 0, len(userIDs))
678+
if err = e.In("id", userIDs).Find(&users); err != nil {
679+
return nil, err
680+
}
681+
682+
return users, nil
683+
}
684+
685+
func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, err error) {
686+
if err = repo.getOwner(e); err != nil {
687+
return nil, err
688+
}
689+
690+
if repo.IsPrivate ||
691+
(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
692+
users, err = repo.getReviewersPrivate(x, doerID, posterID)
693+
} else {
694+
users, err = repo.getReviewersPublic(x, doerID, posterID)
695+
}
696+
return
697+
}
698+
699+
// GetReviewers get all users can be requested to review
700+
// for private rpo , that return all users that have read access or higher to the repository.
701+
// but for public rpo, that return all users that have write access or higher to the repository,
702+
// and all repo watchers.
703+
// TODO: may be we should hava a busy choice for users to block review request to them.
704+
func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, err error) {
705+
return repo.getReviewers(x, doerID, posterID)
706+
}
707+
625708
// GetMilestoneByID returns the milestone belongs to repository by given ID.
626709
func (repo *Repository) GetMilestoneByID(milestoneID int64) (*Milestone, error) {
627710
return GetMilestoneByRepoID(repo.ID, milestoneID)

0 commit comments

Comments
 (0)