Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dismiss review feature #12674

Merged
merged 28 commits into from
Feb 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
4c5a98f
Add dismiss review feature
a1012112796 Sep 1, 2020
a45b133
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Sep 1, 2020
58a6148
make fmt
a1012112796 Sep 1, 2020
2b84a0a
fix test
a1012112796 Sep 1, 2020
365fc83
fix test
a1012112796 Sep 1, 2020
8e57c49
change modal ui and error message
a1012112796 Sep 3, 2020
ca6282d
Add unDismissReview api
a1012112796 Sep 3, 2020
ab6c402
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Sep 3, 2020
b969c85
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Nov 8, 2020
dbaebf7
fix bugs
a1012112796 Nov 8, 2020
35dedbb
fix test
a1012112796 Nov 8, 2020
46cbcc2
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Nov 20, 2020
a060513
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Nov 22, 2020
26a5999
simplify logic
a1012112796 Nov 22, 2020
786f823
fix bug
a1012112796 Nov 22, 2020
c599356
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Jan 25, 2021
0feea44
Update options/locale/locale_en-US.ini
6543 Jan 26, 2021
8f5873d
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Jan 27, 2021
f323120
fix bug
a1012112796 Jan 27, 2021
e5bb544
Merge branch 'master' into feature/dismissing_reviews
6543 Jan 28, 2021
456db50
apply suggestion
6543 Jan 28, 2021
d017a10
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Feb 3, 2021
9b9d7b3
Apply suggestions from code review
a1012112796 Feb 9, 2021
28a7f96
fix nit
a1012112796 Feb 9, 2021
349c294
Merge branch 'master' into feature/dismissing_reviews
a1012112796 Feb 9, 2021
91091fa
Merge branch 'master' into feature/dismissing_reviews
zeripath Feb 10, 2021
02f445a
Merge branch 'master' into feature/dismissing_reviews
zeripath Feb 10, 2021
5897902
Merge branch 'master' into feature/dismissing_reviews
6543 Feb 11, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions integrations/api_pull_review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
51 changes: 26 additions & 25 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
zeripath marked this conversation as resolved.
Show resolved Hide resolved
)

// Action represents user operation type and other information to
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion models/branches.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion models/fixtures/review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,4 @@
issue_id: 12
official: true
updated_unix: 1603196749
created_unix: 1603196749
created_unix: 1603196749
2 changes: 2 additions & 0 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ const (
CommentTypeProject
// 31 Project board changed
CommentTypeProjectBoard
// Dismiss Review
CommentTypeDismissReview
)

// CommentTag defines comment tag type
Expand Down
2 changes: 1 addition & 1 deletion models/issue_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -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").
Expand Down
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 22 additions & 0 deletions models/migrations/v170.go
Original file line number Diff line number Diff line change
@@ -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
}
2 changes: 1 addition & 1 deletion models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 19 additions & 5 deletions models/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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()
Expand Down
10 changes: 10 additions & 0 deletions models/review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
1 change: 1 addition & 0 deletions modules/convert/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
6 changes: 6 additions & 0 deletions modules/forms/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

// __________ .__
// \______ \ ____ | | ____ _____ ______ ____
// | _// __ \| | _/ __ \\__ \ / ___// __ \
Expand Down
20 changes: 20 additions & 0 deletions modules/notification/action/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
zeripath marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand Down
1 change: 1 addition & 0 deletions modules/notification/base/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions modules/notification/base/null.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
}
Expand Down
6 changes: 6 additions & 0 deletions modules/notification/mail/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions modules/notification/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 9 additions & 0 deletions modules/notification/ui/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
6 changes: 6 additions & 0 deletions modules/structs/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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"`
Expand Down
2 changes: 2 additions & 0 deletions modules/templates/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down
Loading