Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1350,6 +1350,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
2 changes: 1 addition & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ func CreatePullRequest(ctx *context.APIContext) {
}
}

if err := pull_service.NewPullRequest(ctx, repo, prIssue, labelIDs, []string{}, pr, assigneeIDs); err != nil {
if err := pull_service.NewPullRequest(ctx, repo, prIssue, labelIDs, []string{}, pr, assigneeIDs, []int64{}); 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 @@ -35,6 +36,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 @@ -791,6 +793,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
54 changes: 41 additions & 13 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1045,15 +1045,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 @@ -1062,7 +1062,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 @@ -1085,11 +1085,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 @@ -1099,11 +1099,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 @@ -1115,26 +1115,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 @@ -1144,7 +1144,35 @@ 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 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 @@ -1162,7 +1190,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 @@ -1363,7 +1363,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 @@ -1413,7 +1413,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 @@ -439,6 +439,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 []int64, 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
56 changes: 56 additions & 0 deletions templates/repo/issue/new_form.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,62 @@

<div class="issue-content-right ui segment">
{{template "repo/issue/branch_selector_field" .}}
{{if .PageIsComparePull}}
<input id="reviewer_ids" name="reviewer_ids" type="hidden" value="{{.reviewer_ids}}">
<div class="ui {{if not .Reviewers}}disabled{{end}} floating jump select-reviewers dropdown">
<span class="text flex-text-block">
<strong>{{.locale.Tr "repo.issues.review.reviewers"}}</strong>
{{svg "octicon-gear" 16 "gt-ml-2"}}
</span>
<div class="filter menu" data-id="#reviewer_ids">
<div class="ui icon search input">
<i class="icon gt-df gt-ac gt-jc">{{svg "octicon-search" 16}}</i>
<input type="text" placeholder="{{.locale.Tr "repo.issues.filter_reviewers"}}">
</div>
<div class="no-select item">{{.locale.Tr "repo.issues.new.clear_reviewers"}}</div>
{{range .Reviewers}}
{{if .User}}
<a class="item muted" data-id="{{.ItemID}}" data-id-selector="#reviewer_{{.ItemID}}">
<span class="octicon-check invisible">{{svg "octicon-check"}}</span>
<span class="text">
{{ctx.AvatarUtils.Avatar .User 28 "gt-mr-3"}}{{template "repo/search_name" .User}}
</span>
</a>
{{end}}
{{end}}
<div class="divider"></div>
{{range .Reviewers}}
{{if .Team}}
<a class="item muted" data-id="{{.ItemID}}" data-id-selector="#reviewer_{{.ItemID}}">
<span class="octicon-check invisible">{{svg "octicon-check" 16}}</span>
<span class="text">
{{svg "octicon-people" 16 "gt-ml-4 gt-mr-2"}}{{$.Repository.OwnerName}}/{{.Team.Name}}
</span>
</a>
{{end}}
{{end}}
</div>
</div>
<div class="ui reviewers list">
<span class="no-select item">
{{.locale.Tr "repo.issues.new.no_reviewers"}}
</span>
<div class="selected">
{{range .Reviewers}}
{{if .User}}
<a class="item gt-p-2 muted gt-hidden" id="reviewer_{{.ItemID}}" href="#">
{{ctx.AvatarUtils.Avatar .User 28 "gt-mr-3 gt-vm"}}{{.User.GetDisplayName}}
</a>
{{else if .Team}}
<a class="item gt-p-2 muted gt-hidden" id="reviewer_{{.ItemID}}" href="#">
{{svg "octicon-people" 20 "gt-mr-3"}}{{$.Repository.OwnerName}}/{{.Team.Name}}
</a>
{{end}}
{{end}}
</div>
</div>
<div class="divider"></div>
{{end}}

<input id="label_ids" name="label_ids" type="hidden" value="{{.label_ids}}">
{{template "repo/issue/labels/labels_selector_field" .}}
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/actions_trigger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
BaseRepo: baseRepo,
Type: issues_model.PullRequestGitea,
}
err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil)
err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil, nil)
assert.NoError(t, err)

// load and compare ActionRun
Expand Down Expand Up @@ -188,7 +188,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
BaseRepo: baseRepo,
Type: issues_model.PullRequestGitea,
}
err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil)
err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil, nil)
assert.NoError(t, err)

// the new pull request cannot trigger actions, so there is still only 1 record
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ func TestConflictChecking(t *testing.T) {
BaseRepo: baseRepo,
Type: issues_model.PullRequestGitea,
}
err = pull.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil)
err = pull.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil, nil)
assert.NoError(t, err)

issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"})
Expand Down
Loading