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

Add reviewers selection to new pull request fixes #26289 #32403

Merged
merged 30 commits into from
Nov 9, 2024

Conversation

CalK16
Copy link
Contributor

@CalK16 CalK16 commented Nov 2, 2024

The UI looks just like the assignee selection here because I think the actions from the pull request edit dialog don't make much sense here. The assignee selection on the pr create and edit pages also differ.

图片

图片


Hi, I've not done this before, so bear with me. I tried to make changes on @splitt3r 's pull request but was rejected due to a permission issue. Therefore, I had to fork my own repository and incorporate what was done in that pull request. In addition to @splitt3r and @sebastian-sauer's commits, I've also added a commit to resolve the conflicts. Furthermore, I have included two other commits to address @jpraet 's comments.

Please let me know if there is anything else I need to address. Thank you!

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 2, 2024
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 2, 2024
@github-actions github-actions bot added modifies/translation 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 modifies/frontend labels Nov 2, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 2, 2024
@CalK16 CalK16 changed the title Add reviewers selection to new pull request form #26596 Add reviewers selection to new pull request fixes #26289 Nov 2, 2024
@wxiaoguang
Copy link
Contributor

Thank you all for the work.

Actually IMO there are still problems in this PR, for example: incorrect CSS classes like gt-df.

There is also a blocker in repo-legacy.ts, I have proposed a refactoring PR "Refactor repo legacy #32404", after #32404, I could help to resolve the conflicts here and fix the problems.

@lunny
Copy link
Member

lunny commented Nov 2, 2024

Does this replace #26596 ?

@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 8, 2024
@wxiaoguang
Copy link
Contributor

Made some changes, and merge the duplicate code.

Now we just use one "reviewer_list.tmpl" to handle the selection.

And the new initIssueSidebarComboList should be easier to understand and maintain, the legacy code should be refactored to the new mechanism later.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

It's almost done from my side and I would vote my approval.

Some changes seem more than expected, indeed they were done for improving the maintainability, some legacy functions are too complex to maintain.

I have manually tested the New PR page and PR view page. If anything wrong happens, please let me know and I will fix them ASAP.

Screenshots:

image

image

image

image

@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 Nov 8, 2024
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Nov 8, 2024
@wxiaoguang wxiaoguang added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 8, 2024
@wxiaoguang wxiaoguang force-pushed the main branch 3 times, most recently from 44f8163 to 823b1c9 Compare November 9, 2024 02:10
@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 Nov 9, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 9, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) November 9, 2024 04:11
@wxiaoguang wxiaoguang disabled auto-merge November 9, 2024 04:24
@wxiaoguang wxiaoguang enabled auto-merge (squash) November 9, 2024 04:26
@wxiaoguang wxiaoguang linked an issue Nov 9, 2024 that may be closed by this pull request
@wxiaoguang wxiaoguang merged commit 18aeca5 into go-gitea:main Nov 9, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 9, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 10, 2024
* giteaofficial/main:
  Refactor sidebar label selector (go-gitea#32460)
  Fix mermaid diagram height when initially hidden (go-gitea#32457)
  Add reviewers selection to new pull request (go-gitea#32403)
  Fix issue sidebar (go-gitea#32455)
  Refactor language menu and dom utils (go-gitea#32450)
  Fix broken releases when re-pushing tags (go-gitea#32435)
  Refactor issue page info (go-gitea#32445)
  Split issue sidebar into small templates (go-gitea#32444)
  Only provide the commit summary for Discord webhook push events (go-gitea#32432)
  Add new event commit status creation and webhook implementation (go-gitea#27151)
  Support quote selected comments to reply (go-gitea#32431)
  Move AddCollabrator and CreateRepositoryByExample to service layer (go-gitea#32419)

# Conflicts:
#	templates/repo/issue/view_content/context_menu.tmpl
@@ -197,7 +210,17 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
}
notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID])
}

permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these changes are duplicated as line 193 issue_service.ReviewRequestNotify

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assign reviewers on pull request creation
6 participants