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.

Sign up for GitHub

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

Jump to bottom

Prevent allow/reject reviews on merged/closed PRs #30686

Merged
merged 9 commits into from Apr 27, 2024
Merged

Prevent allow/reject reviews on merged/closed PRs #30686

merged 9 commits into from Apr 27, 2024

Conversation

kemzeb
Copy link
Contributor

@kemzeb kemzeb commented Apr 24, 2024

Resolves #30675.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 24, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 24, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Apr 24, 2024
@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 24, 2024

Manually tested both approve and reject reviews on open, merged, and closed PRs and they appear to produce valid results.

Currently making this a draft PR so that I can:

  • Find good places to write tests
  • Catch the error object I created in the API (if we have associated API)

@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Apr 24, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 24, 2024
@silverwind
Copy link
Member

Already looking quite good :)

@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 25, 2024

Already looking quite good :)

Thanks for the review @silverwind! Just having trouble setting up a proper test for these changes since I need to use a git.Repository object in SubmitReview() where the code expects an actual git repository to exist. I'll look to see if I can write an integration test for both closed and merged PRs 👍

@lunny
Copy link
Member

lunny commented Apr 25, 2024

But they should still be allowed to add comment without approve/reject. So I think the template should not be changed too much. Just disable that two buttons.

@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 25, 2024 •
edited

But they should still be allowed to add comment without approve/reject. So I think the template should not be changed too much. Just disable that two buttons.

Thanks for the review! The user should be able to create a review comment with my changes. Here is an example with a closed PR:
Screenshot 2024-04-24 at 9 48 47 PM

This appears to be in-line with GitHub (though you only see a "Submit Review" button rather than a "Comment" button). If you wish, I can go ahead and change it to just conditionally add the disabled attribute to the buttons.

@lunny
Copy link
Member

lunny commented Apr 25, 2024

But they should still be allowed to add comment without approve/reject. So I think the template should not be changed too much. Just disable that two buttons.

Thanks for the review! The user should be able to create a review comment with my changes. Here is an example with a closed PR: Screenshot 2024-04-24 at 9 48 47 PM

This appears to be in-line with GitHub (though you only see a "Submit Review" button rather than a "Comment" button). If you wish, I can go ahead and change it to just conditionally add the disabled attribute to the buttons.

I think the current design is OK for me.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 26, 2024
@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 26, 2024

Recent change adds integration tests and has the web handler just return the HTTP status rather than changing the response body.

Please take a careful eye on the test code as I could be misusing the testing API or there could be an opportunity to improve the test performance that I haven't seen.

@kemzeb kemzeb marked this pull request as ready for review April 26, 2024 21:18
@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 26, 2024

Hmm, going to take a look at the second test again as I'm not sure if its valid

@kemzeb kemzeb marked this pull request as draft April 26, 2024 21:27
@kemzeb kemzeb marked this pull request as ready for review April 26, 2024 21:40
wxiaoguang
wxiaoguang reviewed Apr 27, 2024
View reviewed changes
services/pull/review.go Outdated Show resolved Hide resolved
wxiaoguang
wxiaoguang reviewed Apr 27, 2024
View reviewed changes
services/pull/review.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 27, 2024
@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 27, 2024

Yikes not sure why I decided to move the IsClosed check to the top as this has users unable to comment on the close/merged PR. Will make the necessary change now

@kemzeb
Copy link
Contributor Author

kemzeb commented Apr 27, 2024

Just to make sure, I manually tested both the API and web route with closed/merged PRs and everything appears fine from what I can see.

@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 27, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 27, 2024 11:24
@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Apr 27, 2024
@wxiaoguang wxiaoguang merged commit dd301ca into go-gitea:main Apr 27, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Apr 27, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 27, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 27, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
289aed7 
* giteaofficial/main:
  Make Ctrl+Enter work for issue/comment edit (go-gitea#30720)
  Rename migration package name for 1.22-rc1 (go-gitea#30730)
  Issue card improvements (go-gitea#30687)
  Don't show loading indicators when refreshing the system status (go-gitea#30712)
  Add some tests to clarify the "must-change-password" behavior (go-gitea#30693)
  Prevent allow/reject reviews on merged/closed PRs (go-gitea#30686)
  Update JS dependencies (go-gitea#30713)
  Improve diff stats bar (go-gitea#30669)
  Remove unused parameter for some functions in `services/mirror` (go-gitea#30724)
  Update misspell to 0.5.1 and add `misspellings.csv` (go-gitea#30573)
  Suppress browserslist warning in webpack target (go-gitea#30571)
  [skip ci] Updated translations via Crowdin
  Diff color enhancements, add line number background (go-gitea#30670)
@wxiaoguang wxiaoguang modified the milestones: 1.23.0, 1.22.0 Apr 27, 2024
@kemzeb kemzeb deleted the disallow-reviews-of-closed-prs branch April 27, 2024 16:09
silverwind added a commit to silverwind/gitea that referenced this pull request Apr 27, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into noext
d98bdfc 
* origin/main:
  Replace deprecated `math/rand` functions (go-gitea#30733)
  Make Ctrl+Enter work for issue/comment edit (go-gitea#30720)
  Rename migration package name for 1.22-rc1 (go-gitea#30730)
  Issue card improvements (go-gitea#30687)
  Don't show loading indicators when refreshing the system status (go-gitea#30712)
  Add some tests to clarify the "must-change-password" behavior (go-gitea#30693)
  Prevent allow/reject reviews on merged/closed PRs (go-gitea#30686)
  Update JS dependencies (go-gitea#30713)
  Improve diff stats bar (go-gitea#30669)
  Remove unused parameter for some functions in `services/mirror` (go-gitea#30724)
  Update misspell to 0.5.1 and add `misspellings.csv` (go-gitea#30573)
  Suppress browserslist warning in webpack target (go-gitea#30571)
  [skip ci] Updated translations via Crowdin
  Diff color enhancements, add line number background (go-gitea#30670)
  feat(api): enhance Actions Secrets Management API for repository (go-gitea#30656)
  Fix code search input for different views (go-gitea#30678)
  Fix incorrect object id hash function (go-gitea#30708)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

Merged/Closed PR should not be approvable or request changes in diff page?
5 participants
@kemzeb @silverwind @lunny @wxiaoguang @GiteaBot