simplify logic

models/review.go

@@ -559,26 +559,13 @@ func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) {
 	return
 }
 
-// MarkReviewAsDismissed marks existing reviews as stale
-func MarkReviewAsDismissed(review *Review) (err error) {
-	if review.Dismissed || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
+// 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 = true
-
-	_, err = x.Cols("dismissed").Update(review)
-
-	return
-}
-
-// MarkReviewAsUnDismissed marks dismissed reviews as not dismissed
-func MarkReviewAsUnDismissed(review *Review) (err error) {
-	if !review.Dismissed || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
-		return nil
-	}
-
-	review.Dismissed = false
+	review.Dismissed = isDismiss
 
 	_, err = x.Cols("dismissed").Update(review)
 

models/review_test.go

@@ -143,10 +143,12 @@ func TestGetReviewersByIssueID(t *testing.T) {
 	}
 }
 
-func TestMarkReviewAsDismissed(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, MarkReviewAsDismissed(review1))
-	assert.NoError(t, MarkReviewAsDismissed(review2))
-	assert.NoError(t, MarkReviewAsUnDismissed(review2))
+	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))
 }

routers/api/v1/repo/pull_review.go

@@ -794,43 +794,7 @@ func DismissPullReview(ctx *context.APIContext, opts api.DismissPullReviewOption
 	//     "$ref": "#/responses/forbidden"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
-	if !ctx.Repo.IsAdmin() {
-		ctx.Error(http.StatusForbidden, "", "Must be repo admin")
-		return
-	}
-	review, pr, isWrong := prepareSingleReview(ctx)
-	if isWrong {
-		return
-	}
-
-	if review.Type != models.ReviewTypeApprove && review.Type != models.ReviewTypeReject {
-		ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because it's type is not Approve or change request")
-		return
-	}
-
-	if pr.Issue.IsClosed {
-		ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed")
-		return
-	}
-
-	_, err := pull_service.DismissReview(review.ID, opts.Message, ctx.User)
-	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
-		return
-	}
-
-	if review, err = models.GetReviewByID(review.ID); err != nil {
-		ctx.Error(http.StatusInternalServerError, "GetReviewByID", err)
-		return
-	}
-
-	// convert response
-	apiReview, err := convert.ToPullReview(review, ctx.User)
-	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "convertToPullReview", err)
-		return
-	}
-	ctx.JSON(http.StatusOK, apiReview)
+	disMissReview(ctx, true)
 }
 
 // UnDismissPullReview cancel to dismiss a review for a pull request
@@ -870,6 +834,10 @@ func UnDismissPullReview(ctx *context.APIContext) {
 	//     "$ref": "#/responses/forbidden"
 	//   "422":
 	//     "$ref": "#/responses/validationError"
+	disMissReview(ctx, false)
+}
+
+func disMissReview(ctx *context.APIContext, isDismiss bool) {
 	if !ctx.Repo.IsAdmin() {
 		ctx.Error(http.StatusForbidden, "", "Must be repo admin")
 		return
@@ -889,12 +857,12 @@ func UnDismissPullReview(ctx *context.APIContext) {
 		return
 	}
 
-	if err := pull_service.UnDismissReview(review.ID); err != nil {
+	_, err := pull_service.DismissReview(review.ID, opts.Message, ctx.User, isDismiss)
+	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
 		return
 	}
 
-	var err error
 	if review, err = models.GetReviewByID(review.ID); err != nil {
 		ctx.Error(http.StatusInternalServerError, "GetReviewByID", err)
 		return

routers/repo/pull_review.go

@@ -161,7 +161,7 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) {
 
 // DismissReview dismissing stale review by repo admin
 func DismissReview(ctx *context.Context, form auth.DismissReviewForm) {
-	comm, err := pull_service.DismissReview(form.ReviewID, form.Message, ctx.User)
+	comm, err := pull_service.DismissReview(form.ReviewID, form.Message, ctx.User, true)
 	if err != nil {
 		ctx.ServerError("pull_service.DismissReview", err)
 		return

services/pull/review.go

@@ -232,7 +232,7 @@ func SubmitReview(doer *models.User, gitRepo *git.Repository, issue *models.Issu
 }
 
 // DismissReview dismissing stale review by repo admin
-func DismissReview(reviewID int64, message string, doer *models.User) (comment *models.Comment, err error) {
+func DismissReview(reviewID int64, message string, doer *models.User, isDismiss bool) (comment *models.Comment, err error) {
 	review, err := models.GetReviewByID(reviewID)
 	if err != nil {
 		return
@@ -242,10 +242,14 @@ func DismissReview(reviewID int64, message string, doer *models.User) (comment *
 		return nil, fmt.Errorf("Wrong using")
 	}
 
-	if err = models.MarkReviewAsDismissed(review); err != nil {
+	if err = models.DismissReview(review, isDismiss); err != nil {
 		return
 	}
 
+	if !isDismiss {
+		return nil, nil
+	}
+
 	// load data for notify
 	if err = review.LoadAttributes(); err != nil {
 		return
@@ -277,18 +281,3 @@ func DismissReview(reviewID int64, message string, doer *models.User) (comment *
 
 	return
 }
-
-// UnDismissReview cancel dismissed stale review by repo admin
-func UnDismissReview(reviewID int64) (err error) {
-	review, err := models.GetReviewByID(reviewID)
-	if err != nil {
-		return
-	}
-
-	if review.Type != models.ReviewTypeApprove && review.Type != models.ReviewTypeReject {
-		return fmt.Errorf("Wrong using")
-	}
-
-	err = models.MarkReviewAsUnDismissed(review)
-	return
-}