Skip to content

Commit ac70163

Browse files
a1012112796zeripath6543
authored
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 <art27@cantab.net> Co-authored-by: 6543 <6543@obermui.de>
1 parent c69c01d commit ac70163

File tree

36 files changed

+593
-39
lines changed

36 files changed

+593
-39
lines changed

‎integrations/api_pull_review_test.go

+16
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,22 @@ func TestAPIPullReview(t *testing.T) {
111111
assert.EqualValues(t, "APPROVED", review.State)
112112
assert.EqualValues(t, 3, review.CodeCommentsCount)
113113

114+
// test dismiss review
115+
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{
116+
Message: "test",
117+
})
118+
resp = session.MakeRequest(t, req, http.StatusOK)
119+
DecodeJSON(t, resp, &review)
120+
assert.EqualValues(t, 6, review.ID)
121+
assert.EqualValues(t, true, review.Dismissed)
122+
123+
// test dismiss review
124+
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))
125+
resp = session.MakeRequest(t, req, http.StatusOK)
126+
DecodeJSON(t, resp, &review)
127+
assert.EqualValues(t, 6, review.ID)
128+
assert.EqualValues(t, false, review.Dismissed)
129+
114130
// test DeletePullReview
115131
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{
116132
Body: "just a comment",

‎models/action.go

+26-25
Original file line numberDiff line numberDiff line change
@@ -26,30 +26,31 @@ type ActionType int
2626

2727
// Possible action types.
2828
const (
29-
ActionCreateRepo ActionType = iota + 1 // 1
30-
ActionRenameRepo // 2
31-
ActionStarRepo // 3
32-
ActionWatchRepo // 4
33-
ActionCommitRepo // 5
34-
ActionCreateIssue // 6
35-
ActionCreatePullRequest // 7
36-
ActionTransferRepo // 8
37-
ActionPushTag // 9
38-
ActionCommentIssue // 10
39-
ActionMergePullRequest // 11
40-
ActionCloseIssue // 12
41-
ActionReopenIssue // 13
42-
ActionClosePullRequest // 14
43-
ActionReopenPullRequest // 15
44-
ActionDeleteTag // 16
45-
ActionDeleteBranch // 17
46-
ActionMirrorSyncPush // 18
47-
ActionMirrorSyncCreate // 19
48-
ActionMirrorSyncDelete // 20
49-
ActionApprovePullRequest // 21
50-
ActionRejectPullRequest // 22
51-
ActionCommentPull // 23
52-
ActionPublishRelease // 24
29+
ActionCreateRepo ActionType = iota + 1 // 1
30+
ActionRenameRepo // 2
31+
ActionStarRepo // 3
32+
ActionWatchRepo // 4
33+
ActionCommitRepo // 5
34+
ActionCreateIssue // 6
35+
ActionCreatePullRequest // 7
36+
ActionTransferRepo // 8
37+
ActionPushTag // 9
38+
ActionCommentIssue // 10
39+
ActionMergePullRequest // 11
40+
ActionCloseIssue // 12
41+
ActionReopenIssue // 13
42+
ActionClosePullRequest // 14
43+
ActionReopenPullRequest // 15
44+
ActionDeleteTag // 16
45+
ActionDeleteBranch // 17
46+
ActionMirrorSyncPush // 18
47+
ActionMirrorSyncCreate // 19
48+
ActionMirrorSyncDelete // 20
49+
ActionApprovePullRequest // 21
50+
ActionRejectPullRequest // 22
51+
ActionCommentPull // 23
52+
ActionPublishRelease // 24
53+
ActionPullReviewDismissed // 25
5354
)
5455

5556
// Action represents user operation type and other information to
@@ -259,7 +260,7 @@ func (a *Action) GetCreate() time.Time {
259260
// GetIssueInfos returns a list of issues associated with
260261
// the action.
261262
func (a *Action) GetIssueInfos() []string {
262-
return strings.SplitN(a.Content, "|", 2)
263+
return strings.SplitN(a.Content, "|", 3)
263264
}
264265

265266
// GetIssueTitle returns the title of first issue associated

‎models/branches.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,8 @@ func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool {
157157
func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 {
158158
sess := x.Where("issue_id = ?", pr.IssueID).
159159
And("type = ?", ReviewTypeApprove).
160-
And("official = ?", true)
160+
And("official = ?", true).
161+
And("dismissed = ?", false)
161162
if protectBranch.DismissStaleApprovals {
162163
sess = sess.And("stale = ?", false)
163164
}
@@ -178,6 +179,7 @@ func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullReque
178179
rejectExist, err := x.Where("issue_id = ?", pr.IssueID).
179180
And("type = ?", ReviewTypeReject).
180181
And("official = ?", true).
182+
And("dismissed = ?", false).
181183
Exist(new(Review))
182184
if err != nil {
183185
log.Error("MergeBlockedByRejectedReview: %v", err)

‎models/fixtures/review.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -104,4 +104,4 @@
104104
issue_id: 12
105105
official: true
106106
updated_unix: 1603196749
107-
created_unix: 1603196749
107+
created_unix: 1603196749

‎models/issue_comment.go

+2
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ const (
9999
CommentTypeProject
100100
// 31 Project board changed
101101
CommentTypeProjectBoard
102+
// Dismiss Review
103+
CommentTypeDismissReview
102104
)
103105

104106
// CommentTag defines comment tag type

‎models/issue_list.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -530,7 +530,7 @@ func (issues IssueList) getApprovalCounts(e Engine) (map[int64][]*ReviewCount, e
530530
}
531531
sess := e.In("issue_id", ids)
532532
err := sess.Select("issue_id, type, count(id) as `count`").
533-
Where("official = ?", true).
533+
Where("official = ? AND dismissed = ?", true, false).
534534
GroupBy("issue_id, type").
535535
OrderBy("issue_id").
536536
Table("review").

‎models/migrations/migrations.go

+2
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,8 @@ var migrations = []Migration{
286286
NewMigration("Recreate user table to fix default values", recreateUserTableToFixDefaultValues),
287287
// v169 -> v170
288288
NewMigration("Update DeleteBranch comments to set the old_ref to the commit_sha", commentTypeDeleteBranchUseOldRef),
289+
// v170 -> v171
290+
NewMigration("Add Dismissed to Review table", addDismissedReviewColumn),
289291
}
290292

291293
// GetCurrentDBVersion returns the current db version

‎models/migrations/v170.go

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright 2021 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package migrations
6+
7+
import (
8+
"fmt"
9+
10+
"xorm.io/xorm"
11+
)
12+
13+
func addDismissedReviewColumn(x *xorm.Engine) error {
14+
type Review struct {
15+
Dismissed bool `xorm:"NOT NULL DEFAULT false"`
16+
}
17+
18+
if err := x.Sync2(new(Review)); err != nil {
19+
return fmt.Errorf("Sync2: %v", err)
20+
}
21+
return nil
22+
}

‎models/pull.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ func (pr *PullRequest) GetApprovalCounts() ([]*ReviewCount, error) {
234234
func (pr *PullRequest) getApprovalCounts(e Engine) ([]*ReviewCount, error) {
235235
rCounts := make([]*ReviewCount, 0, 6)
236236
sess := e.Where("issue_id = ?", pr.IssueID)
237-
return rCounts, sess.Select("issue_id, type, count(id) as `count`").Where("official = ?", true).GroupBy("issue_id, type").Table("review").Find(&rCounts)
237+
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)
238238
}
239239

240240
// GetApprovers returns the approvers of the pull request

‎models/review.go

+19-5
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,10 @@ type Review struct {
6363
IssueID int64 `xorm:"index"`
6464
Content string `xorm:"TEXT"`
6565
// Official is a review made by an assigned approver (counts towards approval)
66-
Official bool `xorm:"NOT NULL DEFAULT false"`
67-
CommitID string `xorm:"VARCHAR(40)"`
68-
Stale bool `xorm:"NOT NULL DEFAULT false"`
66+
Official bool `xorm:"NOT NULL DEFAULT false"`
67+
CommitID string `xorm:"VARCHAR(40)"`
68+
Stale bool `xorm:"NOT NULL DEFAULT false"`
69+
Dismissed bool `xorm:"NOT NULL DEFAULT false"`
6970

7071
CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
7172
UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"`
@@ -466,8 +467,8 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
466467
}
467468

468469
// Get latest review of each reviwer, sorted in order they were made
469-
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",
470-
issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
470+
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",
471+
issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest, false).
471472
Find(&reviews); err != nil {
472473
return nil, err
473474
}
@@ -558,6 +559,19 @@ func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) {
558559
return
559560
}
560561

562+
// DismissReview change the dismiss status of a review
563+
func DismissReview(review *Review, isDismiss bool) (err error) {
564+
if review.Dismissed == isDismiss || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
565+
return nil
566+
}
567+
568+
review.Dismissed = isDismiss
569+
570+
_, err = x.Cols("dismissed").Update(review)
571+
572+
return
573+
}
574+
561575
// InsertReviews inserts review and review comments
562576
func InsertReviews(reviews []*Review) error {
563577
sess := x.NewSession()

‎models/review_test.go

+10
Original file line numberDiff line numberDiff line change
@@ -142,3 +142,13 @@ func TestGetReviewersByIssueID(t *testing.T) {
142142
}
143143
}
144144
}
145+
146+
func TestDismissReview(t *testing.T) {
147+
review1 := AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review)
148+
review2 := AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review)
149+
assert.NoError(t, DismissReview(review1, true))
150+
assert.NoError(t, DismissReview(review2, true))
151+
assert.NoError(t, DismissReview(review2, true))
152+
assert.NoError(t, DismissReview(review2, false))
153+
assert.NoError(t, DismissReview(review2, false))
154+
}

‎modules/convert/pull_review.go

+1
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error)
3434
CommitID: r.CommitID,
3535
Stale: r.Stale,
3636
Official: r.Official,
37+
Dismissed: r.Dismissed,
3738
CodeCommentsCount: r.GetCodeCommentsCount(),
3839
Submitted: r.CreatedUnix.AsTime(),
3940
HTMLURL: r.HTMLURL(),

‎modules/forms/repo_form.go

+6
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,12 @@ func (f SubmitReviewForm) HasEmptyContent() bool {
622622
len(strings.TrimSpace(f.Content)) == 0
623623
}
624624

625+
// DismissReviewForm for dismissing stale review by repo admin
626+
type DismissReviewForm struct {
627+
ReviewID int64 `binding:"Required"`
628+
Message string
629+
}
630+
625631
// __________ .__
626632
// \______ \ ____ | | ____ _____ ______ ____
627633
// | _// __ \| | _/ __ \\__ \ / ___// __ \

‎modules/notification/action/action.go

+20
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,26 @@ func (*actionNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mode
275275
}
276276
}
277277

278+
func (*actionNotifier) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) {
279+
reviewerName := review.Reviewer.Name
280+
if len(review.OriginalAuthor) > 0 {
281+
reviewerName = review.OriginalAuthor
282+
}
283+
if err := models.NotifyWatchers(&models.Action{
284+
ActUserID: doer.ID,
285+
ActUser: doer,
286+
OpType: models.ActionPullReviewDismissed,
287+
Content: fmt.Sprintf("%d|%s|%s", review.Issue.Index, reviewerName, comment.Content),
288+
RepoID: review.Issue.Repo.ID,
289+
Repo: review.Issue.Repo,
290+
IsPrivate: review.Issue.Repo.IsPrivate,
291+
CommentID: comment.ID,
292+
Comment: comment,
293+
}); err != nil {
294+
log.Error("NotifyWatchers [%d]: %v", review.Issue.ID, err)
295+
}
296+
}
297+
278298
func (a *actionNotifier) NotifyPushCommits(pusher *models.User, repo *models.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
279299
data, err := json.Marshal(commits)
280300
if err != nil {

‎modules/notification/base/notifier.go

+1
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ type Notifier interface {
3939
NotifyPullRequestCodeComment(pr *models.PullRequest, comment *models.Comment, mentions []*models.User)
4040
NotifyPullRequestChangeTargetBranch(doer *models.User, pr *models.PullRequest, oldBranch string)
4141
NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, comment *models.Comment)
42+
NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment)
4243

4344
NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
4445
issue *models.Issue, comment *models.Comment, mentions []*models.User)

‎modules/notification/base/null.go

+4
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,10 @@ func (*NullNotifier) NotifyPullRequestChangeTargetBranch(doer *models.User, pr *
6262
func (*NullNotifier) NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, comment *models.Comment) {
6363
}
6464

65+
// NotifyPullRevieweDismiss notifies when a review was dismissed by repo admin
66+
func (*NullNotifier) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) {
67+
}
68+
6569
// NotifyUpdateComment places a place holder function
6670
func (*NullNotifier) NotifyUpdateComment(doer *models.User, c *models.Comment, oldContent string) {
6771
}

‎modules/notification/mail/mail.go

+6
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,12 @@ func (m *mailNotifier) NotifyPullRequestPushCommits(doer *models.User, pr *model
152152
m.NotifyCreateIssueComment(doer, comment.Issue.Repo, comment.Issue, comment, nil)
153153
}
154154

155+
func (m *mailNotifier) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) {
156+
if err := mailer.MailParticipantsComment(comment, models.ActionPullReviewDismissed, review.Issue, []*models.User{}); err != nil {
157+
log.Error("MailParticipantsComment: %v", err)
158+
}
159+
}
160+
155161
func (m *mailNotifier) NotifyNewRelease(rel *models.Release) {
156162
if err := rel.LoadAttributes(); err != nil {
157163
log.Error("NotifyNewRelease: %v", err)

‎modules/notification/notification.go

+7
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,13 @@ func NotifyPullRequestPushCommits(doer *models.User, pr *models.PullRequest, com
108108
}
109109
}
110110

111+
// NotifyPullRevieweDismiss notifies when a review was dismissed by repo admin
112+
func NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) {
113+
for _, notifier := range notifiers {
114+
notifier.NotifyPullRevieweDismiss(doer, review, comment)
115+
}
116+
}
117+
111118
// NotifyUpdateComment notifies update comment to notifiers
112119
func NotifyUpdateComment(doer *models.User, c *models.Comment, oldContent string) {
113120
for _, notifier := range notifiers {

‎modules/notification/ui/ui.go

+9
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,15 @@ func (ns *notificationService) NotifyPullRequestPushCommits(doer *models.User, p
161161
_ = ns.issueQueue.Push(opts)
162162
}
163163

164+
func (ns *notificationService) NotifyPullRevieweDismiss(doer *models.User, review *models.Review, comment *models.Comment) {
165+
var opts = issueNotificationOpts{
166+
IssueID: review.IssueID,
167+
NotificationAuthorID: doer.ID,
168+
CommentID: comment.ID,
169+
}
170+
_ = ns.issueQueue.Push(opts)
171+
}
172+
164173
func (ns *notificationService) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) {
165174
if !removed {
166175
var opts = issueNotificationOpts{

‎modules/structs/pull_review.go

+6
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ type PullReview struct {
3636
CommitID string `json:"commit_id"`
3737
Stale bool `json:"stale"`
3838
Official bool `json:"official"`
39+
Dismissed bool `json:"dismissed"`
3940
CodeCommentsCount int `json:"comments_count"`
4041
// swagger:strfmt date-time
4142
Submitted time.Time `json:"submitted_at"`
@@ -92,6 +93,11 @@ type SubmitPullReviewOptions struct {
9293
Body string `json:"body"`
9394
}
9495

96+
// DismissPullReviewOptions are options to dismiss a pull review
97+
type DismissPullReviewOptions struct {
98+
Message string `json:"message"`
99+
}
100+
95101
// PullReviewRequestOptions are options to add or remove pull review requests
96102
type PullReviewRequestOptions struct {
97103
Reviewers []string `json:"reviewers"`

‎modules/templates/helper.go

+2
Original file line numberDiff line numberDiff line change
@@ -798,6 +798,8 @@ func ActionIcon(opType models.ActionType) string {
798798
return "diff"
799799
case models.ActionPublishRelease:
800800
return "tag"
801+
case models.ActionPullReviewDismissed:
802+
return "x"
801803
default:
802804
return "question"
803805
}

0 commit comments

Comments
 (0)