Skip to content

Add reviewers selection to new pull request form #26596

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

Closed
wants to merge 13 commits into from
4 changes: 3 additions & 1 deletion modules/structs/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ type CreatePullRequestOption struct {
Milestone int64 `json:"milestone"`
Labels []int64 `json:"labels"`
// swagger:strfmt date-time
Deadline *time.Time `json:"due_date"`
Deadline *time.Time `json:"due_date"`
Reviewers []string `json:"reviewers"`
TeamReviewers []string `json:"team_reviewers"`
}

// EditPullRequestOption options when modify pull request
Expand Down
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,7 @@ issues.new.assignees = Assignees
issues.new.clear_assignees = Clear assignees
issues.new.no_assignees = No Assignees
issues.new.no_reviewers = No reviewers
issues.new.clear_reviewers = Clear reviewers
issues.choose.get_started = Get Started
issues.choose.open_external_link = Open
issues.choose.blank = Default
Expand Down
41 changes: 40 additions & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
activities_model "code.gitea.io/gitea/models/activities"
git_model "code.gitea.io/gitea/models/git"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/organization"
access_model "code.gitea.io/gitea/models/perm/access"
pull_model "code.gitea.io/gitea/models/pull"
repo_model "code.gitea.io/gitea/models/repo"
Expand Down Expand Up @@ -420,8 +421,46 @@ func CreatePullRequest(ctx *context.APIContext) {
return
}
}
// handle reviewers
var reviewerIds []int64

if err := pull_service.NewPullRequest(ctx, repo, prIssue, labelIDs, []string{}, pr, assigneeIDs); err != nil {
for _, r := range form.Reviewers {
var reviewer *user_model.User
if strings.Contains(r, "@") {
reviewer, err = user_model.GetUserByEmail(ctx, r)
} else {
reviewer, err = user_model.GetUserByName(ctx, r)
}

if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.NotFound("UserNotExist", fmt.Sprintf("User with id '%s' not exist", r))
return
}
ctx.Error(http.StatusInternalServerError, "GetUser", err)
return
}
reviewerIds = append(reviewerIds, reviewer.ID)
}

// handle teams as reviewers
if ctx.Repo.Repository.Owner.IsOrganization() && len(form.TeamReviewers) > 0 {
for _, t := range form.TeamReviewers {
var teamReviewer *organization.Team
teamReviewer, err = organization.GetTeam(ctx, ctx.Repo.Owner.ID, t)
if err != nil {
if organization.IsErrTeamNotExist(err) {
ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t))
return
}
ctx.Error(http.StatusInternalServerError, "ReviewRequest", err)
return
}
reviewerIds = append(reviewerIds, -teamReviewer.ID)
}
}

if err := pull_service.NewPullRequest(ctx, repo, prIssue, labelIDs, []string{}, pr, assigneeIDs, reviewerIds); err != nil {
if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) {
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err)
return
Expand Down
43 changes: 43 additions & 0 deletions routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/models/db"
git_model "code.gitea.io/gitea/models/git"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/organization"
access_model "code.gitea.io/gitea/models/perm/access"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
Expand All @@ -36,6 +37,7 @@ import (
"code.gitea.io/gitea/modules/upload"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/services/gitdiff"
repo_service "code.gitea.io/gitea/services/repository"
)

const (
Expand Down Expand Up @@ -800,6 +802,47 @@ func CompareDiff(ctx *context.Context) {
if ctx.Written() {
return
}

// Get reviewer info for pr
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be moved into RetrieveRepoMetas, where similar code exists for populating ctx.Data["Assignees"]?

var (
reviewers []*user_model.User
teamReviewers []*organization.Team
reviewersResult []*repoReviewerSelection
)
reviewers, err = repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, ctx.Doer.ID)
if err != nil {
ctx.ServerError("GetReviewers", err)
return
}

teamReviewers, err = repo_service.GetReviewerTeams(ctx, ctx.Repo.Repository)
if err != nil {
ctx.ServerError("GetReviewerTeams", err)
return
}

for _, user := range reviewers {
reviewersResult = append(reviewersResult, &repoReviewerSelection{
IsTeam: false,
CanChange: true,
User: user,
ItemID: user.ID,
})
}

// negative reviewIDs represent team requests
for _, team := range teamReviewers {
reviewersResult = append(reviewersResult, &repoReviewerSelection{
IsTeam: true,
CanChange: true,
Team: team,
ItemID: -team.ID,
})
}
ctx.Data["Reviewers"] = reviewersResult
if ctx.Written() {
return
}
}
}
beforeCommitID := ctx.Data["BeforeCommitID"].(string)
Expand Down
56 changes: 43 additions & 13 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1075,15 +1075,15 @@ func DeleteIssue(ctx *context.Context) {
}

// ValidateRepoMetas check and returns repository's meta information
func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull bool) ([]int64, []int64, int64, int64) {
func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull bool) ([]int64, []int64, []int64, int64, int64) {
var (
repo = ctx.Repo.Repository
err error
)

labels := RetrieveRepoMetas(ctx, ctx.Repo.Repository, isPull)
if ctx.Written() {
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}

var labelIDs []int64
Expand All @@ -1092,7 +1092,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
if len(form.LabelIDs) > 0 {
labelIDs, err = base.StringsToInt64s(strings.Split(form.LabelIDs, ","))
if err != nil {
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}
labelIDMark := make(container.Set[int64])
labelIDMark.AddMultiple(labelIDs...)
Expand All @@ -1115,11 +1115,11 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
milestone, err := issues_model.GetMilestoneByRepoID(ctx, ctx.Repo.Repository.ID, milestoneID)
if err != nil {
ctx.ServerError("GetMilestoneByID", err)
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}
if milestone.RepoID != repo.ID {
ctx.ServerError("GetMilestoneByID", err)
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}
ctx.Data["Milestone"] = milestone
ctx.Data["milestone_id"] = milestoneID
Expand All @@ -1129,11 +1129,11 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
p, err := project_model.GetProjectByID(ctx, form.ProjectID)
if err != nil {
ctx.ServerError("GetProjectByID", err)
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}
if p.RepoID != ctx.Repo.Repository.ID && p.OwnerID != ctx.Repo.Repository.OwnerID {
ctx.NotFound("", nil)
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}

ctx.Data["Project"] = p
Expand All @@ -1145,26 +1145,26 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
if len(form.AssigneeIDs) > 0 {
assigneeIDs, err = base.StringsToInt64s(strings.Split(form.AssigneeIDs, ","))
if err != nil {
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}

// Check if the passed assignees actually exists and is assignable
for _, aID := range assigneeIDs {
assignee, err := user_model.GetUserByID(ctx, aID)
if err != nil {
ctx.ServerError("GetUserByID", err)
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}

valid, err := access_model.CanBeAssigned(ctx, assignee, repo, isPull)
if err != nil {
ctx.ServerError("CanBeAssigned", err)
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}

if !valid {
ctx.ServerError("canBeAssigned", repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name})
return nil, nil, 0, 0
return nil, nil, nil, 0, 0
}
}
}
Expand All @@ -1174,7 +1174,37 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
assigneeIDs = append(assigneeIDs, form.AssigneeID)
}

return labelIDs, assigneeIDs, milestoneID, form.ProjectID
// Check reviewers
var reviewerIDs []int64
if isPull {
if len(form.ReviewerIDs) > 0 {
reviewerIDs, err = base.StringsToInt64s(strings.Split(form.ReviewerIDs, ","))
if err != nil {
return nil, nil, nil, 0, 0
}

// Check if the passed reviewers (user/team) actually exist
for _, rID := range reviewerIDs {
// negative reviewIDs represent team requests
if rID < 0 {
_, err := organization.GetTeamByID(ctx, -rID)
if err != nil {
ctx.ServerError("GetTeamByID", err)
return nil, nil, nil, 0, 0
}
continue
}

_, err := user_model.GetUserByID(ctx, rID)
if err != nil {
ctx.ServerError("GetUserByID", err)
return nil, nil, nil, 0, 0
}
}
}
}

return labelIDs, assigneeIDs, reviewerIDs, milestoneID, form.ProjectID
}

// NewIssuePost response for creating new issue
Expand All @@ -1192,7 +1222,7 @@ func NewIssuePost(ctx *context.Context) {
attachments []string
)

labelIDs, assigneeIDs, milestoneID, projectID := ValidateRepoMetas(ctx, *form, false)
labelIDs, assigneeIDs, _, milestoneID, projectID := ValidateRepoMetas(ctx, *form, false)
if ctx.Written() {
return
}
Expand Down
4 changes: 2 additions & 2 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -1379,7 +1379,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
return
}

labelIDs, assigneeIDs, milestoneID, _ := ValidateRepoMetas(ctx, *form, true)
labelIDs, assigneeIDs, reviewerIDs, milestoneID, _ := ValidateRepoMetas(ctx, *form, true)
if ctx.Written() {
return
}
Expand Down Expand Up @@ -1429,7 +1429,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
// FIXME: check error in the case two people send pull request at almost same time, give nice error prompt
// instead of 500.

if err := pull_service.NewPullRequest(ctx, repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs); err != nil {
if err := pull_service.NewPullRequest(ctx, repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs, reviewerIDs); err != nil {
if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) {
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err.Error())
return
Expand Down
2 changes: 1 addition & 1 deletion services/agit/agit.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
Flow: issues_model.PullRequestFlowAGit,
}

if err := pull_service.NewPullRequest(ctx, repo, prIssue, []int64{}, []string{}, pr, []int64{}); err != nil {
if err := pull_service.NewPullRequest(ctx, repo, prIssue, []int64{}, []string{}, pr, []int64{}, []int64{}); err != nil {
return nil, err
}

Expand Down
1 change: 1 addition & 0 deletions services/forms/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ type CreateIssueForm struct {
Title string `binding:"Required;MaxSize(255)"`
LabelIDs string `form:"label_ids"`
AssigneeIDs string `form:"assignee_ids"`
ReviewerIDs string `form:"reviewer_ids"`
Ref string `form:"ref"`
MilestoneID int64
ProjectID int64
Expand Down
40 changes: 39 additions & 1 deletion services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"code.gitea.io/gitea/models/db"
git_model "code.gitea.io/gitea/models/git"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/organization"
access_model "code.gitea.io/gitea/models/perm/access"
repo_model "code.gitea.io/gitea/models/repo"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/base"
Expand All @@ -37,7 +39,7 @@ import (
var pullWorkingPool = sync.NewExclusivePool()

// NewPullRequest creates new pull request with labels for repository.
func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error {
func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs, reviewerIDs []int64) error {
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
if err != nil {
if !git_model.IsErrBranchNotExist(err) {
Expand Down Expand Up @@ -80,6 +82,42 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
assigneeCommentMap[assigneeID] = comment
}

for _, reviewerID := range reviewerIDs {
// negative reviewIDs represent team requests
if reviewerID < 0 {
team, err := organization.GetTeamByID(ctx, -reviewerID)
if err != nil {
return err
}
err = issue_service.IsValidTeamReviewRequest(ctx, team, issue.Poster, true, issue)
if err != nil {
return err
}
_, err = issue_service.TeamReviewRequest(ctx, issue, issue.Poster, team, true)
if err != nil {
return err
}
continue
}

reviewer, err := user_model.GetUserByID(ctx, reviewerID)
if err != nil {
return err
}
permDoer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, issue.Poster)
if err != nil {
return err
}
err = issue_service.IsValidReviewRequest(ctx, reviewer, issue.Poster, true, issue, &permDoer)
if err != nil {
return err
}
_, err = issue_service.ReviewRequest(ctx, issue, issue.Poster, reviewer, true)
Copy link
Member

Choose a reason for hiding this comment

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

This also creates the review request notification. So the user will receive the "review requested" notification before the "pull request created" notification. If you look at the assignee code, they are first added without sending a notification. The notification is sent later on:

notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID])

Due to this, it also seems like the UI notification cannot be created. I'm seeing this in the logs:

2023/12/30 13:20:58 ...tification/notify.go:56:handler() [E] Was unable to create issue notification: issue does not exist [id: 10, repo_id: 0, index: 0]

I suspect this is because the notifier runs in a different transaction and does not "see" the newly created PR yet?

if err != nil {
return err
}
}

pr.Issue = issue
issue.PullRequest = pr

Expand Down
Loading