Skip to content

Commit 26a5999

Browse files
committed
simplify logic
1 parent a060513 commit 26a5999

File tree

5 files changed

+24
-78
lines changed

5 files changed

+24
-78
lines changed

models/review.go

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -559,26 +559,13 @@ func MarkReviewsAsNotStale(issueID int64, commitID string) (err error) {
559559
return
560560
}
561561

562-
// MarkReviewAsDismissed marks existing reviews as stale
563-
func MarkReviewAsDismissed(review *Review) (err error) {
564-
if review.Dismissed || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
562+
// DismissReview change the dismiss status of a review
563+
func DismissReview(review *Review, isDismiss bool) (err error) {
564+
if review.Dismissed == isDismiss || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
565565
return nil
566566
}
567567

568-
review.Dismissed = true
569-
570-
_, err = x.Cols("dismissed").Update(review)
571-
572-
return
573-
}
574-
575-
// MarkReviewAsUnDismissed marks dismissed reviews as not dismissed
576-
func MarkReviewAsUnDismissed(review *Review) (err error) {
577-
if !review.Dismissed || (review.Type != ReviewTypeApprove && review.Type != ReviewTypeReject) {
578-
return nil
579-
}
580-
581-
review.Dismissed = false
568+
review.Dismissed = isDismiss
582569

583570
_, err = x.Cols("dismissed").Update(review)
584571

models/review_test.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,12 @@ func TestGetReviewersByIssueID(t *testing.T) {
143143
}
144144
}
145145

146-
func TestMarkReviewAsDismissed(t *testing.T) {
146+
func TestDismissReview(t *testing.T) {
147147
review1 := AssertExistsAndLoadBean(t, &Review{ID: 9}).(*Review)
148148
review2 := AssertExistsAndLoadBean(t, &Review{ID: 11}).(*Review)
149-
assert.NoError(t, MarkReviewAsDismissed(review1))
150-
assert.NoError(t, MarkReviewAsDismissed(review2))
151-
assert.NoError(t, MarkReviewAsUnDismissed(review2))
149+
assert.NoError(t, DismissReview(review1, true))
150+
assert.NoError(t, DismissReview(review2, true))
151+
assert.NoError(t, DismissReview(review2, true))
152+
assert.NoError(t, DismissReview(review2, false))
153+
assert.NoError(t, DismissReview(review2, false))
152154
}

routers/api/v1/repo/pull_review.go

Lines changed: 7 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -794,43 +794,7 @@ func DismissPullReview(ctx *context.APIContext, opts api.DismissPullReviewOption
794794
// "$ref": "#/responses/forbidden"
795795
// "422":
796796
// "$ref": "#/responses/validationError"
797-
if !ctx.Repo.IsAdmin() {
798-
ctx.Error(http.StatusForbidden, "", "Must be repo admin")
799-
return
800-
}
801-
review, pr, isWrong := prepareSingleReview(ctx)
802-
if isWrong {
803-
return
804-
}
805-
806-
if review.Type != models.ReviewTypeApprove && review.Type != models.ReviewTypeReject {
807-
ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because it's type is not Approve or change request")
808-
return
809-
}
810-
811-
if pr.Issue.IsClosed {
812-
ctx.Error(http.StatusForbidden, "", "not need to dismiss this review because this pr is closed")
813-
return
814-
}
815-
816-
_, err := pull_service.DismissReview(review.ID, opts.Message, ctx.User)
817-
if err != nil {
818-
ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
819-
return
820-
}
821-
822-
if review, err = models.GetReviewByID(review.ID); err != nil {
823-
ctx.Error(http.StatusInternalServerError, "GetReviewByID", err)
824-
return
825-
}
826-
827-
// convert response
828-
apiReview, err := convert.ToPullReview(review, ctx.User)
829-
if err != nil {
830-
ctx.Error(http.StatusInternalServerError, "convertToPullReview", err)
831-
return
832-
}
833-
ctx.JSON(http.StatusOK, apiReview)
797+
disMissReview(ctx, true)
834798
}
835799

836800
// UnDismissPullReview cancel to dismiss a review for a pull request
@@ -870,6 +834,10 @@ func UnDismissPullReview(ctx *context.APIContext) {
870834
// "$ref": "#/responses/forbidden"
871835
// "422":
872836
// "$ref": "#/responses/validationError"
837+
disMissReview(ctx, false)
838+
}
839+
840+
func disMissReview(ctx *context.APIContext, isDismiss bool) {
873841
if !ctx.Repo.IsAdmin() {
874842
ctx.Error(http.StatusForbidden, "", "Must be repo admin")
875843
return
@@ -889,12 +857,12 @@ func UnDismissPullReview(ctx *context.APIContext) {
889857
return
890858
}
891859

892-
if err := pull_service.UnDismissReview(review.ID); err != nil {
860+
_, err := pull_service.DismissReview(review.ID, opts.Message, ctx.User, isDismiss)
861+
if err != nil {
893862
ctx.Error(http.StatusInternalServerError, "pull_service.DismissReview", err)
894863
return
895864
}
896865

897-
var err error
898866
if review, err = models.GetReviewByID(review.ID); err != nil {
899867
ctx.Error(http.StatusInternalServerError, "GetReviewByID", err)
900868
return

routers/repo/pull_review.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) {
161161

162162
// DismissReview dismissing stale review by repo admin
163163
func DismissReview(ctx *context.Context, form auth.DismissReviewForm) {
164-
comm, err := pull_service.DismissReview(form.ReviewID, form.Message, ctx.User)
164+
comm, err := pull_service.DismissReview(form.ReviewID, form.Message, ctx.User, true)
165165
if err != nil {
166166
ctx.ServerError("pull_service.DismissReview", err)
167167
return

services/pull/review.go

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ func SubmitReview(doer *models.User, gitRepo *git.Repository, issue *models.Issu
232232
}
233233

234234
// DismissReview dismissing stale review by repo admin
235-
func DismissReview(reviewID int64, message string, doer *models.User) (comment *models.Comment, err error) {
235+
func DismissReview(reviewID int64, message string, doer *models.User, isDismiss bool) (comment *models.Comment, err error) {
236236
review, err := models.GetReviewByID(reviewID)
237237
if err != nil {
238238
return
@@ -242,10 +242,14 @@ func DismissReview(reviewID int64, message string, doer *models.User) (comment *
242242
return nil, fmt.Errorf("Wrong using")
243243
}
244244

245-
if err = models.MarkReviewAsDismissed(review); err != nil {
245+
if err = models.DismissReview(review, isDismiss); err != nil {
246246
return
247247
}
248248

249+
if !isDismiss {
250+
return nil, nil
251+
}
252+
249253
// load data for notify
250254
if err = review.LoadAttributes(); err != nil {
251255
return
@@ -277,18 +281,3 @@ func DismissReview(reviewID int64, message string, doer *models.User) (comment *
277281

278282
return
279283
}
280-
281-
// UnDismissReview cancel dismissed stale review by repo admin
282-
func UnDismissReview(reviewID int64) (err error) {
283-
review, err := models.GetReviewByID(reviewID)
284-
if err != nil {
285-
return
286-
}
287-
288-
if review.Type != models.ReviewTypeApprove && review.Type != models.ReviewTypeReject {
289-
return fmt.Errorf("Wrong using")
290-
}
291-
292-
err = models.MarkReviewAsUnDismissed(review)
293-
return
294-
}

0 commit comments

Comments
 (0)