From ef89e75d0eb232e98ca7a7ef278b8681c7f4fe50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com> Date: Tue, 7 Apr 2020 00:33:34 +0800 Subject: [PATCH] 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 <18600385+guillep2k@users.noreply.github.com> Co-authored-by: Lauris BH Signed-off-by: a1012112796 <1012112796@qq.com> * 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 --- models/branches.go | 3 +- models/issue_comment.go | 2 + models/notification.go | 77 +++++---- models/notification_test.go | 2 +- models/repo.go | 58 +++++++ models/review.go | 154 +++++++++++++++++- models/review_test.go | 3 +- modules/notification/base/notifier.go | 1 + modules/notification/base/null.go | 4 + modules/notification/mail/mail.go | 7 + modules/notification/notification.go | 7 + modules/notification/ui/ui.go | 35 +++- options/locale/locale_en-US.ini | 12 ++ routers/repo/issue.go | 152 ++++++++++++++++- routers/routes/routes.go | 1 + routers/user/home.go | 2 + services/issue/assignee.go | 20 +++ templates/repo/issue/list.tmpl | 21 ++- templates/repo/issue/milestone_issues.tmpl | 21 ++- .../repo/issue/view_content/comments.tmpl | 22 ++- templates/repo/issue/view_content/pull.tmpl | 22 ++- .../repo/issue/view_content/sidebar.tmpl | 91 +++++++++++ templates/user/dashboard/issues.tmpl | 21 ++- web_src/js/index.js | 43 ++++- 24 files changed, 714 insertions(+), 67 deletions(-) diff --git a/models/branches.go b/models/branches.go index d488fc9fcc..44cfb41403 100644 --- a/models/branches.go +++ b/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 { diff --git a/models/issue_comment.go b/models/issue_comment.go index 303cf3f9c1..f522604afc 100644 --- a/models/issue_comment.go +++ b/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 diff --git a/models/notification.go b/models/notification.go index 0cee8616ca..8d74ac129f 100644 --- a/models/notification.go +++ b/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) diff --git a/models/notification_test.go b/models/notification_test.go index 6485f8dc7a..07b9f97de5 100644 --- a/models/notification_test.go +++ b/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) diff --git a/models/repo.go b/models/repo.go index 74b5a021f4..8f0736ef82 100644 --- a/models/repo.go +++ b/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) diff --git a/models/review.go b/models/review.go index 993b5577bd..3f7223154e 100644 --- a/models/review.go +++ b/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() +} diff --git a/models/review_test.go b/models/review_test.go index dd7ae662ad..45ddb3181d 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -52,7 +52,8 @@ func TestReviewType_Icon(t *testing.T) { assert.Equal(t, "request-changes", ReviewTypeReject.Icon()) assert.Equal(t, "comment", ReviewTypeComment.Icon()) assert.Equal(t, "comment", ReviewTypeUnknown.Icon()) - assert.Equal(t, "comment", ReviewType(4).Icon()) + assert.Equal(t, "primitive-dot", ReviewTypeRequest.Icon()) + assert.Equal(t, "comment", ReviewType(6).Icon()) } func TestFindReviews(t *testing.T) { diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index 1c607ded3b..43b68b603c 100644 --- a/modules/notification/base/notifier.go +++ b/modules/notification/base/notifier.go @@ -24,6 +24,7 @@ type Notifier interface { NotifyIssueChangeStatus(*models.User, *models.Issue, *models.Comment, bool) NotifyIssueChangeMilestone(doer *models.User, issue *models.Issue, oldMilestoneID int64) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) + NotifyPullRewiewRequest(doer *models.User, issue *models.Issue, reviewer *models.User, isRequest bool, comment *models.Comment) NotifyIssueChangeContent(doer *models.User, issue *models.Issue, oldContent string) NotifyIssueClearLabels(doer *models.User, issue *models.Issue) NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string) diff --git a/modules/notification/base/null.go b/modules/notification/base/null.go index f6c423b469..4b5efd80bb 100644 --- a/modules/notification/base/null.go +++ b/modules/notification/base/null.go @@ -86,6 +86,10 @@ func (*NullNotifier) NotifyIssueChangeContent(doer *models.User, issue *models.I func (*NullNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) { } +// NotifyPullRewiewRequest places a place holder function +func (*NullNotifier) NotifyPullRewiewRequest(doer *models.User, issue *models.Issue, reviewer *models.User, isRequest bool, comment *models.Comment) { +} + // NotifyIssueClearLabels places a place holder function func (*NullNotifier) NotifyIssueClearLabels(doer *models.User, issue *models.Issue) { } diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index ec7d9d617e..b5c2db3831 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -100,6 +100,13 @@ func (m *mailNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *model } } +func (m *mailNotifier) NotifyPullRewiewRequest(doer *models.User, issue *models.Issue, reviewer *models.User, isRequest bool, comment *models.Comment) { + if isRequest && doer.ID != reviewer.ID && reviewer.EmailNotifications() == models.EmailNotificationsEnabled { + ct := fmt.Sprintf("Requested to review #%d.", issue.Index) + mailer.SendIssueAssignedMail(issue, doer, ct, comment, []string{reviewer.Email}) + } +} + func (m *mailNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *models.User) { if err := pr.LoadIssue(); err != nil { log.Error("pr.LoadIssue: %v", err) diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 8c5d7d6035..95117c1f3e 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -150,6 +150,13 @@ func NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee } } +// NotifyPullRewiewRequest notifies Request Review change +func NotifyPullRewiewRequest(doer *models.User, issue *models.Issue, reviewer *models.User, isRequest bool, comment *models.Comment) { + for _, notifier := range notifiers { + notifier.NotifyPullRewiewRequest(doer, issue, reviewer, isRequest, comment) + } +} + // NotifyIssueClearLabels notifies clear labels to notifiers func NotifyIssueClearLabels(doer *models.User, issue *models.Issue) { for _, notifier := range notifiers { diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index 525753425a..6ce14a90cc 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -22,6 +22,7 @@ type ( IssueID int64 CommentID int64 NotificationAuthorID int64 + ReceiverID int64 // 0 -- ALL Watcher } ) @@ -39,7 +40,7 @@ func NewNotifier() base.Notifier { func (ns *notificationService) handle(data ...queue.Data) { for _, datum := range data { opts := datum.(issueNotificationOpts) - if err := models.CreateOrUpdateIssueNotifications(opts.IssueID, opts.CommentID, opts.NotificationAuthorID); err != nil { + if err := models.CreateOrUpdateIssueNotifications(opts.IssueID, opts.CommentID, opts.NotificationAuthorID, opts.ReceiverID); err != nil { log.Error("Was unable to create issue notification: %v", err) } } @@ -103,3 +104,35 @@ func (ns *notificationService) NotifyPullRequestReview(pr *models.PullRequest, r } _ = ns.issueQueue.Push(opts) } + +func (ns *notificationService) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) { + if !removed { + var opts = issueNotificationOpts{ + IssueID: issue.ID, + NotificationAuthorID: doer.ID, + ReceiverID: assignee.ID, + } + + if comment != nil { + opts.CommentID = comment.ID + } + + _ = ns.issueQueue.Push(opts) + } +} + +func (ns *notificationService) NotifyPullRewiewRequest(doer *models.User, issue *models.Issue, reviewer *models.User, isRequest bool, comment *models.Comment) { + if isRequest { + var opts = issueNotificationOpts{ + IssueID: issue.ID, + NotificationAuthorID: doer.ID, + ReceiverID: reviewer.ID, + } + + if comment != nil { + opts.CommentID = comment.ID + } + + _ = ns.issueQueue.Push(opts) + } +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index cb34e695ec..dfdae8f82b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -827,6 +827,7 @@ issues.desc = Organize bug reports, tasks and milestones. issues.filter_assignees = Filter Assignee issues.filter_milestones = Filter Milestone issues.filter_labels = Filter Label +issues.filter_reviewers = Filter Reviewer issues.new = New Issue issues.new.title_empty = Title cannot be empty issues.new.labels = Labels @@ -844,6 +845,8 @@ issues.new.assignees = Assignees issues.new.add_assignees_title = Assign users issues.new.clear_assignees = Clear assignees issues.new.no_assignees = No Assignees +issues.new.no_reviewers = No reviewers +issues.new.add_reviewer_title = Request review issues.no_ref = No Branch/Tag Specified issues.create = Create Issue issues.new_label = New Label @@ -937,6 +940,9 @@ issues.ref_from = `from %[1]s` issues.poster = Poster issues.collaborator = Collaborator issues.owner = Owner +issues.re_request_review=Re-request review +issues.remove_request_review=Remove review request +issues.remove_request_review_block=Can't remove review request issues.sign_in_require_desc = Sign in to join this conversation. issues.edit = Edit issues.cancel = Cancel @@ -1048,6 +1054,10 @@ issues.review.approve = "approved these changes %s" issues.review.comment = "reviewed %s" issues.review.content.empty = You need to leave a comment indicating the requested change(s). issues.review.reject = "requested changes %s" +issues.review.wait = "was requested for review %s" +issues.review.add_review_request = "requested review from %s %s" +issues.review.remove_review_request = "removed review request for %s %s" +issues.review.remove_review_request_self = "refused to review %s" issues.review.pending = Pending issues.review.review = Review issues.review.reviewers = Reviewers @@ -1096,6 +1106,8 @@ pulls.approve_count_1 = "%d approval" pulls.approve_count_n = "%d approvals" pulls.reject_count_1 = "%d change request" pulls.reject_count_n = "%d change requests" +pulls.waiting_count_1 = "%d waiting review" +pulls.waiting_count_n = "%d waiting reviews" pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled. pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually. diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 0697d11a66..0a059bf789 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -289,6 +289,8 @@ func issues(ctx *context.Context, milestoneID int64, isPullOption util.OptionalB reviewTyp := models.ReviewTypeApprove if typ == "reject" { reviewTyp = models.ReviewTypeReject + } else if typ == "waiting" { + reviewTyp = models.ReviewTypeRequest } for _, count := range counts { if count.Type == reviewTyp { @@ -377,6 +379,16 @@ 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) + if err != nil { + ctx.ServerError("GetReviewers", err) + return + } +} + // RetrieveRepoMetas find all the meta information of a repository func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository, isPull bool) []*models.Label { if !ctx.Repo.CanWriteIssuesOrPulls(isPull) { @@ -815,6 +827,28 @@ func ViewIssue(ctx *context.Context) { } } + if issue.IsPull { + canChooseReviewer := ctx.Repo.CanWrite(models.UnitTypePullRequests) + if !canChooseReviewer && ctx.User != nil && ctx.IsSigned { + canChooseReviewer, err = models.IsOfficialReviewer(issue, ctx.User) + if err != nil { + ctx.ServerError("IsOfficialReviewer", err) + return + } + } + + if canChooseReviewer { + RetrieveRepoReviewers(ctx, repo, issue.PosterID) + ctx.Data["CanChooseReviewer"] = true + } else { + ctx.Data["CanChooseReviewer"] = false + } + + if ctx.Written() { + return + } + } + if ctx.IsSigned { // Update issue-user. if err = issue.ReadBy(ctx.User.ID); err != nil { @@ -926,7 +960,7 @@ func ViewIssue(ctx *context.Context) { if comment.MilestoneID > 0 && comment.Milestone == nil { comment.Milestone = ghostMilestone } - } else if comment.Type == models.CommentTypeAssignees { + } else if comment.Type == models.CommentTypeAssignees || comment.Type == models.CommentTypeReviewRequest { if err = comment.LoadAssigneeUser(); err != nil { ctx.ServerError("LoadAssigneeUser", err) return @@ -1273,6 +1307,122 @@ func UpdateIssueAssignee(ctx *context.Context) { }) } +func isLegalReviewRequest(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) + } + 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) + } + + permReviewer, err := models.GetUserRepoPermission(issue.Repo, reviewer) + if err != nil { + return err + } + + permDoer, err := models.GetUserRepoPermission(issue.Repo, doer) + if err != nil { + return err + } + + lastreview, err := models.GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID) + if err != nil { + return err + } + + var pemResult bool + 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) + } + + if doer.ID == issue.PosterID && lastreview != nil && lastreview.Type != models.ReviewTypeRequest { + return nil + } + + 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) + } + } + + if doer.ID == reviewer.ID { + return fmt.Errorf("doer can't be reviewer [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name) + } + + if reviewer.ID == issue.PosterID { + return fmt.Errorf("poster of pr can't be reviewer [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name) + } + } else { + if 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 nil +} + +// updatePullReviewRequest change pull's request reviewers +func updatePullReviewRequest(ctx *context.Context) { + issues := getActionIssues(ctx) + if ctx.Written() { + return + } + + reviewID := ctx.QueryInt64("id") + event := ctx.Query("is_add") + + if event != "add" && event != "remove" { + ctx.ServerError("updatePullReviewRequest", fmt.Errorf("is_add should not be \"%s\"", event)) + return + } + + for _, issue := range issues { + if issue.IsPull { + + reviewer, err := models.GetUserByID(reviewID) + if err != nil { + ctx.ServerError("GetUserByID", err) + return + } + + err = isLegalReviewRequest(reviewer, ctx.User, event == "add", issue) + if err != nil { + ctx.ServerError("isLegalRequestReview", err) + return + } + + err = issue_service.ReviewRequest(issue, ctx.User, reviewer, event == "add") + if err != nil { + ctx.ServerError("ReviewRequest", err) + return + } + } else { + ctx.ServerError("updatePullReviewRequest", fmt.Errorf("%d in %d is not Pull Request", issue.ID, issue.Repo.ID)) + } + } + + ctx.JSON(200, map[string]interface{}{ + "ok": true, + }) +} + +// 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) diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 4409830dfe..5f477a847e 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -738,6 +738,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/labels", reqRepoIssuesOrPullsWriter, repo.UpdateIssueLabel) m.Post("/milestone", reqRepoIssuesOrPullsWriter, repo.UpdateIssueMilestone) m.Post("/assignee", reqRepoIssuesOrPullsWriter, repo.UpdateIssueAssignee) + m.Post("/request_review", reqRepoIssuesOrPullsReader, repo.UpdatePullReviewRequest) m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus) }, context.RepoMustNotBeArchived()) m.Group("/comments/:id", func() { diff --git a/routers/user/home.go b/routers/user/home.go index 71a65d6d9f..3807025ea5 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -632,6 +632,8 @@ func Issues(ctx *context.Context) { reviewTyp := models.ReviewTypeApprove if typ == "reject" { reviewTyp = models.ReviewTypeReject + } else if typ == "waiting" { + reviewTyp = models.ReviewTypeRequest } for _, count := range counts { if count.Type == reviewTyp { diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 281f824da7..9711de4c60 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -51,3 +51,23 @@ 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. +func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User, isAdd bool) (err error) { + var comment *models.Comment + if isAdd { + comment, err = models.AddRewiewRequest(issue, reviewer, doer) + } else { + comment, err = models.RemoveRewiewRequest(issue, reviewer, doer) + } + + if err != nil { + return + } + + if comment != nil { + notification.NotifyPullRewiewRequest(doer, issue, reviewer, isAdd, comment) + } + + return nil +} diff --git a/templates/repo/issue/list.tmpl b/templates/repo/issue/list.tmpl index 05018bacdd..4fe0aaa00d 100644 --- a/templates/repo/issue/list.tmpl +++ b/templates/repo/issue/list.tmpl @@ -271,14 +271,25 @@ {{if .IsPull}} {{$approveOfficial := call $approvalCounts .ID "approve"}} {{$rejectOfficial := call $approvalCounts .ID "reject"}} - {{if or (gt $approveOfficial 0) (gt $rejectOfficial 0)}} + {{$waitingOfficial := call $approvalCounts .ID "waiting"}} + {{if gt $approveOfficial 0}} {{svg "octicon-check" 16}} {{$.i18n.Tr (TrN $.i18n.Lang $approveOfficial "repo.pulls.approve_count_1" "repo.pulls.approve_count_n") $approveOfficial}} - {{if or (gt $rejectOfficial 0)}} - {{svg "octicon-x" 16}} - {{$.i18n.Tr (TrN $.i18n.Lang $rejectOfficial "repo.pulls.reject_count_1" "repo.pulls.reject_count_n") $rejectOfficial}} - {{end}} + {{end}} + + {{if gt $rejectOfficial 0}} + {{svg "octicon-request-changes" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $rejectOfficial "repo.pulls.reject_count_1" "repo.pulls.reject_count_n") $rejectOfficial}} + + {{end}} + + {{if gt $waitingOfficial 0}} + {{svg "octicon-eye" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $waitingOfficial "repo.pulls.waiting_count_1" "repo.pulls.waiting_count_n") $waitingOfficial}} + + {{end}} + {{if and (not .PullRequest.HasMerged) (gt (len .PullRequest.ConflictedFiles) 0)}} {{svg "octicon-mirror" 16}} {{$.i18n.Tr (TrN $.i18n.Lang (len .PullRequest.ConflictedFiles) "repo.pulls.num_conflicting_files_1" "repo.pulls.num_conflicting_files_n") (len .PullRequest.ConflictedFiles)}} {{end}} diff --git a/templates/repo/issue/milestone_issues.tmpl b/templates/repo/issue/milestone_issues.tmpl index d144a97f14..5307c05733 100644 --- a/templates/repo/issue/milestone_issues.tmpl +++ b/templates/repo/issue/milestone_issues.tmpl @@ -241,14 +241,25 @@ {{if .IsPull}} {{$approveOfficial := call $approvalCounts .ID "approve"}} {{$rejectOfficial := call $approvalCounts .ID "reject"}} - {{if or (gt $approveOfficial 0) (gt $rejectOfficial 0)}} + {{$waitingOfficial := call $approvalCounts .ID "waiting"}} + {{if gt $approveOfficial 0}} {{svg "octicon-check" 16}} {{$.i18n.Tr (TrN $.i18n.Lang $approveOfficial "repo.pulls.approve_count_1" "repo.pulls.approve_count_n") $approveOfficial}} - {{if or (gt $rejectOfficial 0)}} - {{svg "octicon-x" 16}} - {{$.i18n.Tr (TrN $.i18n.Lang $rejectOfficial "repo.pulls.reject_count_1" "repo.pulls.reject_count_n") $rejectOfficial}} - {{end}} + {{end}} + + {{if gt $rejectOfficial 0}} + {{svg "octicon-request-changes" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $rejectOfficial "repo.pulls.reject_count_1" "repo.pulls.reject_count_n") $rejectOfficial}} + + {{end}} + + {{if gt $waitingOfficial 0}} + {{svg "octicon-eye" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $waitingOfficial "repo.pulls.waiting_count_1" "repo.pulls.waiting_count_n") $waitingOfficial}} + + {{end}} + {{if and (not .PullRequest.HasMerged) (gt (len .PullRequest.ConflictedFiles) 0)}} {{svg "octicon-mirror" 16}} {{$.i18n.Tr (TrN $.i18n.Lang (len .PullRequest.ConflictedFiles) "repo.pulls.num_conflicting_files_1" "repo.pulls.num_conflicting_files_n") (len .PullRequest.ConflictedFiles)}} {{end}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index bb8188def9..022e96a02e 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -7,7 +7,7 @@ 13 = STOP_TRACKING, 14 = ADD_TIME_MANUAL, 16 = ADDED_DEADLINE, 17 = MODIFIED_DEADLINE, 18 = REMOVED_DEADLINE, 19 = ADD_DEPENDENCY, 20 = REMOVE_DEPENDENCY, 21 = CODE, 22 = REVIEW, 23 = ISSUE_LOCKED, 24 = ISSUE_UNLOCKED, 25 = TARGET_BRANCH_CHANGED, - 26 = DELETE_TIME_MANUAL --> + 26 = DELETE_TIME_MANUAL, 27 = REVIEW_REQUEST --> {{if eq .Type 0}}
{{if .OriginalAuthor }} @@ -468,5 +468,25 @@ {{.Content}}
+ {{else if eq .Type 27}} +
+ {{svg "octicon-eye" 16}} + + + + + {{.Poster.GetDisplayName}} + {{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.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}} + {{end}} + +
+ {{end}} {{end}} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index c7cf737b34..09c7cd1d9f 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -10,7 +10,25 @@ + {{- else if eq .Type 4}}yellow + {{else}}grey{{end}}"> + + {{$canChoose := false}} + {{if eq .Type 4}} + {{if or (eq .ReviewerID $.SignedUserID) $.Permission.IsAdmin}} + {{$canChoose = true}} + {{end}} + {{else}} + {{if and (or $.IsIssuePoster $.CanChooseReviewer) (not (eq $.SignedUserID .ReviewerID))}} + {{$canChoose = true}} + {{end}} + {{end}} + + {{if $canChoose }} + + {{svg "octicon-sync" 16}} + + {{end}} {{svg (printf "octicon-%s" .Type.Icon) 16}} {{if .Stale}} @@ -28,6 +46,8 @@ {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} {{else if eq .Type 3}} {{$.i18n.Tr "repo.issues.review.reject" $createdStr | Safe}} + {{else if eq .Type 4}} + {{$.i18n.Tr "repo.issues.review.wait" $createdStr | Safe}} {{else}} {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} {{end}} diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 2aed8f3158..941141b55f 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -2,6 +2,97 @@
{{template "repo/issue/branch_selector_field" .}} + {{if .Issue.IsPull }} + + +
+ + {{.i18n.Tr "repo.issues.review.reviewers"}} + {{if and .CanChooseReviewer (not .Repository.IsArchived)}} + {{svg "octicon-gear" 16}} + {{end}} + +
+
{{.i18n.Tr "repo.issues.new.add_reviewer_title"}}
+ {{if .Reviewers}} +
+ + +
+ {{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}} + {{end}} + {{end}} + + {{ if $notReviewed}} + {{$canChoose = true}} + {{end}} + + + {{svg "octicon-check" 16}} + + {{.GetDisplayName}} + + + {{end}} +
+
+ +
+ {{.i18n.Tr "repo.issues.new.no_reviewers"}} +
+ {{range .PullReviewers}} +
+  {{.Reviewer.GetDisplayName}} + + + {{$canChoose := false}} + {{if eq .Type 4}} + {{if or (eq .ReviewerID $.SignedUserID) $.Permission.IsAdmin}} + {{$canChoose = true}} + {{end}} + {{else}} + {{if and (or $.IsIssuePoster $.CanChooseReviewer) (not (eq $.SignedUserID .ReviewerID))}} + {{$canChoose = true}} + {{end}} + {{end}} + + {{if $canChoose}} + + {{svg "octicon-sync" 16}} + + {{end}} + {{svg (printf "octicon-%s" .Type.Icon) 16}} + +
+ {{end}} +
+
+ + {{end}} + +
+
{{.i18n.Tr "repo.issues.new.labels"}} diff --git a/templates/user/dashboard/issues.tmpl b/templates/user/dashboard/issues.tmpl index 447801d849..780c8c1257 100644 --- a/templates/user/dashboard/issues.tmpl +++ b/templates/user/dashboard/issues.tmpl @@ -173,14 +173,25 @@ {{if .IsPull}} {{$approveOfficial := call $approvalCounts .ID "approve"}} {{$rejectOfficial := call $approvalCounts .ID "reject"}} - {{if or (gt $approveOfficial 0) (gt $rejectOfficial 0) }} + {{$waitingOfficial := call $approvalCounts .ID "waiting"}} + {{if gt $approveOfficial 0}} {{svg "octicon-check" 16}} {{$.i18n.Tr (TrN $.i18n.Lang $approveOfficial "repo.pulls.approve_count_1" "repo.pulls.approve_count_n") $approveOfficial}} - {{if or (gt $rejectOfficial 0)}} - {{svg "octicon-x" 16}} - {{$.i18n.Tr (TrN $.i18n.Lang $rejectOfficial "repo.pulls.reject_count_1" "repo.pulls.reject_count_n") $rejectOfficial}} - {{end}} + {{end}} + + {{if gt $rejectOfficial 0}} + {{svg "octicon-request-changes" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $rejectOfficial "repo.pulls.reject_count_1" "repo.pulls.reject_count_n") $rejectOfficial}} + + {{end}} + + {{if gt $waitingOfficial 0}} + {{svg "octicon-eye" 16}} + {{$.i18n.Tr (TrN $.i18n.Lang $waitingOfficial "repo.pulls.waiting_count_1" "repo.pulls.waiting_count_n") $waitingOfficial}} + + {{end}} + {{if and (not .PullRequest.HasMerged) (gt (len .PullRequest.ConflictedFiles) 0)}} {{svg "octicon-mirror" 16}} {{$.i18n.Tr (TrN $.i18n.Lang (len .PullRequest.ConflictedFiles) "repo.pulls.num_conflicting_files_1" "repo.pulls.num_conflicting_files_n") (len .PullRequest.ConflictedFiles)}} {{end}} diff --git a/web_src/js/index.js b/web_src/js/index.js index a0e410c133..93b27483be 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -158,7 +158,7 @@ function initLabelEdit() { }); } -function updateIssuesMeta(url, action, issueIds, elementId) { +function updateIssuesMeta(url, action, issueIds, elementId, isAdd) { return new Promise(((resolve) => { $.ajax({ type: 'POST', @@ -167,7 +167,8 @@ function updateIssuesMeta(url, action, issueIds, elementId) { _csrf: csrf, action, issue_ids: issueIds, - id: elementId + id: elementId, + is_add: isAdd }, success: resolve }); @@ -390,7 +391,8 @@ function initCommentForm() { label['update-url'], label.action, label['issue-id'], - elementId + elementId, + label['is-checked'] ); promises.push(promise); }); @@ -400,22 +402,30 @@ function initCommentForm() { $listMenu.find('.item:not(.no-select)').click(function () { // we don't need the action attribute when updating assignees - if (selector === 'select-assignees-modify') { + if (selector === 'select-assignees-modify' || selector === 'select-reviewers-modify') { // UI magic. We need to do this here, otherwise it would destroy the functionality of // adding/removing labels + + if ($(this).data('can-change') === 'block') { + return false; + } + if ($(this).hasClass('checked')) { $(this).removeClass('checked'); $(this).find('.octicon-check').addClass('invisible'); + $(this).data('is-checked', 'remove'); } else { $(this).addClass('checked'); $(this).find('.octicon-check').removeClass('invisible'); + $(this).data('is-checked', 'add'); } updateIssuesMeta( $listMenu.data('update-url'), '', $listMenu.data('issue-id'), - $(this).data('id') + $(this).data('id'), + $(this).data('is-checked') ); $listMenu.data('action', 'update'); // Update to reload the page when we updated items return false; @@ -474,6 +484,7 @@ function initCommentForm() { $listMenu.data('update-url'), 'clear', $listMenu.data('issue-id'), + '', '' ).then(reload); } @@ -481,6 +492,7 @@ function initCommentForm() { $(this).parent().find('.item').each(function () { $(this).removeClass('checked'); $(this).find('.octicon').addClass('invisible'); + $(this).data('is-checked', 'remove'); }); $list.find('.item').each(function () { @@ -495,6 +507,7 @@ function initCommentForm() { initListSubmits('select-label', 'labels'); initListSubmits('select-assignees', 'assignees'); initListSubmits('select-assignees-modify', 'assignees'); + initListSubmits('select-reviewers-modify', 'assignees'); function selectItem(select_id, input_id) { const $menu = $(`${select_id} .menu`); @@ -512,7 +525,8 @@ function initCommentForm() { $menu.data('update-url'), '', $menu.data('issue-id'), - $(this).data('id') + $(this).data('id'), + $(this).data('is-checked') ).then(reload); } switch (input_id) { @@ -538,7 +552,8 @@ function initCommentForm() { $menu.data('update-url'), '', $menu.data('issue-id'), - $(this).data('id') + $(this).data('id'), + $(this).data('is-checked') ).then(reload); } @@ -648,6 +663,18 @@ function initInstall() { function initIssueComments() { if ($('.repository.view.issue .comments').length === 0) return; + $('.re-request-review').click((event) => { + const $this = $('.re-request-review'); + event.preventDefault(); + updateIssuesMeta( + $this.data('update-url'), + '', + $this.data('issue-id'), + $this.data('id'), + $this.data('is-checked') + ).then(reload); + }); + $(document).click((event) => { const urlTarget = $(':target'); if (urlTarget.length === 0) return; @@ -2516,7 +2543,7 @@ $(document).ready(async () => { elementId = ''; action = 'clear'; } - updateIssuesMeta(url, action, issueIDs, elementId).then(() => { + updateIssuesMeta(url, action, issueIDs, elementId, '').then(() => { // NOTICE: This reset of checkbox state targets Firefox caching behaviour, as the checkboxes stay checked after reload if (action === 'close' || action === 'open') { // uncheck all checkboxes