From edd4ab49c8c0554a4983effc1d0b5487191717a5 Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 15 Apr 2021 16:24:59 +0100 Subject: [PATCH] Ensure review dismissal only dismisses the correct review (#15477) (#15489) Backport #15477 Fix #15472 Signed-off-by: Andrew Thornton art27@cantab.net Co-authored-by: 6543 <6543@obermui.de> --- models/review.go | 6 ++++- models/review_test.go | 60 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/models/review.go b/models/review.go index 702e9634ef799..343621c0fa5ab 100644 --- a/models/review.go +++ b/models/review.go @@ -566,7 +566,11 @@ func DismissReview(review *Review, isDismiss bool) (err error) { review.Dismissed = isDismiss - _, err = x.Cols("dismissed").Update(review) + if review.ID == 0 { + return ErrReviewNotExist{} + } + + _, err = x.ID(review.ID).Cols("dismissed").Update(review) return } diff --git a/models/review_test.go b/models/review_test.go index 4f049b45e39d7..accc1841933ec 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -143,11 +143,57 @@ 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)) + assert.NoError(t, PrepareTestDatabase()) + + rejectReviewExample := AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + requestReviewExample := AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + approveReviewExample := AssertExistsAndLoadBean(t, &Review{ID: 8}).(*Review) + assert.False(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.False(t, approveReviewExample.Dismissed) + + assert.NoError(t, DismissReview(rejectReviewExample, true)) + rejectReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + requestReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + + assert.NoError(t, DismissReview(requestReviewExample, true)) + rejectReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + requestReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.False(t, approveReviewExample.Dismissed) + + assert.NoError(t, DismissReview(requestReviewExample, true)) + rejectReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + requestReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.False(t, approveReviewExample.Dismissed) + + assert.NoError(t, DismissReview(requestReviewExample, false)) + rejectReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + requestReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.False(t, approveReviewExample.Dismissed) + + assert.NoError(t, DismissReview(requestReviewExample, false)) + rejectReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + requestReviewExample = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.False(t, approveReviewExample.Dismissed) + + assert.NoError(t, DismissReview(rejectReviewExample, false)) + assert.False(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.False(t, approveReviewExample.Dismissed) + + assert.NoError(t, DismissReview(approveReviewExample, true)) + assert.False(t, rejectReviewExample.Dismissed) + assert.False(t, requestReviewExample.Dismissed) + assert.True(t, approveReviewExample.Dismissed) + }