Prevent re-review and dismiss review actions on closed and merged PRs #30065

kemzeb · 2024-03-25T07:48:40Z

Resolves #29965.

Manually tested this by:

Following the installation guide (but built a local Docker image instead)
Creating 2 users, one who is the Owner of a newly-created repository and the other a Collaborator
Had the Collaborator create a PR that the Owner reviews
Collaborator resolves conversation and Owner merges PR

And with this change we see that we can no longer see re-request review button for the Owner:

lunny · 2024-03-25T07:50:30Z

Maybe closed pull requests cannot be reviewed anymore.

kemzeb · 2024-03-25T08:19:31Z

Maybe closed pull requests cannot be reviewed anymore.

Thanks for the quick response! I actually never seen such an edge case in the wild before.

I also just realized that in the issue it explains they can still re-request or delete a review request when the PR has already been merged. So if I understand this correctly, this looks like more than just hiding a button when a PR is closed/merged; rather, some backend logic needs to be changed so that users cannot re-request a review on a merged PR. Still kind of fuzzy how closed PRs should be handled, but I'll make this a draft PR and see if I can resolve this.

kemzeb · 2024-03-25T11:00:48Z

I believe I found the culprit route handler, that being UpdatePullReviewRequest(). I manually tested the code path by doing what I did in my original description (this time running Gitea in a dev environment rather than having to fire up the containerized app). I got the following logs when trying to re-request a review of a merged PR:

2024/03/25 10:43:00 ...rs/web/repo/issue.go:2417:UpdatePullReviewRequest() [W] UpdatePullReviewRequest: cannot add or remove a review for merged PR gek/test-repo#3
2024/03/25 10:43:00 ...eb/routing/logger.go:102:func1() [I] router: completed POST /gek/test-repo/issues/request_review for 127.0.0.1:56852, 403 Forbidden in 32.2ms @ repo/issue.go:2387(repo.UpdatePullReviewRequest)

I'll make this PR ready-to-review. I do have some questions/comments:

I did not see any tests for UpdatePullReviewRequest(). Should I add one to account for this bug and if so should I add eit to issue_test.go?
Is there an api/v1 route I should look out for too? Maybe /api/v1/repo/pull_review.go?
Closed PRs were discussed here. I may be wrong, but I believe closed PRs can still be reviewed since the HTML template if check I added was just to check if the PR hasn't been merged, not if it was closed (which I think we can do $.Issue.IsClosed for that).

Thanks in advance for your time!

silverwind · 2024-03-25T18:26:23Z

I tested changing reviewers and GitHub does allow adding/removing review requests in both merged and closed states. Only the "re-review" and "dismiss" actions are not possible in those states in GitHub.

Also regarding review comments: On GitHub, it's not possible to positively or negatively review a PR it either closed or merged state. It is however possible to submit a neutral review.

kemzeb · 2024-03-25T19:21:06Z

I tested changing reviewers and GitHub does allow adding/removing review requests in both merged and closed states. Only the "re-review" and "dismiss" actions are not possible in those states in GitHub.

Thanks for the review! Reading this, it looks like simply throwing a http.StatusForbidden when requesting for a review on a merged PR is not good enough. I guess I need to figure out if we were given a re-review or dismiss action for the reviewer. Looking so far, I'm unsure if this is something the frontend communicates but I'll see if I can find a solution 👍

routers/web/repo/issue.go

kemzeb · 2024-03-26T04:06:24Z

Most recent change makes us also consider closed PRs when a user is trying to perform a re-review or dismiss request.

Will look into how unit testing works here and how I can cover these code paths.

models/issues/review.go

kemzeb · 2024-03-27T06:04:09Z

My recent commit does the following:

Rollback failures in review.AddReviewRequest() instead of committing them
Throw the PR merge checking logic under a if issue.IsPull branch
Fix a UI logic bug
Fix error messages to include both closed and merged

Continuing to look into how unit testing works here in the meantime

kemzeb · 2024-03-27T09:39:43Z

Added some unit tests. Let me know if I should add more or make improvements.

Next I'll scour the API routes looking for any signs of issues similar to this one.

kemzeb · 2024-03-27T09:58:25Z

(Sorry for the repeated force-pushes, had some linting problems)

kemzeb · 2024-03-27T19:00:49Z

Made the necessary changes to the API and from testing using the swagger API client it looks like it works as expected. I haven't seen any tests related to the API code, but let me know if I should add some for my changes.

* upstream/main: Remove fomantic checkbox module (go-gitea#30162) Refactor topic Find functions and add more tests for pagination (go-gitea#30127) replace jquery-minicolors with coloris (go-gitea#30055) Add API for `Variables` (go-gitea#29520) Fix `DEFAULT_SHOW_FULL_NAME=false` has no effect in commit list and commit graph page (go-gitea#30096) Fix migration v292 (go-gitea#30153) Adjust VS Code debug filename match in .gitignore (go-gitea#30158) Prevent re-review and dismiss review actions on closed and merged PRs (go-gitea#30065) Render code tags in commit messages (go-gitea#30146) Bump `@github/relative-time-element` to v4.4.0 (go-gitea#30154) Migrate font-family to tailwind (go-gitea#30118) Move from `max( id )` to `max( index )` for latest commit statuses (go-gitea#30076) Remember login for a month by default (go-gitea#30150)

This release stands as a monumental milestone in our development journey with a record-breaking incorporation of [1528](https://github.com/go-gitea/gitea/pulls?q=is%3Apr+milestone%3A1.22.0+is%3Amerged) pull requests. It marks the most extensive update in Gitea's history, showcasing a plethora of new features and infrastructure improvements. Noteworthy advancements in this release include the introduction of `HTMX` and `Tailwind`, signaling a strategic shift as we gradually phase out `jquery` and `Fomantic UI`. These changes reflect our commitment to embracing modern technologies and enhancing the user experience. Key highlights of this release encompass significant changes categorized under `BREAKING`, `FEATURES`, `ENHANCEMENTS`, and `PERFORMANCE`, each contributing to a more robust and efficient Gitea platform. * BREAKING * Improve reverse proxy documents and clarify the AppURL guessing behavior (go-gitea#31003) (go-gitea#31020) * Remember log in for a month by default (go-gitea#30150) * Breaking summary for template refactoring (go-gitea#29395) * All custom templates need to follow these changes * Recommend/convert to use case-sensitive collation for MySQL/MSSQL (go-gitea#28662) * Make offline mode as default to not connect external avatar service by default (go-gitea#28548) * Include public repos in the doer's dashboard for issue search (go-gitea#28304) * Use restricted sanitizer for repository description (go-gitea#28141) * Support storage base path as prefix (go-gitea#27827) * Enhanced auth token / remember me (go-gitea#27606) * Rename the default themes to `gitea-light`, `gitea-dark`, `gitea-auto` (go-gitea#27419) * If you didn't see the new themes, please remove the `[ui].THEMES` config option from `app.ini` * Require MySQL 8.0, PostgreSQL 12, MSSQL 2012 (go-gitea#27337) * FEATURES * Allow everyone to read or write a wiki by a repo unit setting (go-gitea#30495) * Use raw Wiki links for non-renderable Wiki files (go-gitea#30273) * Render embedded code preview by permalink in markdown (go-gitea#30234) (go-gitea#30249) * Support repo code search without setting up an indexer (go-gitea#29998) * Support pasting URLs over markdown text (go-gitea#29566) * Allow to change primary email before account activation (go-gitea#29412) * Customizable "Open with" applications for repository clone (go-gitea#29320) * Allow options to disable user deletion from the interface on app.ini (go-gitea#29275) * Extend issue template YAML engine (go-gitea#29274) * Add support for `linguist-detectable` and `linguist-documentation` (go-gitea#29267) * Implement code frequency graph (go-gitea#29191) * Show commit status for releases (go-gitea#29149) * Add user blocking (go-gitea#29028) * Actions Artifacts v4 backend (go-gitea#28965) * Add merge style `fast-forward-only` (go-gitea#28954) * Retarget depending pulls when the parent branch is deleted (go-gitea#28686) * Add global setting on how timestamps should be rendered (go-gitea#28657) * Implement actions badge SVGs (go-gitea#28102) * Add skip ci functionality (go-gitea#28075) * Show latest commit for file (go-gitea#28067) * Allow to sync tags from the admin dashboard (go-gitea#28045) * Add Profile Readme for Organisations (go-gitea#27955) * Implement contributors graph (go-gitea#27882) * Artifact deletion in actions ui (go-gitea#27172) * Add API routes to get runner registration token (go-gitea#27144) * Add support for forking single branch (go-gitea#25821) * Add support for sha256 repositories (go-gitea#23894) * Add admin API route for managing user's badges (go-gitea#23106) * ENHANCEMENTS * Make gitea webhooks openproject compatible (go-gitea#28435) (go-gitea#31081) * Support using label names when changing issue labels (go-gitea#30943) (go-gitea#30958) * Fix various problems around project board view (go-gitea#30696) (go-gitea#30902) * Improve context popup rendering (go-gitea#30824) (go-gitea#30829) * Allow to save empty comment (go-gitea#30706) * Prevent allow/reject reviews on merged/closed PRs (go-gitea#30686) * Initial support for colorblindness-friendly themes (go-gitea#30625) * Some NuGet package enhancements (go-gitea#30280) (go-gitea#30324) * Markup color and font size fixes (go-gitea#30282) (go-gitea#30310) * Show 12 lines in markup code preview (go-gitea#30255) (go-gitea#30257) * Add `[other].SHOW_FOOTER_POWERED_BY` setting to hide `Powered by` (go-gitea#30253) * Pulse page improvements (go-gitea#30149) * Render code tags in commit messages (go-gitea#30146) * Prevent re-review and dismiss review actions on closed and merged PRs (go-gitea#30065) * Cancel previous runs of the same PR automatically (go-gitea#29961) * Drag-and-drop improvements for projects and issue pins (go-gitea#29875) * Add default board to new projects, remove uncategorized pseudo-board (go-gitea#29874) * Prevent layout shift in `<overflow-menu>` items (go-gitea#29831) * Add skip ci support for pull request title (go-gitea#29774) * Add more stats tables (go-gitea#29730) * Update API to return 'source_id' for users (go-gitea#29718) * Determine fuzziness of bleve indexer by keyword length (go-gitea#29706) * Expose fuzzy search for issues/pulls (go-gitea#29701) * Put an edit file button on pull request files to allow a quick operation (go-gitea#29697) * Fix action runner offline label padding (go-gitea#29691) * Update allowed attachment types (go-gitea#29688) * Completely style the webkit autofill (go-gitea#29683) * Highlight archived labels (go-gitea#29680) * Add a warning for disallowed email domains (go-gitea#29658) * Set user's 24h preference from their current OS locale (go-gitea#29651) * Add setting to disable user features when user login type is not plain (go-gitea#29615) * Improve natural sort (go-gitea#29611) * Make wiki default branch name changeable (go-gitea#29603) * Unify search boxes (go-gitea#29530) * Add support for API blob upload of release attachments (go-gitea#29507) * Detect broken git hooks (go-gitea#29494) * Sync branches to DB immediately when handling git hook calling (go-gitea#29493) * Allow options to disable user GPG key configuration from the interface on app.ini (go-gitea#29486) * Allow options to disable user SSH key configuration from the interface on app.ini (go-gitea#29447) * Use relative links for commits, mentions, and issues in markdown (go-gitea#29427) * Add `<overflow-menu>`, rename webcomponents (go-gitea#29400) * Include resource state events in Gitlab downloads (go-gitea#29382) * Properly migrate target branch change GitLab comment (go-gitea#29340) * Recolor dark theme to blue shade (go-gitea#29283) * Partially enable MSSQL case-sensitive collation support (go-gitea#29238) * Auto-update the system status in the admin dashboard (go-gitea#29163) * Integrate alpine `noarch` packages into other architectures index (go-gitea#29137) * Document how the TOC election process works (go-gitea#29135) * Tweak repo header (go-gitea#29134) * Make blockquote border size less aggressive (go-gitea#29124) * Downscale pasted PNG images based on metadata (go-gitea#29123) * Show `View at this point in history` for every commit (go-gitea#29122) * Add support for action artifact serve direct (go-gitea#29120) * Change webhook-type in create-view (go-gitea#29114) * Drop "@" from the email sender to avoid spam filters (go-gitea#29109) * Allow non-admin users to delete review requests (go-gitea#29057) * Improve user search display name (go-gitea#29002) * Include username in email headers (go-gitea#28981) * Show whether a PR is WIP inside popups (go-gitea#28975) * Also match weakly validated ETags (go-gitea#28957) * Support nuspec manifest download for Nuget packages (go-gitea#28921) * Fix hardcoded GitHub icon used as migrated release avatar (go-gitea#28910) * Propagate install_if and provider_priority to APKINDEX (go-gitea#28899) * Add artifacts v4 JWT to job message and accept it (go-gitea#28885) * Enable/disable owner and repo projects independently (go-gitea#28805) * Add non-JS fallback for reaction tooltips (go-gitea#28785) * Add the ability to see open and closed issues at the same time (go-gitea#28757) * Move sign-in labels to be above inputs (go-gitea#28753) * Display the latest sync time for pull mirrors on the repo page (go-gitea#28712) * Show in Web UI if the file is vendored and generated (go-gitea#28620) * Add orphaned topic consistency check (go-gitea#28507) * Add branch protection setting for ignoring stale approvals (go-gitea#28498) * Add option to set language in admin user view (go-gitea#28449) * Fix incorrect run order of action jobs (go-gitea#28367) * Add missing exclusive in advanced label options (go-gitea#28322) * Added instance-level variables (go-gitea#28115) * Add edit option for README.md (go-gitea#28071) * Fix link to `Code` tab on wiki commits (go-gitea#28041) * Allow to set explore page default sort (go-gitea#27951) * Improve PR diff view on mobile (go-gitea#27883) * Properly migrate automatic merge GitLab comments (go-gitea#27873) * Display issue task list on project cards (go-gitea#27865) * Add Index to pull_auto_merge.doer_id (go-gitea#27811) * Fix display member unit in the menu bar if there are no hidden members in public org (go-gitea#27795) * List all Debian package versions in `Packages` (go-gitea#27786) * Allow pull requests Manually Merged option to be used by non-admins (go-gitea#27780) * Only show diff file tree when more than one file changed (go-gitea#27775) * Show placeholder email in privacy popup (go-gitea#27770) * Revamp repo header (go-gitea#27760) * Add `must-change-password` command line parameter (go-gitea#27626) * Unify password changing and invalidate auth tokens (go-gitea#27625) * Add border to file tree 'sub-items' and add padding to 'item-file' (go-gitea#27593) * Add slow SQL query warning (go-gitea#27545) * Pre-register OAuth application for tea (go-gitea#27509) * Differentiate between `push` and `pull` `mirror sync in progress` (go-gitea#27390) * Link to file from its history (go-gitea#27354) * Add a shortcut to user's profile page to admin user details (go-gitea#27299) * Doctor: delete action entries without existing user (go-gitea#27292) * Show total TrackedTime on issue/pull/milestone lists (go-gitea#26672) * Don't show the new pull request button when the page is not compare pull (go-gitea#26431) * Add `Hide/Show all checks` button to commit status check (go-gitea#26284) * Improvements of releases list and tags list (go-gitea#25859) * PERFORMANCE * Fix package list performance (go-gitea#30520) (go-gitea#30616) * Add commit status summary table to reduce query from commit status table (go-gitea#30223) * Refactor markup/csv: don't read all to memory (go-gitea#29760) * Lazy load object format with command line and don't do it in OpenRepository (go-gitea#29712) * Add cache for branch divergence on branch list page (go-gitea#29577) * Do some performance optimization for issues list and view issue/pull (go-gitea#29515) * Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444) * Use `crypto/sha256` (go-gitea#29386) * Some performance optimization on the dashboard and issues page (go-gitea#29010) * Add combined index for issue_user.uid and issue_id (go-gitea#28080) # -----BEGIN PGP SIGNATURE----- # # iQIzBAABCAAdFiEEumb2f9c/cFjXEtMIw7fJG2Mvc4oFAmZUPBEACgkQw7fJG2Mv # c4polxAAjJgg1UISxasNGbX/V4G2P9eeXam9lQ5DYAGS6d+RYdTcYdGSbOZujIGG # cVqoZjYCJm7b3KuL1Jjrf7sIAjPB3E9gO8aJ3r+6PGjRpMwCACPUlCo8QWRPDhcU # /eWleWFs59ZMxHnHT3oLH0TGwbdY4tc35/iKElUIc6sX8WBal0SsYScfBJrRTtS/ # DxBaovmkiG0RUWHK3mK0zHrRW9nJecz/4XFWIHgBjKzvCHlzSYrOjUPvytERlWtO # o7i+1Wsret1JLWoW53L3ZQIXCwBBLYjsan9oq7YgSD0Usl9En3o0+S06+TVNiWMp # MNbOQt2SQsIJPPP83pql+rPbKqAp1dibWXLF70mJrBVpdw/b0VHK2K8fIa5Eiilk # EOR9D8uoOgjvxWVjNI9Gg+lzbT0KoQI90Iexwcloq22MDFPJC3EqirDpwBdkZFJA # 5/6iGvPILD1SVkAzjdMD6Ukc8zWwPVLENwcTCVxr8H2q6/Jbo+29WIlCM+/0a0t3 # 1kN77Yx1So9VFUOqPjHsoO72Wc/Zved1ZDqg9UgqP6L2dH2ns0mh92QM+Pplqi2T # NsJQih4NeZsfDQ7rm0oMcvFqV21a62zZYDi/KqFePRAs5D9K4PU2EmR7jcvf4uax # ZRyHsqDh00/OSYE/CefriyIMshWmTVCDbT/I3/SCXZX1scrKa3k= # =UO/N # -----END PGP SIGNATURE----- # gpg: Signature made Mon, May 27, 2024 3:53:53 PM # gpg: using RSA key BA66F67FD73F7058D712D308C3B7C91B632F738A # gpg: Can't check signature: No public key # Conflicts: # .github/workflows/release-nightly.yml

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 25, 2024

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 25, 2024

github-actions bot added the modifies/templates This PR modifies the template files label Mar 25, 2024

kemzeb marked this pull request as draft March 25, 2024 08:20

kemzeb changed the title ~~Hide re-request review button when rendering merged PR~~ [WIP] Hide re-request review button when rendering merged PR Mar 25, 2024

kemzeb force-pushed the fix-pr-sidebar branch from 7fb99e4 to 1af1f65 Compare March 25, 2024 10:46

pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 25, 2024

github-actions bot added the modifies/go Pull requests that update Go code label Mar 25, 2024

kemzeb changed the title ~~[WIP] Hide re-request review button when rendering merged PR~~ Don't allow adding or removing review requests for a merged PR Mar 25, 2024

kemzeb marked this pull request as ready for review March 25, 2024 11:00

kemzeb changed the title ~~Don't allow adding or removing review requests for a merged PR~~ [WIP] Don't allow adding or removing review requests for a merged PR Mar 25, 2024

wxiaoguang reviewed Mar 26, 2024

View reviewed changes

routers/web/repo/issue.go Outdated Show resolved Hide resolved

wxiaoguang approved these changes Mar 26, 2024

View reviewed changes

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 Mar 26, 2024

kemzeb force-pushed the fix-pr-sidebar branch from 1af1f65 to 4ca7b0f Compare March 26, 2024 03:30

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 26, 2024

kemzeb changed the title ~~[WIP] Don't allow adding or removing review requests for a merged PR~~ Prevent re-review and dismiss review actions on merged PRs Mar 26, 2024

kemzeb force-pushed the fix-pr-sidebar branch from 4ca7b0f to 5b972b8 Compare March 26, 2024 04:04

kemzeb commented Mar 26, 2024

View reviewed changes

models/issues/review.go Outdated Show resolved Hide resolved

lunny reviewed Mar 27, 2024

View reviewed changes

models/issues/review.go Outdated Show resolved Hide resolved

Prevent re-review and dismiss review actions on closed or merged PRs

12139f5

kemzeb force-pushed the fix-pr-sidebar branch from 5b972b8 to 12139f5 Compare March 27, 2024 06:01

kemzeb changed the title ~~Prevent re-review and dismiss review actions on merged PRs~~ Prevent re-review and dismiss review actions on closed and merged PRs Mar 27, 2024

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 Mar 27, 2024

kemzeb force-pushed the fix-pr-sidebar branch from a84120c to 887ee2d Compare March 27, 2024 09:41

Add unit tests

13d98fb

kemzeb force-pushed the fix-pr-sidebar branch from 887ee2d to 13d98fb Compare March 27, 2024 09:57

github-actions bot added the modifies/api This PR adds API routes or modifies them label Mar 27, 2024

Make sure API endpoints handle re-review/dismiss on close/merged PRs

9f9e7e3

kemzeb force-pushed the fix-pr-sidebar branch from 898cfad to 9f9e7e3 Compare March 27, 2024 18:56

silverwind approved these changes Mar 28, 2024

View reviewed changes

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 Mar 28, 2024

lunny added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. type/enhancement An improvement of existing functionality labels Mar 28, 2024

Merge branch 'main' into fix-pr-sidebar

38680bb

lunny enabled auto-merge (squash) March 28, 2024 14:52

lunny merged commit 242b331 into go-gitea:main Mar 28, 2024
26 checks passed

GiteaBot added this to the 1.23.0 milestone Mar 28, 2024

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 28, 2024

kemzeb deleted the fix-pr-sidebar branch March 28, 2024 16:02

techknowlogick modified the milestones: 1.23.0, 1.22.0 Mar 28, 2024

go-gitea locked as resolved and limited conversation to collaborators Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent re-review and dismiss review actions on closed and merged PRs #30065

Prevent re-review and dismiss review actions on closed and merged PRs #30065

kemzeb commented Mar 25, 2024 •

edited

Loading

lunny commented Mar 25, 2024

kemzeb commented Mar 25, 2024

kemzeb commented Mar 25, 2024 •

edited

Loading

silverwind commented Mar 25, 2024 •

edited

Loading

kemzeb commented Mar 25, 2024

kemzeb commented Mar 26, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024

Prevent re-review and dismiss review actions on closed and merged PRs #30065

Prevent re-review and dismiss review actions on closed and merged PRs #30065

Conversation

kemzeb commented Mar 25, 2024 • edited Loading

lunny commented Mar 25, 2024

kemzeb commented Mar 25, 2024

kemzeb commented Mar 25, 2024 • edited Loading

silverwind commented Mar 25, 2024 • edited Loading

kemzeb commented Mar 25, 2024

kemzeb commented Mar 26, 2024 • edited Loading

kemzeb commented Mar 27, 2024 • edited Loading

kemzeb commented Mar 27, 2024 • edited Loading

kemzeb commented Mar 27, 2024 • edited Loading

kemzeb commented Mar 27, 2024

kemzeb commented Mar 25, 2024 •

edited

Loading

kemzeb commented Mar 25, 2024 •

edited

Loading

silverwind commented Mar 25, 2024 •

edited

Loading

kemzeb commented Mar 26, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024 •

edited

Loading

kemzeb commented Mar 27, 2024 •

edited

Loading