Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User shouldn't be able to approve or reject his/her own PR #4729

Merged
Merged
2 changes: 2 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
1 change: 1 addition & 0 deletions routers/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ func ViewPullFiles(ctx *context.Context) {
return
}

ctx.Data["IsOwner"] = ctx.Repo.IsWriter() || (ctx.IsSigned && issue.IsPoster(ctx.User.ID))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be 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)
Expand Down
24 changes: 24 additions & 0 deletions routers/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please redirect to %s/pulls/%d/files instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base/alert template isn't imported in the template for the page %s/pulls/%d/files which is why I redirected back to the main page... Should I just add the alert template in there or just leave it as it is ? ( #4632 redirects to the main PR page though )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add alert to to files template

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed @JonasFranzDEV and @lafriks

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))
Expand Down
6 changes: 3 additions & 3 deletions templates/repo/diff/new_review.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
placeholder="{{$.i18n.Tr "repo.diff.review.placeholder"}}"></textarea>
</div>
<div class="ui divider"></div>
<button type="submit" name="type" value="approve"
<button type="submit" name="type" value="approve" {{ if .IsOwner }} disabled {{ end }}
class="ui submit green tiny button btn-submit">{{$.i18n.Tr "repo.diff.review.approve"}}</button>
<button type="submit" name="type" value="comment"
class="ui submit tiny basic button btn-submit">{{$.i18n.Tr "repo.diff.review.comment"}}</button>
<button type="submit" name="type" value="reject"
class="ui submit tiny basic button btn-submit">{{$.i18n.Tr "repo.diff.review.comment"}}</button>
<button type="submit" name="type" value="reject" {{ if .IsOwner }} disabled {{ end }}
class="ui submit red tiny button btn-submit">{{$.i18n.Tr "repo.diff.review.reject"}}</button>
</form>
</div>
Expand Down