From 8951891ded5ec565c4df86e9dd09dd6a2608d04a Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Thu, 16 Aug 2018 13:43:17 +0100 Subject: [PATCH 1/7] Make sure author cannot reject/approve their own PR --- options/locale/locale_en-US.ini | 2 ++ routers/repo/pull_review.go | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 7779f1e96b0d0..cc74cad6f1470 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -814,6 +814,8 @@ issues.dependency.add_error_dep_not_exist = Dependency does not exist. issues.dependency.add_error_dep_exists = Dependency already exists. issues.dependency.add_error_cannot_create_circular = You cannot create a dependency with two issues blocking each other. issues.dependency.add_error_dep_not_same_repo = Both issues must be in the same repository. +issues.review.self.approval = You cannot approve your own pull request. +issues.review.self.rejection = You cannot request changes on your own pull request. issues.review.approve = "approved these changes %s" issues.review.comment = "reviewed %s" issues.review.content.empty = You need to leave a comment indicating the requested change(s). diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index 7ca02ac809098..afcd9c376def3 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -108,6 +108,30 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { return } + switch reviewType { + case models.ReviewTypeUnknown: + ctx.ServerError("GetCurrentReview", fmt.Errorf("unknown ReviewType: %s", form.Type)) + return + + // can not approve/reject your own PR + case models.ReviewTypeApprove, models.ReviewTypeReject: + + if issue.Poster.ID == ctx.User.ID { + + var translated string + + if reviewType == models.ReviewTypeApprove { + translated = ctx.Tr("repo.issues.review.self.approval") + } else { + translated = ctx.Tr("repo.issues.review.self.rejection") + } + + ctx.Flash.Error(translated) + ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) + return + } + } + if form.HasEmptyContent() { ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty")) ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) From 4bb258c3b299651e11ae1ee165350ccefcc8f6a5 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Thu, 16 Aug 2018 14:03:41 +0100 Subject: [PATCH 2/7] Disable buttons in templates too --- routers/repo/pull.go | 1 + templates/repo/diff/new_review.tmpl | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 57fe33f85547b..e3f9c6903e0e3 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -484,6 +484,7 @@ func ViewPullFiles(ctx *context.Context) { return } + ctx.Data["IsOwner"] = ctx.Repo.IsWriter() || (ctx.IsSigned && issue.IsPoster(ctx.User.ID)) ctx.Data["IsImageFile"] = commit.IsImageFile ctx.Data["SourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", endCommitID) ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", startCommitID) diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index 2b49ac7296be0..ea5b5d55011ac 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -16,11 +16,11 @@ placeholder="{{$.i18n.Tr "repo.diff.review.placeholder"}}">
- - + From 27e5587966ffc562f25bfe0e4ddab8cb70339c33 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Thu, 16 Aug 2018 15:17:08 +0100 Subject: [PATCH 3/7] Remove unneccessary if check since the switch below catches it --- routers/repo/pull_review.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index afcd9c376def3..c39d2f1bd4f67 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -103,10 +103,6 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { var err error reviewType := form.ReviewType() - if reviewType == models.ReviewTypeUnknown { - ctx.ServerError("GetCurrentReview", fmt.Errorf("unknown ReviewType: %s", form.Type)) - return - } switch reviewType { case models.ReviewTypeUnknown: From 55edbc33e317f727be0ed0d5e6ae3217459185e1 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Thu, 16 Aug 2018 21:25:00 +0100 Subject: [PATCH 4/7] Fix IsOwner check --- routers/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index e3f9c6903e0e3..3e7645401c238 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -484,7 +484,7 @@ func ViewPullFiles(ctx *context.Context) { return } - ctx.Data["IsOwner"] = ctx.Repo.IsWriter() || (ctx.IsSigned && issue.IsPoster(ctx.User.ID)) + ctx.Data["IsOwner"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID) ctx.Data["IsImageFile"] = commit.IsImageFile ctx.Data["SourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", endCommitID) ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", startCommitID) From 9f86967d921f62f89192962c442af4a44a5e5850 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Fri, 17 Aug 2018 00:03:07 +0100 Subject: [PATCH 5/7] Update template and remove new template variable --- routers/repo/pull.go | 1 - templates/repo/diff/new_review.tmpl | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 3e7645401c238..57fe33f85547b 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -484,7 +484,6 @@ func ViewPullFiles(ctx *context.Context) { return } - ctx.Data["IsOwner"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID) ctx.Data["IsImageFile"] = commit.IsImageFile ctx.Data["SourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", endCommitID) ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(headTarget, "src", "commit", startCommitID) diff --git a/templates/repo/diff/new_review.tmpl b/templates/repo/diff/new_review.tmpl index ea5b5d55011ac..68d8f893f2a34 100644 --- a/templates/repo/diff/new_review.tmpl +++ b/templates/repo/diff/new_review.tmpl @@ -16,11 +16,11 @@ placeholder="{{$.i18n.Tr "repo.diff.review.placeholder"}}">
- - From 2a4a5fcda95c02185138f50b1c21f69ef9d553dc Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Sun, 19 Aug 2018 15:01:34 +0100 Subject: [PATCH 6/7] Add alert template and redirect to diff page on review failure --- routers/repo/pull_review.go | 2 +- templates/repo/pulls/files.tmpl | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index c39d2f1bd4f67..26a7903a84429 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -123,7 +123,7 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { } ctx.Flash.Error(translated) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) + ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) return } } diff --git a/templates/repo/pulls/files.tmpl b/templates/repo/pulls/files.tmpl index 7663788c688ef..fb46919f88e83 100644 --- a/templates/repo/pulls/files.tmpl +++ b/templates/repo/pulls/files.tmpl @@ -11,6 +11,7 @@
{{template "repo/issue/view_title" .}} {{template "repo/pulls/tab_menu" .}} + {{template "base/alert" .}}
{{template "repo/diff/box" .}}
From 654daad82fd94ffd88c32b80252bbbac559a5537 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Sun, 19 Aug 2018 15:06:07 +0100 Subject: [PATCH 7/7] Redirect to files diff as a little update to #4632 --- routers/repo/pull_review.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/repo/pull_review.go b/routers/repo/pull_review.go index 26a7903a84429..1269a7a7e7a4b 100644 --- a/routers/repo/pull_review.go +++ b/routers/repo/pull_review.go @@ -130,7 +130,7 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) { if form.HasEmptyContent() { ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty")) - ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) + ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) return }