-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(api): add resolve/unresolve review comment endpoints #36441
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
base: main
Are you sure you want to change the base?
Conversation
58a4c69 to
a928f12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds API endpoints to resolve and unresolve pull request review comments, providing programmatic access to functionality that previously existed only in the UI.
Changes:
- Added POST endpoints
/repos/{owner}/{repo}/pulls/{index}/comments/{id}/resolveand/repos/{owner}/{repo}/pulls/{index}/comments/{id}/unresolve - Refactored
ToPullReviewCommentListto extract a newToPullReviewCommentfunction for converting single comments - Updated Swagger documentation to reflect the new API endpoints
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| routers/api/v1/api.go | Registers the new resolve/unresolve routes with appropriate middleware for authentication and archive checks |
| routers/api/v1/repo/pull_review.go | Implements the handler functions with permission checks, validation, and response formatting |
| services/convert/pull_review.go | Refactors comment conversion logic to support both list and single comment conversion |
| templates/swagger/v1_json.tmpl | Adds OpenAPI/Swagger documentation for the new endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
It's better to add some tests |
4f90506 to
5bff286
Compare
|
@lunny added some tests 😄 |
routers/api/v1/api.go
Outdated
| m.Post("/comments/{id}/resolve", reqToken(), mustNotBeArchived, repo.ResolvePullReviewComment) | ||
| m.Post("/comments/{id}/unresolve", reqToken(), mustNotBeArchived, repo.UnresolvePullReviewComment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need issue index, see the /issues/comments/{id}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it better go under /pulls/reviews as only review comments can be marked resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it could be not very useful to provide review id in url but there is already /pulls/review/{id}/comments endpoint that can be base for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue endpoints have already deprecated the unnecessary index
// EditIssueCommentDeprecated modify a comment of an issue
func EditIssueCommentDeprecated(ctx *context.APIContext) {
// swagger:operation PATCH /repos/{owner}/{repo}/issues/{index}/comments/{id} issue issueEditCommentDeprecated
56c2c02 to
d10ac57
Compare
|
By the way, no need to rebase: https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#maintaining-open-prs |
|
whoops sorry 👍 |
lafriks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking currently so we can discuss the best endpoint name
If you'd like to edit this PR to update the endpoints, feel free to do so. But I think the current design is good enough, I don't think adding issue index makes sense, I don't think "review id" is related or necessary. Update: I have objection for adding any other unnecessary If you don't want to edit this PR, I think we can go with this. |
b80f4ff to
fa2da7b
Compare
Summary
I added API endpoints to resolve and unresolve pull request review comments.
This mirrors the UI conversation resolve behavior and returns the updated PullReviewComment with resolver information.
Usage
New endpoints:
/repos/{owner}/{repo}/pulls/{index}/comments/{id}/resolve/repos/{owner}/{repo}/pulls/{index}/comments/{id}/unresolveExample: