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

Fix reviewers notification #28543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

earl-warren
Copy link
Contributor

  • In PullRequestCodeOwnersReview use the correct functions that will
    notify added reviewers.

Refs: https://codeberg.org/forgejo/forgejo/pulls/1970

- Move codeowner related code out of the models package, as they quite
frankly don't have anything to do with database models. It has been
moved to `modules/codeowner` for the codeowner file parsing and
`services/issue` for the assinging of codeowners to pull requests.

Refs: https://codeberg.org/forgejo/forgejo/pulls/1970

(cherry picked from commit ea6942585d43dba8b038758782631e58ea8a34b4)
- In `PullRequestCodeOwnersReview` use the correct functions that will
notify added reviewers.

Refs: https://codeberg.org/forgejo/forgejo/pulls/1970

(cherry picked from commit c3bdc5698901768c6a3f016cde9ff03072a5cc77)
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 19, 2023
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 19, 2023

for _, u := range uniqUsers {
if u.ID != pull.Poster.ID {
if _, err := ReviewRequest(ctx, pull, pull.Poster, u, true); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

On services/pull/pull.go:129, the transaction hasn't been submitted but the notifications on ReviewRequest and TeamReviewRequest will be sent immediately. It's not safe enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants