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>

2 files changed, 58 insertions, 8 deletions

diff --git a/models/review.go b/models/review.go
index 702e9634ef..343621c0fa 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 4f049b45e3..accc184193 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)
+
 }