From ac701637b42d2d6bb5fe9b258f3f54959b6a505e Mon Sep 17 00:00:00 2001 From: a1012112796 <1012112796@qq.com> Date: Fri, 12 Feb 2021 01:32:25 +0800 Subject: [PATCH] Add dismiss review feature (#12674) * Add dismiss review feature refs: https://github.blog/2016-10-12-dismissing-reviews-on-pull-requests/ https://developer.github.com/v3/pulls/reviews/#dismiss-a-review-for-a-pull-request * change modal ui and error message * Add unDismissReview api Signed-off-by: a1012112796 <1012112796@qq.com> Co-authored-by: zeripath Co-authored-by: 6543 <6543@obermui.de> --- integrations/api_pull_review_test.go | 16 +++ models/action.go | 51 +++---- models/branches.go | 4 +- models/fixtures/review.yml | 2 +- models/issue_comment.go | 2 + models/issue_list.go | 2 +- models/migrations/migrations.go | 2 + models/migrations/v170.go | 22 +++ models/pull.go | 2 +- models/review.go | 24 +++- models/review_test.go | 10 ++ modules/convert/pull_review.go | 1 + modules/forms/repo_form.go | 6 + modules/notification/action/action.go | 20 +++ modules/notification/base/notifier.go | 1 + modules/notification/base/null.go | 4 + modules/notification/mail/mail.go | 6 + modules/notification/notification.go | 7 + modules/notification/ui/ui.go | 9 ++ modules/structs/pull_review.go | 6 + modules/templates/helper.go | 2 + options/locale/locale_en-US.ini | 7 + routers/api/v1/api.go | 2 + routers/api/v1/repo/pull_review.go | 126 +++++++++++++++++ routers/api/v1/swagger/options.go | 3 + routers/repo/issue.go | 2 +- routers/repo/pull_review.go | 12 ++ routers/routes/web.go | 1 + services/mailer/mail.go | 2 + services/pull/review.go | 51 +++++++ templates/mail/issue/default.tmpl | 2 + .../repo/issue/view_content/comments.tmpl | 45 +++++- templates/repo/issue/view_content/pull.tmpl | 33 ++++- templates/swagger/v1_json.tmpl | 133 ++++++++++++++++++ templates/user/dashboard/feeds.tmpl | 7 + web_src/js/index.js | 7 + 36 files changed, 593 insertions(+), 39 deletions(-) create mode 100644 models/migrations/v170.go diff --git a/integrations/api_pull_review_test.go b/integrations/api_pull_review_test.go index 261a3a8bfa..8be194602f 100644 --- a/integrations/api_pull_review_test.go +++ b/integrations/api_pull_review_test.go @@ -111,6 +111,22 @@ func TestAPIPullReview(t *testing.T) { assert.EqualValues(t, "APPROVED", review.State) assert.EqualValues(t, 3, review.CodeCommentsCount) + // test dismiss review + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews/%d/dismissals?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, review.ID, token), &api.DismissPullReviewOptions{ + Message: "test", + }) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &review) + assert.EqualValues(t, 6, review.ID) + assert.EqualValues(t, true, review.Dismissed) + + // test dismiss review + req = NewRequest(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews/%d/undismissals?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, review.ID, token)) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &review) + assert.EqualValues(t, 6, review.ID) + assert.EqualValues(t, false, review.Dismissed) + // test DeletePullReview req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.CreatePullReviewOptions{ Body: "just a comment", diff --git a/models/action.go b/models/action.go index 2fdab7f4e9..e8a1336566 100644 --- a/models/action.go +++ b/models/action.go @@ -26,30 +26,31 @@ type ActionType int // Possible action types. const ( - ActionCreateRepo ActionType = iota + 1 // 1 - ActionRenameRepo // 2 - ActionStarRepo // 3 - ActionWatchRepo // 4 - ActionCommitRepo // 5 - ActionCreateIssue // 6 - ActionCreatePullRequest // 7 - ActionTransferRepo // 8 - ActionPushTag // 9 - ActionCommentIssue // 10 - ActionMergePullRequest // 11 - ActionCloseIssue // 12 - ActionReopenIssue // 13 - ActionClosePullRequest // 14 - ActionReopenPullRequest // 15 - ActionDeleteTag // 16 - ActionDeleteBranch // 17 - ActionMirrorSyncPush // 18 - ActionMirrorSyncCreate // 19 - ActionMirrorSyncDelete // 20 - ActionApprovePullRequest // 21 - ActionRejectPullRequest // 22 - ActionCommentPull // 23 - ActionPublishRelease // 24 + ActionCreateRepo ActionType = iota + 1 // 1 + ActionRenameRepo // 2 + ActionStarRepo // 3 + ActionWatchRepo // 4 + ActionCommitRepo // 5 + ActionCreateIssue // 6 + ActionCreatePullRequest // 7 + ActionTransferRepo // 8 + ActionPushTag // 9 + ActionCommentIssue // 10 + ActionMergePullRequest // 11 + ActionCloseIssue // 12 + ActionReopenIssue // 13 + ActionClosePullRequest // 14 + ActionReopenPullRequest // 15 + ActionDeleteTag // 16 + ActionDeleteBranch // 17 + ActionMirrorSyncPush // 18 + ActionMirrorSyncCreate // 19 + ActionMirrorSyncDelete // 20 + ActionApprovePullRequest // 21 + ActionRejectPullRequest // 22 + ActionCommentPull // 23 + ActionPublishRelease // 24 + ActionPullReviewDismissed // 25 ) // Action represents user operation type and other information to @@ -259,7 +260,7 @@ func (a *Action) GetCreate() time.Time { // GetIssueInfos returns a list of issues associated with // the action. func (a *Action) GetIssueInfos() []string { - return strings.SplitN(a.Content, "|", 2) + return strings.SplitN(a.Content, "|", 3) } // GetIssueTitle returns the title of first issue associated diff --git a/models/branches.go b/models/branches.go index 80df8c433e..440c093095 100644 --- a/models/branches.go +++ b/models/branches.go @@ -157,7 +157,8 @@ func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 { sess := x.Where("issue_id = ?", pr.IssueID). And("type = ?", ReviewTypeApprove). - And("official = ?", true) + And("official = ?", true). + And("dismissed = ?", false) if protectBranch.DismissStaleApprovals { sess = sess.And("stale = ?", false) } @@ -178,6 +179,7 @@ func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullReque rejectExist, err := x.Where("issue_id = ?", pr.IssueID). And("type = ?", ReviewTypeReject). And("official = ?", true). + And("dismissed = ?", false). Exist(new(Review)) if err != nil { log.Error("MergeBlockedByRejectedReview: %v", err) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index c7c16fb109..d44d0cde98 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -104,4 +104,4 @@ issue_id: 12 official: true updated_unix: 1603196749 - created_unix: 1603196749 \ No newline at end of file + created_unix: 1603196749 diff --git a/models/issue_comment.go b/models/issue_comment.go index ea179d49d9..b15b5169ff 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -99,6 +99,8 @@ const ( CommentTypeProject // 31 Project board changed CommentTypeProjectBoard + // Dismiss Review + CommentTypeDismissReview ) // CommentTag defines comment tag type diff --git a/models/issue_list.go b/models/issue_list.go index 628058eb35..5789ad84ae 100644 --- a/models/issue_list.go +++ b/models/issue_list.go @@ -530,7 +530,7 @@ func (issues IssueList) getApprovalCounts(e Engine) (map[int64][]*ReviewCount, e } sess := e.In("issue_id", ids) err := sess.Select("issue_id, type, count(id) as `count`"). - Where("official = ?", true). + Where("official = ? AND dismissed = ?", true, false). GroupBy("issue_id, type"). OrderBy("issue_id"). Table("review"). diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index c926ee6ccf..16e2f177ad 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -286,6 +286,8 @@ var migrations = []Migration{ NewMigration("Recreate user table to fix default values", recreateUserTableToFixDefaultValues), // v169 -> v170 NewMigration("Update DeleteBranch comments to set the old_ref to the commit_sha", commentTypeDeleteBranchUseOldRef), + // v170 -> v171 + NewMigration("Add Dismissed to Review table", addDismissedReviewColumn), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v170.go b/models/migrations/v170.go new file mode 100644 index 0000000000..853a23d290 --- /dev/null +++ b/models/migrations/v170.go @@ -0,0 +1,22 @@ +// Copyright 2021 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 ( + "fmt" + + "xorm.io/xorm" +) + +func addDismissedReviewColumn(x *xorm.Engine) error { + type Review struct { + Dismissed bool `xorm:"NOT NULL DEFAULT false"` + } + + if err := x.Sync2(new(Review)); err != nil { + return fmt.Errorf("Sync2: %v", err) + } + return nil +} diff --git a/models/pull.go b/models/pull.go index 9b6f0830d7..0d4691aac9 100644 --- a/models/pull.go +++ b/models/pull.go @@ -234,7 +234,7 @@ func (pr *PullRequest) GetApprovalCounts() ([]*ReviewCount, error) { func (pr *PullRequest) getApprovalCounts(e Engine) ([]*ReviewCount, error) { rCounts := make([]*ReviewCount, 0, 6) sess := e.Where("issue_id = ?", pr.IssueID) - return rCounts, sess.Select("issue_id, type, count(id) as `count`").Where("official = ?", true).GroupBy("issue_id, type").Table("review").Find(&rCounts) + return rCounts, sess.Select("issue_id, type, count(id) as `count`").Where("official = ? AND dismissed = ?", true, false).GroupBy("issue_id, type").Table("review").Find(&rCounts) } // GetApprovers returns the approvers of the pull request diff --git a/models/review.go b/models/review.go index aeb5f21ea9..7775fcdf53 100644 --- a/models/review.go +++ b/models/review.go @@ -63,9 +63,10 @@ type Review struct { IssueID int64 `xorm:"index"` Content string `xorm:"TEXT"` // Official is a review made by an assigned approver (counts towards approval) - Official bool `xorm:"NOT NULL DEFAULT false"` - CommitID string `xorm:"VARCHAR(40)"` - Stale bool `xorm:"NOT NULL DEFAULT false"` + Official bool `xorm:"NOT NULL DEFAULT false"` + CommitID string `xorm:"VARCHAR(40)"` + Stale bool `xorm:"NOT NULL DEFAULT false"` + Dismissed bool `xorm:"NOT NULL DEFAULT false"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` @@ -466,8 +467,8 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, 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 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). + 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 dismissed = ? AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC", + issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest, false). Find(&reviews); err != nil { return nil, err } @@ -558,6 +559,19 @@ func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) { return } +// DismissReview change the dismiss status of a review +func DismissReview(review *Review, isDismiss bool) (err error) { + if review.Dismissed == isDismiss || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) { + return nil + } + + review.Dismissed = isDismiss + + _, err = x.Cols("dismissed").Update(review) + + return +} + // InsertReviews inserts review and review comments func InsertReviews(reviews []*Review) error { sess := x.NewSession() diff --git a/models/review_test.go b/models/review_test.go index 702e216824..7315650488 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -142,3 +142,13 @@ func TestGetReviewersByIssueID(t *testing.T) { } } } + +func TestDismissReview(t *testing.T) { + review1 := AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review2 := AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.NoError(t, DismissReview(review1, true)) + assert.NoError(t, DismissReview(review2, true)) + assert.NoError(t, DismissReview(review2, true)) + assert.NoError(t, DismissReview(review2, false)) + assert.NoError(t, DismissReview(review2, false)) +} diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index 0ef1fec39c..d1d6e767d4 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -34,6 +34,7 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) CommitID: r.CommitID, Stale: r.Stale, Official: r.Official, + Dismissed: r.Dismissed, CodeCommentsCount: r.GetCodeCommentsCount(), Submitted: r.CreatedUnix.AsTime(), HTMLURL: r.HTMLURL(), diff --git a/modules/forms/repo_form.go b/modules/forms/repo_form.go index f177b21f05..48af3450f3 100644 --- a/modules/forms/repo_form.go +++ b/modules/forms/repo_form.go @@ -622,6 +622,12 @@ func (f SubmitReviewForm) HasEmptyContent() bool { len(strings.TrimSpace(f.Content)) == 0 } +// DismissReviewForm for dismissing stale review by repo admin +type DismissReviewForm struct { + ReviewID int64 `binding:"Required"` + Message string +} + // __________ .__ // \______ \ ____ | | ____ _____ ______ ____ // | _// __ \| | _/ __ \\__ \ / ___// __ \ diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go index 360906f076..836cb51b3e 100644 --- a/modules/notification/action/action.go +++ b/modules/notification/action/action.go @@ -275,6 +275,26 @@ func (*actionNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mode } } +func (*actionNotifier) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) { + reviewerName := review.Reviewer.Name + if len(review.OriginalAuthor) > 0 { + reviewerName = review.OriginalAuthor + } + if err := models.NotifyWatchers(&models.Action{ + ActUserID: doer.ID, + ActUser: doer, + OpType: models.ActionPullReviewDismissed, + Content: fmt.Sprintf("%d|%s|%s", review.Issue.Index, reviewerName, comment.Content), + RepoID: review.Issue.Repo.ID, + Repo: review.Issue.Repo, + IsPrivate: review.Issue.Repo.IsPrivate, + CommentID: comment.ID, + Comment: comment, + }); err != nil { + log.Error("NotifyWatchers [%d]: %v", review.Issue.ID, err) + } +} + func (a *actionNotifier) NotifyPushCommits(pusher *models.User, repo *models.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { data, err := json.Marshal(commits) if err != nil { diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index b01026dfc5..5bb833d275 100644 --- a/modules/notification/base/notifier.go +++ b/modules/notification/base/notifier.go @@ -39,6 +39,7 @@ type Notifier interface { NotifyPullRequestCodeComment(pr *models.PullRequest, comment *models.Comment, mentions []*models.User) NotifyPullRequestChangeTargetBranch(doer *models.User, pr *models.PullRequest, oldBranch string) NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, comment *models.Comment) + NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) NotifyCreateIssueComment(doer *models.User, repo *models.Repository, issue *models.Issue, comment *models.Comment, mentions []*models.User) diff --git a/modules/notification/base/null.go b/modules/notification/base/null.go index d80ba859f3..2386f925ce 100644 --- a/modules/notification/base/null.go +++ b/modules/notification/base/null.go @@ -62,6 +62,10 @@ func (*NullNotifier) NotifyPullRequestChangeTargetBranch(doer *models.User, pr * func (*NullNotifier) NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, comment *models.Comment) { } +// NotifyPullRevieweDismiss notifies when a review was dismissed by repo admin +func (*NullNotifier) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) { +} + // NotifyUpdateComment places a place holder function func (*NullNotifier) NotifyUpdateComment(doer *models.User, c *models.Comment, oldContent string) { } diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index ee8a0c436c..f984ea7661 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -152,6 +152,12 @@ func (m *mailNotifier) NotifyPullRequestPushCommits(doer *models.User, pr *model m.NotifyCreateIssueComment(doer, comment.Issue.Repo, comment.Issue, comment, nil) } +func (m *mailNotifier) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) { + if err := mailer.MailParticipantsComment(comment, models.ActionPullReviewDismissed, review.Issue, []*models.User{}); err != nil { + log.Error("MailParticipantsComment: %v", err) + } +} + func (m *mailNotifier) NotifyNewRelease(rel *models.Release) { if err := rel.LoadAttributes(); err != nil { log.Error("NotifyNewRelease: %v", err) diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 7ced57ce2d..d22d157bec 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -108,6 +108,13 @@ func NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, com } } +// NotifyPullRevieweDismiss notifies when a review was dismissed by repo admin +func NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) { + for _, notifier := range notifiers { + notifier.NotifyPullRevieweDismiss(doer, review, comment) + } +} + // NotifyUpdateComment notifies update comment to notifiers func NotifyUpdateComment(doer *models.User, c *models.Comment, oldContent string) { for _, notifier := range notifiers { diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index 8e510e9cd4..25ea4d91c6 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -161,6 +161,15 @@ func (ns *notificationService) NotifyPullRequestPushCommits(doer *models.User, p _ = ns.issueQueue.Push(opts) } +func (ns *notificationService) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) { + var opts = issueNotificationOpts{ + IssueID: review.IssueID, + NotificationAuthorID: doer.ID, + CommentID: comment.ID, + } + _ = 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{ diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 07fa968d28..261d00fde8 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -36,6 +36,7 @@ type PullReview struct { CommitID string `json:"commit_id"` Stale bool `json:"stale"` Official bool `json:"official"` + Dismissed bool `json:"dismissed"` CodeCommentsCount int `json:"comments_count"` // swagger:strfmt date-time Submitted time.Time `json:"submitted_at"` @@ -92,6 +93,11 @@ type SubmitPullReviewOptions struct { Body string `json:"body"` } +// DismissPullReviewOptions are options to dismiss a pull review +type DismissPullReviewOptions struct { + Message string `json:"message"` +} + // PullReviewRequestOptions are options to add or remove pull review requests type PullReviewRequestOptions struct { Reviewers []string `json:"reviewers"` diff --git a/modules/templates/helper.go b/modules/templates/helper.go index 987a6ad983..b8e4f5d505 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -798,6 +798,8 @@ func ActionIcon(opType models.ActionType) string { return "diff" case models.ActionPublishRelease: return "tag" + case models.ActionPullReviewDismissed: + return "x" default: return "question" } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index a4b677e43b..767696cfb9 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -76,6 +76,7 @@ pull_requests = Pull Requests issues = Issues milestones = Milestones +ok = OK cancel = Cancel save = Save add = Add @@ -1104,6 +1105,8 @@ issues.re_request_review=Re-request review issues.is_stale = There have been changes to this PR since this review issues.remove_request_review=Remove review request issues.remove_request_review_block=Can't remove review request +issues.dismiss_review = Dismiss Review +issues.dismiss_review_warning = Are you sure you want to dismiss this review? issues.sign_in_require_desc = Sign in to join this conversation. issues.edit = Edit issues.cancel = Cancel @@ -1216,6 +1219,8 @@ issues.review.self.approval = You cannot approve your own pull request. issues.review.self.rejection = You cannot request changes on your own pull request. issues.review.approve = "approved these changes %s" issues.review.comment = "reviewed %s" +issues.review.dismissed = "dismissed %s’s review %s" +issues.review.dismissed_label = Dismissed issues.review.left_comment = left a comment issues.review.content.empty = You need to leave a comment indicating the requested change(s). issues.review.reject = "requested changes %s" @@ -2519,6 +2524,8 @@ mirror_sync_delete = synced and deleted reference %[2]s at %s#%[2]s` reject_pull_request = `suggested changes for %s#%[2]s` publish_release = `released "%[4]s" at %[3]s` +review_dismissed = `dismissed review from %[4]s for %[3]s#%[2]s` +review_dismissed_reason = Reason: create_branch = created branch %[3]s in %[4]s [tool] diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 9c21107a28..85c4e4d5bf 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -891,6 +891,8 @@ func Routes() *web.Route { Post(reqToken(), bind(api.SubmitPullReviewOptions{}), repo.SubmitPullReview) m.Combo("/comments"). Get(repo.GetPullReviewComments) + m.Post("/dismissals", reqToken(), bind(api.DismissPullReviewOptions{}), repo.DismissPullReview) + m.Post("/undismissals", reqToken(), repo.UnDismissPullReview) }) }) m.Combo("/requested_reviewers"). diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index d39db4c660..63179aa990 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -757,3 +757,129 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions return } } + +// DismissPullReview dismiss a review for a pull request +func DismissPullReview(ctx *context.APIContext) { + // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/reviews/{id}/dismissals repository repoDismissPullReview + // --- + // summary: Dismiss a review for a pull request + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: id + // in: path + // description: id of the review + // type: integer + // format: int64 + // required: true + // - name: body + // in: body + // required: true + // schema: + // "$ref": "#/definitions/DismissPullReviewOptions" + // responses: + // "200": + // "$ref": "#/responses/PullReview" + // "403": + // "$ref": "#/responses/forbidden" + // "422": + // "$ref": "#/responses/validationError" + opts := web.GetForm(ctx).(*api.DismissPullReviewOptions) + dismissReview(ctx, opts.Message, true) +} + +// UnDismissPullReview cancel to dismiss a review for a pull request +func UnDismissPullReview(ctx *context.APIContext) { + // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/reviews/{id}/undismissals repository repoUnDismissPullReview + // --- + // summary: Cancel to dismiss a review for a pull request + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: id + // in: path + // description: id of the review + // type: integer + // format: int64 + // required: true + // responses: + // "200": + // "$ref": "#/responses/PullReview" + // "403": + // "$ref": "#/responses/forbidden" + // "422": + // "$ref": "#/responses/validationError" + dismissReview(ctx, "", false) +} + +func dismissReview(ctx *context.APIContext, msg string, isDismiss bool) { + if !ctx.Repo.IsAdmin() { + ctx.Error(http.StatusForbidden, "", "Must be repo admin") + return + } + review, pr, isWrong := prepareSingleReview(ctx) + if isWrong { + return + } + + if review.Type != models.ReviewTypeApprove && review.Type != models.ReviewTypeReject { + ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because it's type is not Approve or change request") + return + } + + if pr.Issue.IsClosed { + ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed") + return + } + + _, err := pull_service.DismissReview(review.ID, msg, ctx.User, isDismiss) + if err != nil { + ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err) + return + } + + if review, err = models.GetReviewByID(review.ID); err != nil { + ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) + return + } + + // convert response + apiReview, err := convert.ToPullReview(review, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReview", err) + return + } + ctx.JSON(http.StatusOK, apiReview) +} diff --git a/routers/api/v1/swagger/options.go b/routers/api/v1/swagger/options.go index 8919a969ec..a2dc2193a8 100644 --- a/routers/api/v1/swagger/options.go +++ b/routers/api/v1/swagger/options.go @@ -150,6 +150,9 @@ type swaggerParameterBodies struct { // in:body SubmitPullReviewOptions api.SubmitPullReviewOptions + // in:body + DismissPullReviewOptions api.DismissPullReviewOptions + // in:body MigrateRepoOptions api.MigrateRepoOptions diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 71c8f1efbb..fa1ee99771 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1364,7 +1364,7 @@ func ViewIssue(ctx *context.Context) { return } } - } else if comment.Type == models.CommentTypeCode || comment.Type == models.CommentTypeReview { + } else if comment.Type == models.CommentTypeCode || comment.Type == models.CommentTypeReview || comment.Type == models.CommentTypeDismissReview { comment.RenderedContent = string(markdown.Render([]byte(comment.Content), ctx.Repo.RepoLink, ctx.Repo.Repository.ComposeMetas())) if err = comment.LoadReview(); err != nil && !models.IsErrReviewNotExist(err) { diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index df49b6cfe1..89e87ccc44 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -223,3 +223,15 @@ func SubmitReview(ctx *context.Context) { ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag())) } + +// DismissReview dismissing stale review by repo admin +func DismissReview(ctx *context.Context) { + form := web.GetForm(ctx).(*auth.DismissReviewForm) + comm, err := pull_service.DismissReview(form.ReviewID, form.Message, ctx.User, true) + if err != nil { + ctx.ServerError("pull_service.DismissReview", err) + return + } + + ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag())) +} diff --git a/routers/routes/web.go b/routers/routes/web.go index 9e3e690fb9..2f28e567f9 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -734,6 +734,7 @@ func RegisterRoutes(m *web.Route) { m.Post("/projects", reqRepoIssuesOrPullsWriter, repo.UpdateIssueProject) m.Post("/assignee", reqRepoIssuesOrPullsWriter, repo.UpdateIssueAssignee) m.Post("/request_review", reqRepoIssuesOrPullsReader, repo.UpdatePullReviewRequest) + m.Post("/dismiss_review", reqRepoAdmin, bindIgnErr(auth.DismissReviewForm{}), repo.DismissReview) m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus) m.Post("/resolve_conversation", reqRepoIssuesOrPullsReader, repo.UpdateResolveConversation) m.Post("/attachments", repo.UploadIssueAttachment) diff --git a/services/mailer/mail.go b/services/mailer/mail.go index b4217c0466..e87d34ab29 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -304,6 +304,8 @@ func actionToTemplate(issue *models.Issue, actionType models.ActionType, name = "reopen" case models.ActionMergePullRequest: name = "merge" + case models.ActionPullReviewDismissed: + name = "review_dismissed" default: switch commentType { case models.CommentTypeReview: diff --git a/services/pull/review.go b/services/pull/review.go index 8994a9e78a..4e77e11daa 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -253,3 +253,54 @@ func SubmitReview(doer *models.User, gitRepo *git.Repository, issue *models.Issu return review, comm, nil } + +// DismissReview dismissing stale review by repo admin +func DismissReview(reviewID int64, message string, doer *models.User, isDismiss bool) (comment *models.Comment, err error) { + review, err := models.GetReviewByID(reviewID) + if err != nil { + return + } + + if review.Type != models.ReviewTypeApprove && review.Type != models.ReviewTypeReject { + return nil, fmt.Errorf("not need to dismiss this review because it's type is not Approve or change request") + } + + if err = models.DismissReview(review, isDismiss); err != nil { + return + } + + if !isDismiss { + return nil, nil + } + + // load data for notify + if err = review.LoadAttributes(); err != nil { + return + } + if err = review.Issue.LoadPullRequest(); err != nil { + return + } + if err = review.Issue.LoadAttributes(); err != nil { + return + } + + comment, err = models.CreateComment(&models.CreateCommentOptions{ + Doer: doer, + Content: message, + Type: models.CommentTypeDismissReview, + ReviewID: review.ID, + Issue: review.Issue, + Repo: review.Issue.Repo, + }) + if err != nil { + return + } + + comment.Review = review + comment.Poster = doer + comment.Issue = review.Issue + + notification.NotifyPullRevieweDismiss(doer, review, comment) + + return +} diff --git a/templates/mail/issue/default.tmpl b/templates/mail/issue/default.tmpl index e062dca7f1..b7d576bef4 100644 --- a/templates/mail/issue/default.tmpl +++ b/templates/mail/issue/default.tmpl @@ -49,6 +49,8 @@ @{{.Doer.Name}} requested changes on this pull request. {{else if eq .ActionName "review"}} @{{.Doer.Name}} commented on this pull request. + {{else if eq .ActionName "review_dismissed"}} + @{{.Doer.Name}} dismissed last review from {{.Comment.Review.Reviewer.Name}} for this pull request. {{end}} {{- if eq .Body ""}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 63fe73857c..b971c6b1ae 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -8,7 +8,8 @@ 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, 27 = REVIEW_REQUEST, 28 = MERGE_PULL_REQUEST, - 29 = PULL_PUSH_EVENT, 30 = PROJECT_CHANGED, 31 = PROJECT_BOARD_CHANGED --> + 29 = PULL_PUSH_EVENT, 30 = PROJECT_CHANGED, 31 = PROJECT_BOARD_CHANGED + 32 = DISMISSED_REVIEW --> {{if eq .Type 0}}
{{if .OriginalAuthor }} @@ -415,6 +416,9 @@ {{else}} {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} {{end}} + {{if .Review.Dismissed}} +
{{$.i18n.Tr "repo.issues.review.dismissed_label"}}
+ {{end}}
{{if .Content}} @@ -698,5 +702,44 @@ {{end}} + {{else if eq .Type 32}} +
+
+ + + + {{svg "octicon-x" 16}} + + {{.Poster.GetDisplayName}} + {{$reviewerName := ""}} + {{if eq .Review.OriginalAuthor ""}} + {{$reviewerName = .Review.Reviewer.Name}} + {{else}} + {{$reviewerName = .Review.OriginalAuthor}} + {{end}} + {{$.i18n.Tr "repo.issues.review.dismissed" $reviewerName $createdStr | Safe}} + +
+ {{if .Content}} +
+
+
+ + {{$.i18n.Tr "action.review_dismissed_reason"}} + +
+
+
+ {{if .RenderedContent}} + {{.RenderedContent|Str2html}} + {{else}} + {{$.i18n.Tr "repo.issues.no_content"}} + {{end}} +
+
+
+
+ {{end}} +
{{end}} {{end}} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 34eaa83eb2..9e883c0a93 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -34,9 +34,36 @@
{{if .Review.Stale}} - - - + + + + {{end}} + {{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed))}} + + {{svg "octicon-x" 16}} + + {{end}} {{if or (eq .GetOpType 5) (eq .GetOpType 18)}} @@ -111,6 +115,9 @@

{{index .GetIssueInfos 1}}

{{else if or (eq .GetOpType 12) (eq .GetOpType 13) (eq .GetOpType 14) (eq .GetOpType 15)}} {{.GetIssueTitle | RenderEmoji}} + {{else if eq .GetOpType 25}} +

{{$.i18n.Tr "action.review_dismissed_reason"}}

+

{{index .GetIssueInfos 2 | RenderEmoji}}

{{end}}

{{TimeSince .GetCreate $.i18n.Lang}}

diff --git a/web_src/js/index.js b/web_src/js/index.js index f5f4841410..0d60c21cca 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -677,6 +677,13 @@ function initIssueComments() { return false; }); + $('.dismiss-review-btn').on('click', function (e) { + e.preventDefault(); + const $this = $(this); + const $dismissReviewModal = $this.next(); + $dismissReviewModal.modal('show'); + }); + $(document).on('click', (event) => { const urlTarget = $(':target'); if (urlTarget.length === 0) return;