diff --git a/models/issues/review.go b/models/issues/review.go index 1c5c2ee30ac86..24f8f34d36174 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -46,6 +46,7 @@ func (err ErrReviewNotExist) Unwrap() error { type ErrNotValidReviewRequest struct { Reason string UserID int64 + TeamID int64 RepoID int64 } @@ -56,9 +57,10 @@ func IsErrNotValidReviewRequest(err error) bool { } func (err ErrNotValidReviewRequest) Error() string { - return fmt.Sprintf("%s [user_id: %d, repo_id: %d]", + return fmt.Sprintf("%s [user_id: %d, team_id: %d, repo_id: %d]", err.Reason, err.UserID, + err.TeamID, err.RepoID) } @@ -740,7 +742,7 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user return nil, nil } - if _, err = db.DeleteByBean(ctx, review); err != nil { + if _, err = db.DeleteByID[Review](ctx, review.ID); err != nil { return nil, err } diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index a54225f0fd76d..011068d114a7f 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" ) @@ -324,7 +323,7 @@ func GetReviewers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0) + canChooseReviewer := pull_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0) if !canChooseReviewer { ctx.APIError(http.StatusForbidden, errors.New("doer has no permission to get reviewers")) return diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index f5d0e37c650c4..5668d8cbadd2f 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -15,6 +15,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" @@ -541,8 +542,16 @@ func CreatePullRequest(ctx *context.APIContext) { PullRequest: pr, AssigneeIDs: assigneeIDs, } - prOpts.Reviewers, prOpts.TeamReviewers = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers) - if ctx.Written() { + prOpts.Reviewers, prOpts.TeamReviewers, err = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers) + switch { + case user_model.IsErrUserNotExist(err): + ctx.APIErrorNotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name)) + return + case organization.IsErrTeamNotExist(err): + ctx.APIErrorNotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name)) + return + case err != nil: + ctx.APIError(http.StatusInternalServerError, err) return } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index fb35126a99d72..d11482e9f76f4 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -19,7 +19,6 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" ) @@ -611,7 +610,94 @@ func CreateReviewRequests(ctx *context.APIContext) { // "$ref": "#/responses/notFound" opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) - apiReviewRequest(ctx, *opts, true) + + // this will load issue + pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index")) + if err != nil { + if issues_model.IsErrPullRequestNotExist(err) { + ctx.APIErrorNotFound("GetPullRequestByIndex", err) + } else { + ctx.APIError(http.StatusInternalServerError, err) + } + return + } + + pr.Issue.Repo = ctx.Repo.Repository + + allowedUsers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, pr.Issue.PosterID) + if err != nil { + ctx.APIError(http.StatusInternalServerError, err) + return + } + filteredUsers := make([]*user_model.User, 0, len(opts.Reviewers)) + for _, reviewer := range opts.Reviewers { + found := false + for _, allowedUser := range allowedUsers { + if allowedUser.Name == reviewer || allowedUser.Email == reviewer { + filteredUsers = append(filteredUsers, allowedUser) + found = true + break + } + } + if !found { + ctx.APIError(http.StatusUnprocessableEntity, "") + return + } + } + + filteredTeams := make([]*organization.Team, 0, len(opts.TeamReviewers)) + if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { + allowedTeams, err := pull_service.GetReviewerTeams(ctx, ctx.Repo.Repository) + if err != nil { + ctx.APIError(http.StatusInternalServerError, err) + return + } + for _, teamReviewer := range opts.TeamReviewers { + found := false + for _, allowedTeam := range allowedTeams { + if allowedTeam.Name == teamReviewer { + filteredTeams = append(filteredTeams, allowedTeam) + found = true + break + } + } + if !found { + ctx.APIError(http.StatusUnprocessableEntity, "") + return + } + } + } + comments, err := pull_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams) + if err != nil { + if issues_model.IsErrReviewRequestOnClosedPR(err) { + ctx.APIError(http.StatusForbidden, err) + return + } + if issues_model.IsErrNotValidReviewRequest(err) { + ctx.APIError(http.StatusUnprocessableEntity, err) + return + } + ctx.APIError(http.StatusInternalServerError, err) + return + } + + reviews := make([]*issues_model.Review, 0, len(filteredUsers)) + for _, comment := range comments { + if comment != nil { + if err = comment.LoadReview(ctx); err != nil { + ctx.APIError(http.StatusInternalServerError, err) + return + } + reviews = append(reviews, comment.Review) + } + } + + apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer) + if err != nil { + ctx.APIError(http.StatusInternalServerError, err) + return + } + ctx.JSON(http.StatusCreated, apiReviews) } // DeleteReviewRequests delete review requests to an pull request @@ -653,11 +739,10 @@ func DeleteReviewRequests(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) - apiReviewRequest(ctx, *opts, false) + deleteReviewRequests(ctx, *opts) } -func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team) { - var err error +func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team, err error) { for _, r := range reviewerNames { var reviewer *user_model.User if strings.Contains(r, "@") { @@ -665,14 +750,8 @@ func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerN } else { reviewer, err = user_model.GetUserByName(ctx, r) } - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.APIErrorNotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r)) - return nil, nil - } - ctx.APIErrorInternal(err) - return nil, nil + return nil, nil, err } reviewers = append(reviewers, reviewer) @@ -680,24 +759,18 @@ func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerN if ctx.Repo.Repository.Owner.IsOrganization() && len(teamReviewerNames) > 0 { for _, t := range teamReviewerNames { - var teamReviewer *organization.Team - teamReviewer, err = organization.GetTeam(ctx, ctx.Repo.Owner.ID, t) + teamReviewer, err := organization.GetTeam(ctx, ctx.Repo.Owner.ID, t) if err != nil { - if organization.IsErrTeamNotExist(err) { - ctx.APIErrorNotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t)) - return nil, nil - } - ctx.APIErrorInternal(err) - return nil, nil + return nil, nil, err } teamReviewers = append(teamReviewers, teamReviewer) } } - return reviewers, teamReviewers + return reviewers, teamReviewers, nil } -func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions, isAdd bool) { +func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOptions) { pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index")) if err != nil { if issues_model.IsErrPullRequestNotExist(err) { @@ -719,18 +792,21 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions return } - reviewers, teamReviewers := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers) - if ctx.Written() { + reviewers, teamReviewers, err := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers) + switch { + case user_model.IsErrUserNotExist(err): + ctx.APIErrorNotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name)) + return + case organization.IsErrTeamNotExist(err): + ctx.APIErrorNotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name)) + return + case err != nil: + ctx.APIError(http.StatusInternalServerError, err) return - } - - var reviews []*issues_model.Review - if isAdd { - reviews = make([]*issues_model.Review, 0, len(reviewers)) } for _, reviewer := range reviewers { - comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd) + _, err := pull_service.ReviewRequest(ctx, pr, ctx.Doer, &permDoer, reviewer, false) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.APIError(http.StatusForbidden, err) @@ -743,19 +819,11 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions ctx.APIErrorInternal(err) return } - - if comment != nil && isAdd { - if err = comment.LoadReview(ctx); err != nil { - ctx.APIErrorInternal(err) - return - } - reviews = append(reviews, comment.Review) - } } if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { for _, teamReviewer := range teamReviewers { - comment, err := issue_service.TeamReviewRequest(ctx, pr.Issue, ctx.Doer, teamReviewer, isAdd) + _, err := pull_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, false) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.APIError(http.StatusForbidden, err) @@ -768,28 +836,10 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions ctx.APIErrorInternal(err) return } - - if comment != nil && isAdd { - if err = comment.LoadReview(ctx); err != nil { - ctx.APIErrorInternal(err) - return - } - reviews = append(reviews, comment.Review) - } } } - if isAdd { - apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer) - if err != nil { - ctx.APIErrorInternal(err) - return - } - ctx.JSON(http.StatusCreated, apiReviews) - } else { - ctx.Status(http.StatusNoContent) - return - } + ctx.Status(http.StatusNoContent) } // DismissPullReview dismiss a review for a pull request diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index 93cc38bffa1cf..c3dc7067aa9f4 100644 --- a/routers/web/repo/issue_page_meta.go +++ b/routers/web/repo/issue_page_meta.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/modules/optional" shared_user "code.gitea.io/gitea/routers/web/shared/user" "code.gitea.io/gitea/services/context" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" ) @@ -194,7 +193,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { if d.Issue == nil { data.CanChooseReviewer = true } else { - data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID) + data.CanChooseReviewer = pull_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID) } } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index fb92d24394c0d..317f1e9bd8aa1 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -20,7 +20,6 @@ import ( "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/forms" - issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" user_service "code.gitea.io/gitea/services/user" ) @@ -365,6 +364,10 @@ func UpdatePullReviewRequest(ctx *context.Context) { ctx.Status(http.StatusForbidden) return } + if err := issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("issue.LoadPullRequest", err) + return + } if reviewID < 0 { // negative reviewIDs represent team requests if err := issue.Repo.LoadOwner(ctx); err != nil { @@ -395,7 +398,8 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") + // TODO: Team review request should check if the team has permission to review the PR + _, err = pull_service.TeamReviewRequest(ctx, issue.PullRequest, ctx.Doer, team, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -427,7 +431,8 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") + // TODO: Reviewer review request should check if the user has permission to review the PR + _, err = pull_service.ReviewRequest(ctx, issue.PullRequest, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( diff --git a/routers/web/repo/view_file.go b/routers/web/repo/view_file.go index 4ce7a8e3a4480..5609b503bf040 100644 --- a/routers/web/repo/view_file.go +++ b/routers/web/repo/view_file.go @@ -25,6 +25,7 @@ import ( "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/context" issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" files_service "code.gitea.io/gitea/services/repository/files" "github.com/nektos/act/pkg/model" @@ -78,7 +79,7 @@ func prepareToRenderFile(ctx *context.Context, entry *git.TreeEntry) { if workFlowErr != nil { ctx.Data["FileError"] = ctx.Locale.Tr("actions.runs.invalid_workflow_helper", workFlowErr.Error()) } - } else if issue_service.IsCodeOwnerFile(ctx.Repo.TreePath) { + } else if pull_service.IsCodeOwnerFile(ctx.Repo.TreePath) { if data, err := blob.GetBlobContent(setting.UI.MaxDisplayFileSize); err == nil { _, warnings := issue_model.GetCodeOwnersFromContent(ctx, data) if len(warnings) > 0 { diff --git a/services/issue/assignee.go b/services/issue/assignee.go index c7e24955687f9..98c3c8c873c7b 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -7,13 +7,7 @@ import ( "context" issues_model "code.gitea.io/gitea/models/issues" - "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" - access_model "code.gitea.io/gitea/models/perm/access" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/log" notify_service "code.gitea.io/gitea/services/notify" ) @@ -59,268 +53,3 @@ func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, do return removed, comment, err } - -// ReviewRequest add or remove a review request from a user for this PR, and make comment for it. -func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { - err = isValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) - if err != nil { - return nil, err - } - - if isAdd { - comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) - } else { - comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) - } - - if err != nil { - return nil, err - } - - if comment != nil { - notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewer, isAdd, comment) - } - - return comment, err -} - -// isValidReviewRequest Check permission for ReviewRequest -func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { - if reviewer.IsOrganization() { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Organization can't be added as reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - if doer.IsOrganization() { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Organization can't be doer to add reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer) - if err != nil { - return err - } - - if permDoer == nil { - permDoer = new(access_model.Permission) - *permDoer, err = access_model.GetUserRepoPermission(ctx, issue.Repo, doer) - if err != nil { - return err - } - } - - lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) - if err != nil && !issues_model.IsErrReviewNotExist(err) { - return err - } - - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) - - if isAdd { - if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Reviewer can't read", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 { - return issues_model.ErrNotValidReviewRequest{ - Reason: "poster of pr can't be reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if canDoerChangeReviewRequests { - return nil - } - - if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastReview != nil && lastReview.Type != issues_model.ReviewTypeRequest { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't choose reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if canDoerChangeReviewRequests { - return nil - } - - if lastReview != nil && lastReview.Type == issues_model.ReviewTypeRequest && lastReview.ReviewerID == doer.ID { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't remove reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } -} - -// isValidTeamReviewRequest Check permission for ReviewRequest Team -func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { - if doer.IsOrganization() { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Organization can't be doer to add reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) - - if isAdd { - if issue.Repo.IsPrivate { - hasTeam := organization.HasTeamRepo(ctx, reviewer.OrgID, reviewer.ID, issue.RepoID) - - if !hasTeam { - return issues_model.ErrNotValidReviewRequest{ - Reason: "Reviewing team can't read repo", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - } - - if canDoerChangeReviewRequests { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't choose reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } - } - - if canDoerChangeReviewRequests { - return nil - } - - return issues_model.ErrNotValidReviewRequest{ - Reason: "Doer can't remove reviewer", - UserID: doer.ID, - RepoID: issue.Repo.ID, - } -} - -// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. -func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) { - err = isValidTeamReviewRequest(ctx, reviewer, doer, isAdd, issue) - if err != nil { - return nil, err - } - if isAdd { - comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) - } else { - comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) - } - - if err != nil { - return nil, err - } - - if comment == nil || !isAdd { - return nil, nil - } - - return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) -} - -func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { - for _, reviewNotifier := range reviewNotifiers { - if reviewNotifier.Reviewer != nil { - notify_service.PullRequestReviewRequest(ctx, issue.Poster, issue, reviewNotifier.Reviewer, reviewNotifier.IsAdd, reviewNotifier.Comment) - } else if reviewNotifier.ReviewTeam != nil { - if err := teamReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifier.ReviewTeam, reviewNotifier.IsAdd, reviewNotifier.Comment); err != nil { - log.Error("teamReviewRequestNotify: %v", err) - } - } - } -} - -// teamReviewRequestNotify notify all user in this team -func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool, comment *issues_model.Comment) error { - // notify all user in this team - if err := comment.LoadIssue(ctx); err != nil { - return err - } - - members, err := organization.GetTeamMembers(ctx, &organization.SearchMembersOptions{ - TeamID: reviewer.ID, - }) - if err != nil { - return err - } - - for _, member := range members { - if member.ID == comment.Issue.PosterID { - continue - } - comment.AssigneeID = member.ID - notify_service.PullRequestReviewRequest(ctx, doer, issue, member, isAdd, comment) - } - - return err -} - -// CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR -func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool { - if repo.IsArchived { - return false - } - // The poster of the PR can change the reviewers - if doer.ID == posterID { - return true - } - - // The owner of the repo can change the reviewers - if doer.ID == repo.OwnerID { - return true - } - - // Collaborators of the repo can change the reviewers - isCollaborator, err := repo_model.IsCollaborator(ctx, repo.ID, doer.ID) - if err != nil { - log.Error("IsCollaborator: %v", err) - return false - } - if isCollaborator { - return true - } - - // If the repo's owner is an organization, members of teams with read permission on pull requests can change reviewers - if repo.Owner.IsOrganization() { - teams, err := organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) - if err != nil { - log.Error("GetTeamsWithAccessToRepo: %v", err) - return false - } - for _, team := range teams { - if !team.UnitEnabled(ctx, unit.TypePullRequests) { - continue - } - isMember, err := organization.IsTeamMember(ctx, repo.OwnerID, team.ID, doer.ID) - if err != nil { - log.Error("IsTeamMember: %v", err) - continue - } - if isMember { - return true - } - } - } - - return false -} diff --git a/services/issue/issue.go b/services/issue/issue.go index 455a1ec29781b..7b39783bd098d 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -17,7 +17,6 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" notify_service "code.gitea.io/gitea/services/notify" ) @@ -90,21 +89,7 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode return err } - var reviewNotifiers []*ReviewRequestNotifier - if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { - if err := issue.LoadPullRequest(ctx); err != nil { - return err - } - - var err error - reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue.PullRequest) - if err != nil { - log.Error("PullRequestCodeOwnersReview: %v", err) - } - } - notify_service.IssueChangeTitle(ctx, doer, issue, oldTitle) - ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) return nil } diff --git a/services/issue/pull.go b/services/issue/pull.go deleted file mode 100644 index bd19c254362b9..0000000000000 --- a/services/issue/pull.go +++ /dev/null @@ -1,169 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package issue - -import ( - "context" - "fmt" - "slices" - "time" - - issues_model "code.gitea.io/gitea/models/issues" - org_model "code.gitea.io/gitea/models/organization" - user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/gitrepo" - "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/setting" -) - -func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) { - // Add a temporary remote - tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano()) - if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil { - return "", fmt.Errorf("AddRemote: %w", err) - } - defer func() { - if err := repo.RemoveRemote(tmpRemote); err != nil { - log.Error("getMergeBase: RemoveRemote: %v", err) - } - }() - - mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch) - return mergeBase, err -} - -type ReviewRequestNotifier struct { - Comment *issues_model.Comment - IsAdd bool - Reviewer *user_model.User - ReviewTeam *org_model.Team -} - -var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - -func IsCodeOwnerFile(f string) bool { - return slices.Contains(codeOwnerFiles, f) -} - -func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { - return PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, "", "") // no commit is provided, then it uses PR's base&head branch -} - -func PullRequestCodeOwnersReviewSpecialCommits(ctx context.Context, pr *issues_model.PullRequest, startCommitID, endCommitID string) ([]*ReviewRequestNotifier, error) { - if err := pr.LoadIssue(ctx); err != nil { - return nil, err - } - issue := pr.Issue - if pr.IsWorkInProgress(ctx) { - return nil, nil - } - if err := pr.LoadHeadRepo(ctx); err != nil { - return nil, err - } - if err := pr.LoadBaseRepo(ctx); err != nil { - return nil, err - } - pr.Issue.Repo = pr.BaseRepo - - if pr.BaseRepo.IsFork { - return nil, nil - } - - repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - return nil, err - } - defer repo.Close() - - commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch) - if err != nil { - return nil, err - } - - var data string - for _, file := range codeOwnerFiles { - if blob, err := commit.GetBlobByPath(file); err == nil { - data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) - if err == nil { - break - } - } - } - if data == "" { - return nil, nil - } - - rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) - if len(rules) == 0 { - return nil, nil - } - - if startCommitID == "" && endCommitID == "" { - // get the mergebase - mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) - if err != nil { - return nil, err - } - startCommitID = mergeBase - endCommitID = pr.GetGitRefName() - } - - // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed - // between the merge base and the head commit but not the base branch and the head commit - changedFiles, err := repo.GetFilesChangedBetween(startCommitID, endCommitID) - if err != nil { - return nil, err - } - - uniqUsers := make(map[int64]*user_model.User) - uniqTeams := make(map[string]*org_model.Team) - for _, rule := range rules { - for _, f := range changedFiles { - if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) { - for _, u := range rule.Users { - uniqUsers[u.ID] = u - } - for _, t := range rule.Teams { - uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t - } - } - } - } - - notifiers := make([]*ReviewRequestNotifier, 0, len(uniqUsers)+len(uniqTeams)) - - if err := issue.LoadPoster(ctx); err != nil { - return nil, err - } - - for _, u := range uniqUsers { - if u.ID != issue.Poster.ID { - comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster) - if err != nil { - log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) - return nil, err - } - notifiers = append(notifiers, &ReviewRequestNotifier{ - Comment: comment, - IsAdd: true, - Reviewer: u, - }) - } - } - for _, t := range uniqTeams { - comment, err := issues_model.AddTeamReviewRequest(ctx, issue, t, issue.Poster) - if err != nil { - log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) - return nil, err - } - notifiers = append(notifiers, &ReviewRequestNotifier{ - Comment: comment, - IsAdd: true, - ReviewTeam: t, - }) - } - - return notifiers, nil -} diff --git a/services/pull/check.go b/services/pull/check.go index 9b159891d70f7..99ad3bf2bea2a 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -398,11 +398,12 @@ func CheckPRsForBaseBranch(ctx context.Context, baseRepo *repo_model.Repository, // Init runs the task queue to test all the checking status pull requests func Init() error { prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", handler) - if prPatchCheckerQueue == nil { return fmt.Errorf("unable to create pr_patch_checker queue") } + notify_service.RegisterNotifier(newNotifier()) + go graceful.GetManager().RunWithCancel(prPatchCheckerQueue) go graceful.GetManager().RunWithShutdownContext(InitializePullRequests) return nil diff --git a/services/pull/notify.go b/services/pull/notify.go new file mode 100644 index 0000000000000..c4d630de60d7c --- /dev/null +++ b/services/pull/notify.go @@ -0,0 +1,36 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + + issues_model "code.gitea.io/gitea/models/issues" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" + notify_service "code.gitea.io/gitea/services/notify" +) + +type pullNotifier struct { + notify_service.NullNotifier +} + +var _ notify_service.Notifier = &pullNotifier{} + +// newNotifier create a new indexerNotifier notifier +func newNotifier() notify_service.Notifier { + return &pullNotifier{} +} + +func (r *pullNotifier) IssueChangeTitle(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldTitle string) { + var reviewNotifiers []*ReviewRequestNotifier + if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(issue.Title) { + var err error + reviewNotifiers, err = RequestCodeOwnersReview(ctx, issue.PullRequest) + if err != nil { + log.Error("RequestCodeOwnersReview: %v", err) + } + } + ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 4641d4ac40491..21f632ce6b355 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -64,6 +64,46 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return user_model.ErrBlockedUser } + // check if reviewers are valid + if len(opts.Reviewers) > 0 { + allowedUsers, err := GetReviewers(ctx, repo, pr.Issue.PosterID, pr.Issue.PosterID) + if err != nil { + return err + } + for _, reviewer := range opts.Reviewers { + var found bool + for _, allowedUser := range allowedUsers { + if allowedUser.ID == reviewer.ID { + found = true + break + } + } + if !found { + return issues_model.ErrNotValidReviewRequest{UserID: reviewer.ID} + } + } + } + + // check if team reviewers are valid + if len(opts.TeamReviewers) > 0 { + allowedTeams, err := GetReviewerTeams(ctx, repo) + if err != nil { + return err + } + for _, teamReviewer := range opts.TeamReviewers { + var found bool + for _, allowedTeam := range allowedTeams { + if allowedTeam.ID == teamReviewer.ID { + found = true + break + } + } + if !found { + return issues_model.ErrNotValidReviewRequest{TeamID: teamReviewer.ID} + } + } + } + // user should be a collaborator or a member of the organization for base repo canCreate := issue.Poster.IsAdmin || pr.Flow == issues_model.PullRequestFlowAGit if !canCreate { @@ -116,7 +156,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } defer baseGitRepo.Close() - var reviewNotifiers []*issue_service.ReviewRequestNotifier + var reviewNotifiers []*ReviewRequestNotifier if err := db.WithTx(ctx, func(ctx context.Context) error { if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { return err @@ -175,8 +215,33 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } - if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) + // if there are reviewers or review teams, we don't need to request code owners review + if len(opts.Reviewers)+len(opts.TeamReviewers) > 0 { + for _, reviewer := range opts.Reviewers { + comment, err := issues_model.AddReviewRequest(ctx, pr.Issue, reviewer, issue.Poster) + if err != nil { + return err + } + reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ + Comment: comment, + Reviewer: reviewer, + IsAdd: true, + }) + } + + for _, teamReviewer := range opts.TeamReviewers { + comment, err := issues_model.AddTeamReviewRequest(ctx, pr.Issue, teamReviewer, issue.Poster) + if err != nil { + return err + } + reviewNotifiers = append(reviewNotifiers, &ReviewRequestNotifier{ + Comment: comment, + ReviewTeam: teamReviewer, + IsAdd: true, + }) + } + } else if !pr.IsWorkInProgress(ctx) { + reviewNotifiers, err = RequestCodeOwnersReview(ctx, pr) if err != nil { return err } @@ -191,7 +256,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } baseGitRepo.Close() // close immediately to avoid notifications will open the repository again - issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) + ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { @@ -211,17 +276,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID]) } - permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) - for _, reviewer := range opts.Reviewers { - if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { - return err - } - } - for _, teamReviewer := range opts.TeamReviewers { - if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil { - return err - } - } + return nil } @@ -398,6 +453,11 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { return } + if _, err = prs.LoadIssues(ctx); err != nil { + log.Error("LoadIssues: %v", err) + return + } + for _, pr := range prs { log.Trace("Updating PR[%d]: composing new test task", pr.ID) if pr.Flow == issues_model.PullRequestFlowGithub { @@ -463,17 +523,17 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } if !pr.IsWorkInProgress(ctx) { - var reviewNotifiers []*issue_service.ReviewRequestNotifier + var reviewNotifiers []*ReviewRequestNotifier if opts.IsForcePush { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) + reviewNotifiers, err = RequestCodeOwnersReview(ctx, pr) } else { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, opts.OldCommitID, opts.NewCommitID) + reviewNotifiers, err = RequestCodeOwnersReviewSpecialCommits(ctx, pr, opts.OldCommitID, opts.NewCommitID) } if err != nil { log.Error("PullRequestCodeOwnersReview: %v", err) } if len(reviewNotifiers) > 0 { - issue_service.ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) + ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) } } diff --git a/services/pull/review_request.go b/services/pull/review_request.go new file mode 100644 index 0000000000000..825f423a8ba0d --- /dev/null +++ b/services/pull/review_request.go @@ -0,0 +1,465 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + "fmt" + "slices" + "time" + + issues_model "code.gitea.io/gitea/models/issues" + org_model "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + notify_service "code.gitea.io/gitea/services/notify" +) + +func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) { + // Add a temporary remote + tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano()) + if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil { + return "", fmt.Errorf("AddRemote: %w", err) + } + defer func() { + if err := repo.RemoveRemote(tmpRemote); err != nil { + log.Error("getMergeBase: RemoveRemote: %v", err) + } + }() + + mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch) + return mergeBase, err +} + +type ReviewRequestNotifier struct { + Comment *issues_model.Comment + IsAdd bool + Reviewer *user_model.User + ReviewTeam *org_model.Team +} + +var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} + +func IsCodeOwnerFile(f string) bool { + return slices.Contains(codeOwnerFiles, f) +} + +func RequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { + return RequestCodeOwnersReviewSpecialCommits(ctx, pr, "", "") // no commit is provided, then it uses PR's base&head branch +} + +func RequestCodeOwnersReviewSpecialCommits(ctx context.Context, pr *issues_model.PullRequest, startCommitID, endCommitID string) ([]*ReviewRequestNotifier, error) { + if err := pr.LoadIssue(ctx); err != nil { + return nil, err + } + issue := pr.Issue + if pr.IsWorkInProgress(ctx) { + return nil, nil + } + if err := pr.LoadHeadRepo(ctx); err != nil { + return nil, err + } + if err := pr.LoadBaseRepo(ctx); err != nil { + return nil, err + } + pr.Issue.Repo = pr.BaseRepo + + if pr.BaseRepo.IsFork { + return nil, nil + } + + repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) + if err != nil { + return nil, err + } + defer repo.Close() + + commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch) + if err != nil { + return nil, err + } + + var data string + for _, file := range codeOwnerFiles { + if blob, err := commit.GetBlobByPath(file); err == nil { + data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) + if err == nil { + break + } + } + } + + rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) + if len(rules) == 0 { + return nil, nil + } + + if startCommitID == "" && endCommitID == "" { + // get the mergebase + mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) + if err != nil { + return nil, err + } + startCommitID = mergeBase + endCommitID = pr.GetGitRefName() + } + + // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed + // between the merge base and the head commit but not the base branch and the head commit + changedFiles, err := repo.GetFilesChangedBetween(startCommitID, endCommitID) + if err != nil { + return nil, err + } + + uniqUsers := make(map[int64]*user_model.User) + uniqTeams := make(map[string]*org_model.Team) + for _, rule := range rules { + for _, f := range changedFiles { + if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) { + for _, u := range rule.Users { + uniqUsers[u.ID] = u + } + for _, t := range rule.Teams { + uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t + } + } + } + } + + notifiers := make([]*ReviewRequestNotifier, 0, len(uniqUsers)+len(uniqTeams)) + + if err := issue.LoadPoster(ctx); err != nil { + return nil, err + } + + for _, u := range uniqUsers { + if u.ID != issue.Poster.ID { + comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster) + if err != nil { + log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) + return nil, err + } + notifiers = append(notifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + Reviewer: u, + }) + } + } + for _, t := range uniqTeams { + comment, err := issues_model.AddTeamReviewRequest(ctx, issue, t, issue.Poster) + if err != nil { + log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) + return nil, err + } + notifiers = append(notifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + ReviewTeam: t, + }) + } + + return notifiers, nil +} + +// ReviewRequest add or remove a review request from a user for this PR, and make comment for it. +func ReviewRequest(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { + err = isValidReviewRequest(ctx, reviewer, doer, isAdd, pr.Issue, permDoer) + if err != nil { + return nil, err + } + + if isAdd { + comment, err = issues_model.AddReviewRequest(ctx, pr.Issue, reviewer, doer) + } else { + comment, err = issues_model.RemoveReviewRequest(ctx, pr.Issue, reviewer, doer) + } + + if err != nil { + return nil, err + } + + if comment != nil { + notify_service.PullRequestReviewRequest(ctx, doer, pr.Issue, reviewer, isAdd, comment) + } + + return comment, err +} + +func ReviewRequests(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, reviewers []*user_model.User, reviewTeams []*org_model.Team) (comments []*issues_model.Comment, err error) { + for _, reviewer := range reviewers { + comment, err := ReviewRequest(ctx, pr, doer, nil, reviewer, true) + if err != nil { + return nil, err + } + comments = append(comments, comment) + } + + for _, reviewTeam := range reviewTeams { + comment, err := TeamReviewRequest(ctx, pr, doer, reviewTeam, true) + if err != nil { + return nil, err + } + comments = append(comments, comment) + } + + return comments, nil +} + +// isValidReviewRequest Check permission for ReviewRequest +func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { + if reviewer.IsOrganization() { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Organization can't be added as reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + if doer.IsOrganization() { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Organization can't be doer to add reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + // reviewers can remove themself + if !isAdd && doer.ID == reviewer.ID { + return nil + } + + if err := issue.LoadRepo(ctx); err != nil { + return err + } + + permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer) + if err != nil { + return err + } + + if permDoer == nil { + permDoer = new(access_model.Permission) + *permDoer, err = access_model.GetUserRepoPermission(ctx, issue.Repo, doer) + if err != nil { + return err + } + } + + lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + if err != nil && !issues_model.IsErrReviewNotExist(err) { + return err + } + + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) + + if isAdd { + if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Reviewer can't read", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 { + return issues_model.ErrNotValidReviewRequest{ + Reason: "poster of pr can't be reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if canDoerChangeReviewRequests { + return nil + } + + if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastReview != nil && lastReview.Type != issues_model.ReviewTypeRequest { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't choose reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if canDoerChangeReviewRequests { + return nil + } + + if lastReview != nil && lastReview.Type == issues_model.ReviewTypeRequest && lastReview.ReviewerID == doer.ID { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't remove reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } +} + +// isValidTeamReviewRequest Check permission for ReviewRequest Team +func isValidTeamReviewRequest(ctx context.Context, reviewer *org_model.Team, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { + if doer.IsOrganization() { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Organization can't be doer to add reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) + + if isAdd { + if issue.Repo.IsPrivate { + hasTeam := org_model.HasTeamRepo(ctx, reviewer.OrgID, reviewer.ID, issue.RepoID) + + if !hasTeam { + return issues_model.ErrNotValidReviewRequest{ + Reason: "Reviewing team can't read repo", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + } + + if canDoerChangeReviewRequests { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't choose reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } + } + + if canDoerChangeReviewRequests { + return nil + } + + return issues_model.ErrNotValidReviewRequest{ + Reason: "Doer can't remove reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } +} + +// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. +func TeamReviewRequest(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, reviewer *org_model.Team, isAdd bool) (comment *issues_model.Comment, err error) { + err = isValidTeamReviewRequest(ctx, reviewer, doer, isAdd, pr.Issue) + if err != nil { + return nil, err + } + if isAdd { + comment, err = issues_model.AddTeamReviewRequest(ctx, pr.Issue, reviewer, doer) + } else { + comment, err = issues_model.RemoveTeamReviewRequest(ctx, pr.Issue, reviewer, doer) + } + + if err != nil { + return nil, err + } + + if comment == nil || !isAdd { + return nil, nil + } + + return comment, teamReviewRequestNotify(ctx, pr.Issue, doer, reviewer, isAdd, comment) +} + +func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { + for _, reviewNotifier := range reviewNotifiers { + if reviewNotifier.Reviewer != nil { + notify_service.PullRequestReviewRequest(ctx, issue.Poster, issue, reviewNotifier.Reviewer, reviewNotifier.IsAdd, reviewNotifier.Comment) + } else if reviewNotifier.ReviewTeam != nil { + if err := teamReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifier.ReviewTeam, reviewNotifier.IsAdd, reviewNotifier.Comment); err != nil { + log.Error("teamReviewRequestNotify: %v", err) + } + } + } +} + +// teamReviewRequestNotify notify all user in this team +func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *org_model.Team, isAdd bool, comment *issues_model.Comment) error { + // notify all user in this team + if err := comment.LoadIssue(ctx); err != nil { + return err + } + + members, err := org_model.GetTeamMembers(ctx, &org_model.SearchMembersOptions{ + TeamID: reviewer.ID, + }) + if err != nil { + return err + } + + for _, member := range members { + if member.ID == comment.Issue.PosterID { + continue + } + comment.AssigneeID = member.ID + notify_service.PullRequestReviewRequest(ctx, doer, issue, member, isAdd, comment) + } + + return err +} + +// CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR +func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool { + if repo.IsArchived { + return false + } + // The poster of the PR can change the reviewers + if doer.ID == posterID { + return true + } + + // The owner of the repo can change the reviewers + if doer.ID == repo.OwnerID { + return true + } + + // Collaborators of the repo can change the reviewers + isCollaborator, err := repo_model.IsCollaborator(ctx, repo.ID, doer.ID) + if err != nil { + log.Error("IsCollaborator: %v", err) + return false + } + if isCollaborator { + return true + } + + // If the repo's owner is an organization, members of teams with read permission on pull requests can change reviewers + if repo.Owner.IsOrganization() { + teams, err := org_model.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) + if err != nil { + log.Error("GetTeamsWithAccessToRepo: %v", err) + return false + } + for _, team := range teams { + if !team.UnitEnabled(ctx, unit.TypePullRequests) { + continue + } + isMember, err := org_model.IsTeamMember(ctx, repo.OwnerID, team.ID, doer.ID) + if err != nil { + log.Error("IsTeamMember: %v", err) + continue + } + if isMember { + return true + } + } + } + + return false +} diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 096f51dfc0575..838c8ecbef9ba 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -661,13 +661,14 @@ jobs: assert.NoError(t, err) checkCommitStatusAndInsertFakeStatus(t, repo, sha) + assert.NoError(t, pullIssue.LoadPullRequest(db.DefaultContext)) // review_requested - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user2, nil, user4, true) + _, err = pull_service.ReviewRequest(db.DefaultContext, pullIssue.PullRequest, user2, nil, user4, true) assert.NoError(t, err) checkCommitStatusAndInsertFakeStatus(t, repo, sha) // review_request_removed - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user2, nil, user4, false) + _, err = pull_service.ReviewRequest(db.DefaultContext, pullIssue.PullRequest, user2, nil, user4, false) assert.NoError(t, err) checkCommitStatusAndInsertFakeStatus(t, repo, sha) }) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index b85882a510bc0..b2cce3dd302a9 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -11,13 +11,15 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/json" api "code.gitea.io/gitea/modules/structs" - issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -246,6 +248,13 @@ func TestAPIPullReviewRequest(t *testing.T) { req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{ Reviewers: []string{"user4@example.com", "user8"}, }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusUnprocessableEntity) + + user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8}) + repo_service.AddOrUpdateCollaborator(db.DefaultContext, repo, user8, perm.AccessModeRead) + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user8"}, + }).AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) // poster of pr can't be reviewer @@ -258,7 +267,7 @@ func TestAPIPullReviewRequest(t *testing.T) { req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{ Reviewers: []string{"testOther"}, }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusNotFound) + MakeRequest(t, req, http.StatusUnprocessableEntity) // Test Remove Review Request session2 := loginUser(t, "user4") @@ -287,12 +296,12 @@ func TestAPIPullReviewRequest(t *testing.T) { user38Session := loginUser(t, "user38") user38Token := getTokenForLoggedInUser(t, user38Session, auth_model.AccessTokenScopeWriteRepository) req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ - Reviewers: []string{"user4@example.com"}, + Reviewers: []string{"user40@example.com"}, }).AddTokenAuth(user38Token) MakeRequest(t, req, http.StatusCreated) req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ - Reviewers: []string{"user4@example.com"}, + Reviewers: []string{"user40@example.com"}, }).AddTokenAuth(user38Token) MakeRequest(t, req, http.StatusNoContent) @@ -300,12 +309,12 @@ func TestAPIPullReviewRequest(t *testing.T) { user39Session := loginUser(t, "user39") user39Token := getTokenForLoggedInUser(t, user39Session, auth_model.AccessTokenScopeWriteRepository) req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ - Reviewers: []string{"user8"}, + Reviewers: []string{"user38"}, }).AddTokenAuth(user39Token) MakeRequest(t, req, http.StatusCreated) req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull21Repo.OwnerName, pull21Repo.Name, pullIssue21.Index), &api.PullReviewRequestOptions{ - Reviewers: []string{"user8"}, + Reviewers: []string{"user38"}, }).AddTokenAuth(user39Token) MakeRequest(t, req, http.StatusNoContent) @@ -323,6 +332,12 @@ func TestAPIPullReviewRequest(t *testing.T) { }).AddTokenAuth(user39Token) // user39 is from a team with read permission on pull requests unit MakeRequest(t, req, http.StatusNoContent) + // user8 is not a reviewer, so this will return 422 + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", pull22Repo.OwnerName, pull22Repo.Name, pullIssue22.Index), &api.PullReviewRequestOptions{ + Reviewers: []string{"user8"}, + }).AddTokenAuth(user39Token) // user39 is from a team with read permission on pull requests unit + MakeRequest(t, req, http.StatusUnprocessableEntity) + // Test team review request pullIssue12 := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 12}) assert.NoError(t, pullIssue12.LoadAttributes(db.DefaultContext)) @@ -330,7 +345,7 @@ func TestAPIPullReviewRequest(t *testing.T) { // Test add Team Review Request req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo3.OwnerName, repo3.Name, pullIssue12.Index), &api.PullReviewRequestOptions{ - TeamReviewers: []string{"team1", "owners"}, + TeamReviewers: []string{"team1", "Owners"}, }).AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) @@ -344,7 +359,7 @@ func TestAPIPullReviewRequest(t *testing.T) { req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo3.OwnerName, repo3.Name, pullIssue12.Index), &api.PullReviewRequestOptions{ TeamReviewers: []string{"not_exist_team"}, }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusNotFound) + MakeRequest(t, req, http.StatusUnprocessableEntity) // Test Remove team Review Request req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo3.OwnerName, repo3.Name, pullIssue12.Index), &api.PullReviewRequestOptions{ @@ -376,6 +391,9 @@ func TestAPIPullReviewStayDismissed(t *testing.T) { session8 := loginUser(t, user8.LoginName) token8 := getTokenForLoggedInUser(t, session8, auth_model.AccessTokenScopeWriteRepository) + // add user8 as collaborator of repo 1 otherwise he can't be as reviewer + assert.NoError(t, repo_service.AddOrUpdateCollaborator(db.DefaultContext, repo, user8, perm.AccessModeRead)) + // user2 request user8 req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{ Reviewers: []string{user8.LoginName}, @@ -425,7 +443,8 @@ func TestAPIPullReviewStayDismissed(t *testing.T) { // user8 dismiss review permUser8, err := access_model.GetUserRepoPermission(db.DefaultContext, pullIssue.Repo, user8) assert.NoError(t, err) - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user8, &permUser8, user8, false) + assert.NoError(t, pullIssue.LoadPullRequest(db.DefaultContext)) + _, err = pull_service.ReviewRequest(db.DefaultContext, pullIssue.PullRequest, user8, &permUser8, user8, false) assert.NoError(t, err) reviewsCountCheck(t,