From e96f5d1f92d30033db87a94c86990d75ef2a3335 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 13:10:41 -0800 Subject: [PATCH 01/10] Fix review request API --- routers/api/v1/repo/pull_review.go | 78 ++++++++++++++++++++++++++++-- routers/web/repo/pull_review.go | 8 ++- services/issue/assignee.go | 41 ++++++++++++---- services/pull/pull.go | 4 +- 4 files changed, 114 insertions(+), 17 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 6d7a326370c08..2388f60fd9800 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -611,7 +611,79 @@ func CreateReviewRequests(ctx *context.APIContext) { // "$ref": "#/responses/notFound" opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) - apiReviewRequest(ctx, *opts, true) + + pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64(":index")) + if err != nil { + if issues_model.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + allowedUsers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, pr.Issue.PosterID) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetReviewers", err) + return + } + filteredUsers := make([]*user_model.User, 0, len(opts.Reviewers)) + for _, reviewer := range opts.Reviewers { + for _, allowedUser := range allowedUsers { + if allowedUser.Name == reviewer || allowedUser.Email == reviewer { + filteredUsers = append(filteredUsers, allowedUser) + break + } + } + } + + 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.Error(http.StatusInternalServerError, "GetReviewers", err) + return + } + for _, teamReviewer := range opts.TeamReviewers { + for _, allowedTeam := range allowedTeams { + if allowedTeam.Name == teamReviewer { + filteredTeams = append(filteredTeams, allowedTeam) + break + } + } + } + } + comments, err := issue_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams) + if err != nil { + if issues_model.IsErrReviewRequestOnClosedPR(err) { + ctx.Error(http.StatusForbidden, "", err) + return + } + if issues_model.IsErrNotValidReviewRequest(err) { + ctx.Error(http.StatusUnprocessableEntity, "", err) + return + } + ctx.Error(http.StatusInternalServerError, "ReviewRequests", 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.ServerError("ReviewRequest", err) + return + } + reviews = append(reviews, comment.Review) + } + } + + apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReviewList", err) + return + } + ctx.JSON(http.StatusCreated, apiReviews) } // DeleteReviewRequests delete review requests to an pull request @@ -730,7 +802,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions } for _, reviewer := range reviewers { - comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd) + comment, err := issue_service.ReviewRequest(ctx, pr, ctx.Doer, &permDoer, reviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -755,7 +827,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions 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) + comment, err := issue_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 3e9e615b15dab..1123388c39d5e 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -365,6 +365,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 +399,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") + _, err = issue_service.TeamReviewRequest(ctx, issue.PullRequest, ctx.Doer, team, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -427,7 +431,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") + _, err = issue_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/services/issue/assignee.go b/services/issue/assignee.go index c7e24955687f9..f408cde92f73d 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -8,6 +8,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" + 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" @@ -61,16 +62,16 @@ func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, do } // 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) +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, issue, reviewer, doer) + comment, err = issues_model.AddReviewRequest(ctx, pr.Issue, reviewer, doer) } else { - comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) + comment, err = issues_model.RemoveReviewRequest(ctx, pr.Issue, reviewer, doer) } if err != nil { @@ -78,12 +79,32 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_mo } if comment != nil { - notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewer, isAdd, comment) + 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() { @@ -216,15 +237,15 @@ func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, } // 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) +func TeamReviewRequest(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, reviewer *organization.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, issue, reviewer, doer) + comment, err = issues_model.AddTeamReviewRequest(ctx, pr.Issue, reviewer, doer) } else { - comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) + comment, err = issues_model.RemoveTeamReviewRequest(ctx, pr.Issue, reviewer, doer) } if err != nil { @@ -235,7 +256,7 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use return nil, nil } - return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) + 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) { diff --git a/services/pull/pull.go b/services/pull/pull.go index 5d3758eca6d14..1cb8df580cd7c 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -213,12 +213,12 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } 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 { + if _, err = issue_service.ReviewRequest(ctx, pr, 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 { + if _, err = issue_service.TeamReviewRequest(ctx, pr, issue.Poster, teamReviewer, true); err != nil { return err } } From b5c24c56119cdaad316533d62a0bd4310a6c6df9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 20:40:29 -0800 Subject: [PATCH 02/10] Move review requests functions from issue service to pull service --- routers/api/v1/repo/collaborators.go | 3 +- routers/api/v1/repo/pull_review.go | 7 +- routers/web/repo/issue_page_meta.go | 3 +- routers/web/repo/pull_review.go | 5 +- services/issue/assignee.go | 292 --------------- services/issue/issue.go | 11 - services/issue/pull.go | 147 -------- services/pull/notify.go | 36 ++ services/pull/pull.go | 10 +- services/pull/review_request.go | 437 ++++++++++++++++++++++ tests/integration/api_pull_review_test.go | 5 +- 11 files changed, 488 insertions(+), 468 deletions(-) delete mode 100644 services/issue/pull.go create mode 100644 services/pull/notify.go create mode 100644 services/pull/review_request.go diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index da3ee54e691de..2e49eb0b4078b 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -17,7 +17,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" ) @@ -322,7 +321,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.Error(http.StatusForbidden, "GetReviewers", errors.New("doer has no permission to get reviewers")) return diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 2388f60fd9800..299a90abd4ff7 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" ) @@ -653,7 +652,7 @@ func CreateReviewRequests(ctx *context.APIContext) { } } } - comments, err := issue_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams) + comments, err := pull_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -802,7 +801,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions } for _, reviewer := range reviewers { - comment, err := issue_service.ReviewRequest(ctx, pr, ctx.Doer, &permDoer, reviewer, isAdd) + comment, err := pull_service.ReviewRequest(ctx, pr, ctx.Doer, &permDoer, reviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -827,7 +826,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { for _, teamReviewer := range teamReviewers { - comment, err := issue_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, isAdd) + comment, err := pull_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index 272343f460ff3..a3a4d8b5e6058 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" ) @@ -186,7 +185,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 1123388c39d5e..852674bacaa60 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" ) @@ -399,7 +398,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.TeamReviewRequest(ctx, issue.PullRequest, ctx.Doer, team, action == "attach") + _, err = pull_service.TeamReviewRequest(ctx, issue.PullRequest, ctx.Doer, team, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -431,7 +430,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.ReviewRequest(ctx, issue.PullRequest, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") + _, 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/services/issue/assignee.go b/services/issue/assignee.go index f408cde92f73d..98c3c8c873c7b 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -7,14 +7,7 @@ import ( "context" issues_model "code.gitea.io/gitea/models/issues" - "code.gitea.io/gitea/models/organization" - 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/log" notify_service "code.gitea.io/gitea/services/notify" ) @@ -60,288 +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, 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, - } - } - - 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, pr *issues_model.PullRequest, doer *user_model.User, reviewer *organization.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 *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 091b7c02d7751..a0e652f84de56 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,17 +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) { - var err error - reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue, 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 896802108d6ca..0000000000000 --- a/services/issue/pull.go +++ /dev/null @@ -1,147 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package issue - -import ( - "context" - "fmt" - "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 -} - -func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { - files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - - 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 - } - - 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 files { - 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) - - // get the mergebase - mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) - if err != nil { - return nil, err - } - - // 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(mergeBase, pr.GetGitRefName()) - 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/notify.go b/services/pull/notify.go new file mode 100644 index 0000000000000..bcece2bef22d7 --- /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, 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 1cb8df580cd7c..b2693e3bbecc7 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -116,7 +116,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 @@ -176,7 +176,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, issue, pr) + reviewNotifiers, err = RequestCodeOwnersReview(ctx, issue, pr) if err != nil { return err } @@ -191,7 +191,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 { @@ -213,12 +213,12 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) for _, reviewer := range opts.Reviewers { - if _, err = issue_service.ReviewRequest(ctx, pr, issue.Poster, &permDoer, reviewer, true); err != nil { + if _, err = ReviewRequest(ctx, pr, issue.Poster, &permDoer, reviewer, true); err != nil { return err } } for _, teamReviewer := range opts.TeamReviewers { - if _, err = issue_service.TeamReviewRequest(ctx, pr, issue.Poster, teamReviewer, true); err != nil { + if _, err = TeamReviewRequest(ctx, pr, issue.Poster, teamReviewer, true); err != nil { return err } } diff --git a/services/pull/review_request.go b/services/pull/review_request.go new file mode 100644 index 0000000000000..8ca74aad2c288 --- /dev/null +++ b/services/pull/review_request.go @@ -0,0 +1,437 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + "fmt" + "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 +} + +func RequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { + files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} + + 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 + } + + 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 files { + 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) + + // get the mergebase + mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) + if err != nil { + return nil, err + } + + // 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(mergeBase, pr.GetGitRefName()) + 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, + } + } + + 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/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index b85882a510bc0..114c019f7a27d 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -17,7 +17,7 @@ import ( 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" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -425,7 +425,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, From a9cd81d94e9b4fe8854af17aaf7862fa85475c19 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 20:45:56 -0800 Subject: [PATCH 03/10] some improvements --- services/pull/check.go | 3 ++- services/pull/notify.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/services/pull/check.go b/services/pull/check.go index e1adc3ca3bfa2..ab96d78b31e38 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 index bcece2bef22d7..92c18f2bebe05 100644 --- a/services/pull/notify.go +++ b/services/pull/notify.go @@ -18,8 +18,8 @@ type pullNotifier struct { var _ notify_service.Notifier = &pullNotifier{} -// NewNotifier create a new indexerNotifier notifier -func NewNotifier() notify_service.Notifier { +// newNotifier create a new indexerNotifier notifier +func newNotifier() notify_service.Notifier { return &pullNotifier{} } From 6a76e2d0934719fc7841eada360caf908c1e1fac Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 21:11:24 -0800 Subject: [PATCH 04/10] some improvements --- routers/api/v1/repo/pull_review.go | 41 ++++-------------------------- 1 file changed, 5 insertions(+), 36 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 299a90abd4ff7..c4bd97bf2215d 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -724,7 +724,7 @@ 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) { @@ -768,7 +768,7 @@ func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerN return reviewers, teamReviewers } -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) { @@ -795,13 +795,8 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions return } - var reviews []*issues_model.Review - if isAdd { - reviews = make([]*issues_model.Review, 0, len(reviewers)) - } - for _, reviewer := range reviewers { - comment, err := pull_service.ReviewRequest(ctx, pr, 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.Error(http.StatusForbidden, "", err) @@ -814,19 +809,11 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) return } - - if comment != nil && isAdd { - if err = comment.LoadReview(ctx); err != nil { - ctx.ServerError("ReviewRequest", err) - return - } - reviews = append(reviews, comment.Review) - } } if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { for _, teamReviewer := range teamReviewers { - comment, err := pull_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, isAdd) + _, err := pull_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, false) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -839,28 +826,10 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions ctx.ServerError("TeamReviewRequest", err) return } - - if comment != nil && isAdd { - if err = comment.LoadReview(ctx); err != nil { - ctx.ServerError("ReviewRequest", err) - return - } - reviews = append(reviews, comment.Review) - } } } - if isAdd { - apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer) - if err != nil { - ctx.Error(http.StatusInternalServerError, "convertToPullReviewList", 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 From c2de2fdcdbc81e6db6ce024d9a40ca201b2c9d48 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 21:36:47 -0800 Subject: [PATCH 05/10] Add request review check and move to database transaction --- models/issues/review.go | 4 ++- services/pull/pull.go | 79 ++++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 3e787273be8e1..103f80eccf5a6 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) } diff --git a/services/pull/pull.go b/services/pull/pull.go index b2693e3bbecc7..15f3284d29d2f 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 { @@ -175,7 +215,32 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } - if !pr.IsWorkInProgress(ctx) { + // 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, issue, pr) if err != nil { return err @@ -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 = ReviewRequest(ctx, pr, issue.Poster, &permDoer, reviewer, true); err != nil { - return err - } - } - for _, teamReviewer := range opts.TeamReviewers { - if _, err = TeamReviewRequest(ctx, pr, issue.Poster, teamReviewer, true); err != nil { - return err - } - } + return nil } From 98de3a4b78efff8d2caeb4071d7017461d75bc40 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 21:48:51 -0800 Subject: [PATCH 06/10] Add todo for checking permissions --- routers/web/repo/pull_review.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 852674bacaa60..79f88a6473a4e 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -398,6 +398,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } + // 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) { @@ -430,6 +431,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } + // 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) { From 50b28dcb4c103cf25c8377e024609da3a838237a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 26 Nov 2024 17:16:35 -0800 Subject: [PATCH 07/10] Some improvements --- models/issues/review.go | 2 +- routers/api/v1/repo/pull.go | 13 ++++++- routers/api/v1/repo/pull_review.go | 47 +++++++++++++---------- services/pull/review_request.go | 4 ++ tests/integration/api_pull_review_test.go | 11 +++++- 5 files changed, 53 insertions(+), 24 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 103f80eccf5a6..d6dd3aea32add 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -742,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/pull.go b/routers/api/v1/repo/pull.go index f7fdc93f81db6..3028ac7d556fd 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" @@ -536,8 +537,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.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name)) + return + case organization.IsErrTeamNotExist(err): + ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name)) + return + case err != nil: + ctx.Error(http.StatusInternalServerError, "GetUser", err) return } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index c4bd97bf2215d..7377913ea20ac 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -628,12 +628,18 @@ func CreateReviewRequests(ctx *context.APIContext) { } 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.Error(http.StatusUnprocessableEntity, "", "") + return + } } filteredTeams := make([]*organization.Team, 0, len(opts.TeamReviewers)) @@ -644,12 +650,18 @@ func CreateReviewRequests(ctx *context.APIContext) { 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.Error(http.StatusUnprocessableEntity, "", "") + return + } } } comments, err := pull_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams) @@ -727,8 +739,7 @@ func DeleteReviewRequests(ctx *context.APIContext) { 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, "@") { @@ -736,14 +747,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.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r)) - return nil, nil - } - ctx.Error(http.StatusInternalServerError, "GetUser", err) - return nil, nil + return nil, nil, err } reviewers = append(reviewers, reviewer) @@ -751,21 +756,15 @@ 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.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t)) - return nil, nil - } - ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) - return nil, nil + return nil, nil, err } teamReviewers = append(teamReviewers, teamReviewer) } } - return reviewers, teamReviewers + return reviewers, teamReviewers, nil } func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOptions) { @@ -790,8 +789,16 @@ func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOpt 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.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name)) + return + case organization.IsErrTeamNotExist(err): + ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name)) + return + case err != nil: + ctx.Error(http.StatusInternalServerError, "GetUser", err) return } diff --git a/services/pull/review_request.go b/services/pull/review_request.go index 8ca74aad2c288..49d262f847b15 100644 --- a/services/pull/review_request.go +++ b/services/pull/review_request.go @@ -212,6 +212,10 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } + if err := issue.LoadRepo(ctx); err != nil { + return err + } + permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer) if err != nil { return err diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 114c019f7a27d..eea35e44861e4 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -11,6 +11,7 @@ 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" @@ -18,6 +19,7 @@ import ( "code.gitea.io/gitea/modules/json" api "code.gitea.io/gitea/modules/structs" 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") From b0b40680b57e0ab9c3ad6c6b44eb23654c169b81 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 24 Jan 2025 14:38:17 -0800 Subject: [PATCH 08/10] Fix tests --- models/issues/pull.go | 1 + routers/api/v1/repo/pull_review.go | 5 ++++- services/pull/review_request.go | 5 +++++ tests/integration/actions_trigger_test.go | 5 +++-- tests/integration/api_pull_review_test.go | 18 ++++++++++++------ 5 files changed, 25 insertions(+), 9 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index e3af00224dedb..67e1ff1217b57 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -353,6 +353,7 @@ func (pr *PullRequest) LoadIssue(ctx context.Context) (err error) { pr.Issue, err = GetIssueByID(ctx, pr.IssueID) if err == nil { pr.Issue.PullRequest = pr + pr.Issue.Repo = pr.BaseRepo } return err } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 7377913ea20ac..a30db396e1252 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -611,7 +611,8 @@ func CreateReviewRequests(ctx *context.APIContext) { opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) - pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64(":index")) + // 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.NotFound("GetPullRequestByIndex", err) @@ -621,6 +622,8 @@ func CreateReviewRequests(ctx *context.APIContext) { 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.Error(http.StatusInternalServerError, "GetReviewers", err) diff --git a/services/pull/review_request.go b/services/pull/review_request.go index 49d262f847b15..216354bfc3898 100644 --- a/services/pull/review_request.go +++ b/services/pull/review_request.go @@ -212,6 +212,11 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } + // reviewers can remove themself + if !isAdd && doer.ID == reviewer.ID { + return nil + } + if err := issue.LoadRepo(ctx); err != nil { return err } diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 2c76aa826f2ce..0bcbe21b8c860 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -622,13 +622,14 @@ func TestPullRequestCommitStatusEvent(t *testing.T) { 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 eea35e44861e4..c1a5e21389def 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -296,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) @@ -309,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) @@ -332,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)) @@ -339,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) @@ -353,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{ From dd0ebc6f06af06b4cddeb4bffec0fb0f5d65028d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 24 Jan 2025 15:46:00 -0800 Subject: [PATCH 09/10] Revert unnecessary change --- models/issues/pull.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 67e1ff1217b57..e3af00224dedb 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -353,7 +353,6 @@ func (pr *PullRequest) LoadIssue(ctx context.Context) (err error) { pr.Issue, err = GetIssueByID(ctx, pr.IssueID) if err == nil { pr.Issue.PullRequest = pr - pr.Issue.Repo = pr.BaseRepo } return err } From 9d6588ed4f9bc4874360d2dd8c1d0f135fd1a7c4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Jan 2025 18:57:09 -0800 Subject: [PATCH 10/10] Fix test --- tests/integration/api_pull_review_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index c1a5e21389def..b2cce3dd302a9 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -391,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},