From 833bcf9013a0e21bfc322e38e3fbce080a62a6c5 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 14 Apr 2021 18:01:23 +0100 Subject: [PATCH 1/2] Ensure review dismissal only dismisses the correct review Fix #15472 Signed-off-by: Andrew Thornton --- 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..69fd84f3363da 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()) + + review_reject := AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review_request := AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + review_approve := AssertExistsAndLoadBean(t, &Review{ID: 8}).(*Review) + assert.False(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.False(t, review_approve.Dismissed) + + assert.NoError(t, DismissReview(review_reject, true)) + review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + + assert.NoError(t, DismissReview(review_request, true)) + review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.False(t, review_approve.Dismissed) + + assert.NoError(t, DismissReview(review_request, true)) + review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.False(t, review_approve.Dismissed) + + assert.NoError(t, DismissReview(review_request, false)) + review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.False(t, review_approve.Dismissed) + + assert.NoError(t, DismissReview(review_request, false)) + review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) + review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) + assert.True(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.False(t, review_approve.Dismissed) + + assert.NoError(t, DismissReview(review_reject, false)) + assert.False(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.False(t, review_approve.Dismissed) + + assert.NoError(t, DismissReview(review_approve, true)) + assert.False(t, review_reject.Dismissed) + assert.False(t, review_request.Dismissed) + assert.True(t, review_approve.Dismissed) + } From 2f38d0fd004ed4aee420698b80ed6e96a09fc799 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 14 Apr 2021 18:26:42 +0100 Subject: [PATCH 2/2] placate lint Signed-off-by: Andrew Thornton --- models/review_test.go | 100 +++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/models/review_test.go b/models/review_test.go index 69fd84f3363da..accc1841933ec 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -145,55 +145,55 @@ func TestGetReviewersByIssueID(t *testing.T) { func TestDismissReview(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - review_reject := AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) - review_request := AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) - review_approve := AssertExistsAndLoadBean(t, &Review{ID: 8}).(*Review) - assert.False(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.False(t, review_approve.Dismissed) - - assert.NoError(t, DismissReview(review_reject, true)) - review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) - review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) - assert.True(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - - assert.NoError(t, DismissReview(review_request, true)) - review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) - review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) - assert.True(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.False(t, review_approve.Dismissed) - - assert.NoError(t, DismissReview(review_request, true)) - review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) - review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) - assert.True(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.False(t, review_approve.Dismissed) - - assert.NoError(t, DismissReview(review_request, false)) - review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) - review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) - assert.True(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.False(t, review_approve.Dismissed) - - assert.NoError(t, DismissReview(review_request, false)) - review_reject = AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review) - review_request = AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review) - assert.True(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.False(t, review_approve.Dismissed) - - assert.NoError(t, DismissReview(review_reject, false)) - assert.False(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.False(t, review_approve.Dismissed) - - assert.NoError(t, DismissReview(review_approve, true)) - assert.False(t, review_reject.Dismissed) - assert.False(t, review_request.Dismissed) - assert.True(t, review_approve.Dismissed) + 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) }