From e693713b38539e03719b70ff2ff83f6a1a6cb728 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Tue, 2 May 2023 16:27:38 +0800 Subject: [PATCH 01/22] feat: new webhook for receiving Pull Request review requests --- models/issues/assignees.go | 21 +++++++++++ models/issues/issue.go | 64 +++++++++++++++++++--------------- models/webhook/webhook.go | 7 ++++ models/webhook/webhook_test.go | 2 +- modules/structs/hook.go | 21 ++++++----- modules/structs/issue.go | 5 +-- modules/structs/pull.go | 27 +++++++------- modules/webhook/structs.go | 1 + modules/webhook/type.go | 3 +- routers/api/v1/utils/hook.go | 2 ++ routers/web/repo/webhook.go | 1 + services/convert/issue.go | 8 +++++ services/convert/pull.go | 45 ++++++++++++------------ services/forms/repo_form.go | 1 + services/webhook/notifier.go | 28 +++++++++++++++ 15 files changed, 160 insertions(+), 76 deletions(-) diff --git a/models/issues/assignees.go b/models/issues/assignees.go index fdd0d6f2274b6..ec8366e3c2e63 100644 --- a/models/issues/assignees.go +++ b/models/issues/assignees.go @@ -44,6 +44,27 @@ func (issue *Issue) LoadAssignees(ctx context.Context) (err error) { return err } +// LoadRequestedReviewers load requestedReviewers of this issue. +func (issue *Issue) LoadRequestedReviewers(ctx context.Context) (err error) { + // Reset maybe preexisting reviewers + issue.RequestedReviewers = []*user_model.User{} + issue.RequestedReviewer = nil + + err = db.GetEngine(ctx).Table("`user`"). + Join("INNER", "review", "reviewer_id = `user`.id"). + Where("review.issue_id = ?", issue.ID). + Find(&issue.RequestedReviewers) + if err != nil { + return err + } + + // Check if we have at least one reviewer and if yes put it in as `RequestedReviewer` + if len(issue.RequestedReviewers) > 0 { + issue.RequestedReviewer = issue.RequestedReviewers[0] + } + return err +} + // GetAssigneeIDsByIssue returns the IDs of users assigned to an issue // but skips joining with `user` for performance reasons. // User permissions must be verified elsewhere if required. diff --git a/models/issues/issue.go b/models/issues/issue.go index 8c173433f2546..68377856341c2 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -103,30 +103,31 @@ func (err ErrIssueWasClosed) Error() string { // Issue represents an issue or pull request of repository. type Issue struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"` - Repo *repo_model.Repository `xorm:"-"` - Index int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository. - PosterID int64 `xorm:"INDEX"` - Poster *user_model.User `xorm:"-"` - OriginalAuthor string - OriginalAuthorID int64 `xorm:"index"` - Title string `xorm:"name"` - Content string `xorm:"LONGTEXT"` - RenderedContent string `xorm:"-"` - Labels []*Label `xorm:"-"` - MilestoneID int64 `xorm:"INDEX"` - Milestone *Milestone `xorm:"-"` - Project *project_model.Project `xorm:"-"` - Priority int - AssigneeID int64 `xorm:"-"` - Assignee *user_model.User `xorm:"-"` - IsClosed bool `xorm:"INDEX"` - IsRead bool `xorm:"-"` - IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not. - PullRequest *PullRequest `xorm:"-"` - NumComments int - Ref string + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"` + Repo *repo_model.Repository `xorm:"-"` + Index int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository. + PosterID int64 `xorm:"INDEX"` + Poster *user_model.User `xorm:"-"` + OriginalAuthor string + OriginalAuthorID int64 `xorm:"index"` + Title string `xorm:"name"` + Content string `xorm:"LONGTEXT"` + RenderedContent string `xorm:"-"` + Labels []*Label `xorm:"-"` + MilestoneID int64 `xorm:"INDEX"` + Milestone *Milestone `xorm:"-"` + Project *project_model.Project `xorm:"-"` + Priority int + AssigneeID int64 `xorm:"-"` + Assignee *user_model.User `xorm:"-"` + RequestedReviewer *user_model.User `xorm:"-"` + IsClosed bool `xorm:"INDEX"` + IsRead bool `xorm:"-"` + IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not. + PullRequest *PullRequest `xorm:"-"` + NumComments int + Ref string DeadlineUnix timeutil.TimeStamp `xorm:"INDEX"` @@ -134,11 +135,12 @@ type Issue struct { UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` ClosedUnix timeutil.TimeStamp `xorm:"INDEX"` - Attachments []*repo_model.Attachment `xorm:"-"` - Comments []*Comment `xorm:"-"` - Reactions ReactionList `xorm:"-"` - TotalTrackedTime int64 `xorm:"-"` - Assignees []*user_model.User `xorm:"-"` + Attachments []*repo_model.Attachment `xorm:"-"` + Comments []*Comment `xorm:"-"` + Reactions ReactionList `xorm:"-"` + TotalTrackedTime int64 `xorm:"-"` + Assignees []*user_model.User `xorm:"-"` + RequestedReviewers []*user_model.User `xorm:"-"` // IsLocked limits commenting abilities to users on an issue // with write access @@ -359,6 +361,10 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return } + if err = issue.LoadRequestedReviewers(ctx); err != nil { + return + } + if err = issue.LoadPullRequest(ctx); err != nil && !IsErrPullRequestNotExist(err) { // It is possible pull request is not yet created. return err diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index e3f6b593d977a..875d5ddb09397 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -298,6 +298,12 @@ func (w *Webhook) HasPackageEvent() bool { (w.ChooseEvents && w.HookEvents.Package) } +// HasPullReviewRequestEvent returns true if hook enabled pull request review request event. +func (w *Webhook) HasPullReviewRequestEvent() bool { + return w.SendEverything || + (w.ChooseEvents && w.HookEvents.PullReviewRequest) +} + // EventCheckers returns event checkers func (w *Webhook) EventCheckers() []struct { Has func() bool @@ -329,6 +335,7 @@ func (w *Webhook) EventCheckers() []struct { {w.HasRepositoryEvent, webhook_module.HookEventRepository}, {w.HasReleaseEvent, webhook_module.HookEventRelease}, {w.HasPackageEvent, webhook_module.HookEventPackage}, + {w.HasPullReviewRequestEvent, webhook_module.HookEventPullReviewRequest}, } } diff --git a/models/webhook/webhook_test.go b/models/webhook/webhook_test.go index 74f7aeaa03028..070af821fac90 100644 --- a/models/webhook/webhook_test.go +++ b/models/webhook/webhook_test.go @@ -72,7 +72,7 @@ func TestWebhook_EventsArray(t *testing.T) { "pull_request", "pull_request_assign", "pull_request_label", "pull_request_milestone", "pull_request_comment", "pull_request_review_approved", "pull_request_review_rejected", "pull_request_review_comment", "pull_request_sync", "wiki", "repository", "release", - "package", + "package", "pull_review_request", }, (&Webhook{ HookEvent: &webhook_module.HookEvent{SendEverything: true}, diff --git a/modules/structs/hook.go b/modules/structs/hook.go index df5da6790f835..cd91d4bc462a4 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -342,6 +342,10 @@ const ( HookIssueDemilestoned HookIssueAction = "demilestoned" // HookIssueReviewed is an issue action for when a pull request is reviewed HookIssueReviewed HookIssueAction = "reviewed" + // HookIssueReviewRequested is an issue action for when a reviewer is requested for a pull request. + HookIssueReviewRequested HookIssueAction = "review_requested" + // HookIssueReviewRequestRemoved is an issue action for removing a review request to someone on a pull request. + HookIssueReviewRequestRemoved HookIssueAction = "review_request_removed" ) // IssuePayload represents the payload information that is sent along with an issue event. @@ -381,14 +385,15 @@ type ChangesPayload struct { // PullRequestPayload represents a payload information of pull request event. type PullRequestPayload struct { - Action HookIssueAction `json:"action"` - Index int64 `json:"number"` - Changes *ChangesPayload `json:"changes,omitempty"` - PullRequest *PullRequest `json:"pull_request"` - Repository *Repository `json:"repository"` - Sender *User `json:"sender"` - CommitID string `json:"commit_id"` - Review *ReviewPayload `json:"review"` + Action HookIssueAction `json:"action"` + Index int64 `json:"number"` + Changes *ChangesPayload `json:"changes,omitempty"` + PullRequest *PullRequest `json:"pull_request"` + RequestedReviewer *User `json:"requested_reviewer"` + Repository *Repository `json:"repository"` + Sender *User `json:"sender"` + CommitID string `json:"commit_id"` + Review *ReviewPayload `json:"review"` } // JSONPayload FIXME diff --git a/modules/structs/issue.go b/modules/structs/issue.go index 04e169df84197..8a059a15df3ad 100644 --- a/modules/structs/issue.go +++ b/modules/structs/issue.go @@ -55,8 +55,9 @@ type Issue struct { Labels []*Label `json:"labels"` Milestone *Milestone `json:"milestone"` // deprecated - Assignee *User `json:"assignee"` - Assignees []*User `json:"assignees"` + Assignee *User `json:"assignee"` + Assignees []*User `json:"assignees"` + RequestedReviewers []*User `json:"requested_reviewers"` // Whether the issue is open or closed // // type: string diff --git a/modules/structs/pull.go b/modules/structs/pull.go index f64bb83af9bac..a4a6f60b05684 100644 --- a/modules/structs/pull.go +++ b/modules/structs/pull.go @@ -9,19 +9,20 @@ import ( // PullRequest represents a pull request type PullRequest struct { - ID int64 `json:"id"` - URL string `json:"url"` - Index int64 `json:"number"` - Poster *User `json:"user"` - Title string `json:"title"` - Body string `json:"body"` - Labels []*Label `json:"labels"` - Milestone *Milestone `json:"milestone"` - Assignee *User `json:"assignee"` - Assignees []*User `json:"assignees"` - State StateType `json:"state"` - IsLocked bool `json:"is_locked"` - Comments int `json:"comments"` + ID int64 `json:"id"` + URL string `json:"url"` + Index int64 `json:"number"` + Poster *User `json:"user"` + Title string `json:"title"` + Body string `json:"body"` + Labels []*Label `json:"labels"` + Milestone *Milestone `json:"milestone"` + Assignee *User `json:"assignee"` + Assignees []*User `json:"assignees"` + RequestedReviewers []*User `json:"requested_reviewers"` + State StateType `json:"state"` + IsLocked bool `json:"is_locked"` + Comments int `json:"comments"` HTMLURL string `json:"html_url"` DiffURL string `json:"diff_url"` diff --git a/modules/webhook/structs.go b/modules/webhook/structs.go index 96012de352002..6030940c52e6a 100644 --- a/modules/webhook/structs.go +++ b/modules/webhook/structs.go @@ -21,6 +21,7 @@ type HookEvents struct { PullRequestComment bool `json:"pull_request_comment"` PullRequestReview bool `json:"pull_request_review"` PullRequestSync bool `json:"pull_request_sync"` + PullReviewRequest bool `json:"pull_review_request"` Wiki bool `json:"wiki"` Repository bool `json:"repository"` Release bool `json:"release"` diff --git a/modules/webhook/type.go b/modules/webhook/type.go index db4ab17931e9c..9a72de8eb4307 100644 --- a/modules/webhook/type.go +++ b/modules/webhook/type.go @@ -26,6 +26,7 @@ const ( HookEventPullRequestReviewRejected HookEventType = "pull_request_review_rejected" HookEventPullRequestReviewComment HookEventType = "pull_request_review_comment" HookEventPullRequestSync HookEventType = "pull_request_sync" + HookEventPullReviewRequest HookEventType = "pull_review_request" HookEventWiki HookEventType = "wiki" HookEventRepository HookEventType = "repository" HookEventRelease HookEventType = "release" @@ -46,7 +47,7 @@ func (h HookEventType) Event() string { case HookEventIssues, HookEventIssueAssign, HookEventIssueLabel, HookEventIssueMilestone: return "issues" case HookEventPullRequest, HookEventPullRequestAssign, HookEventPullRequestLabel, HookEventPullRequestMilestone, - HookEventPullRequestSync: + HookEventPullRequestSync, HookEventPullReviewRequest: return "pull_request" case HookEventIssueComment, HookEventPullRequestComment: return "issue_comment" diff --git a/routers/api/v1/utils/hook.go b/routers/api/v1/utils/hook.go index 44625cc9b81b8..268db2903c4e7 100644 --- a/routers/api/v1/utils/hook.go +++ b/routers/api/v1/utils/hook.go @@ -194,6 +194,7 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI PullRequestMilestone: pullHook(form.Events, string(webhook_module.HookEventPullRequestMilestone)), PullRequestComment: pullHook(form.Events, string(webhook_module.HookEventPullRequestComment)), PullRequestReview: pullHook(form.Events, "pull_request_review"), + PullReviewRequest: pullHook(form.Events, string(webhook_module.HookEventPullReviewRequest)), PullRequestSync: pullHook(form.Events, string(webhook_module.HookEventPullRequestSync)), Wiki: util.SliceContainsString(form.Events, string(webhook_module.HookEventWiki), true), Repository: util.SliceContainsString(form.Events, string(webhook_module.HookEventRepository), true), @@ -379,6 +380,7 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh w.PullRequestMilestone = pullHook(form.Events, string(webhook_module.HookEventPullRequestMilestone)) w.PullRequestComment = pullHook(form.Events, string(webhook_module.HookEventPullRequestComment)) w.PullRequestReview = pullHook(form.Events, "pull_request_review") + w.PullReviewRequest = pullHook(form.Events, string(webhook_module.HookEventPullReviewRequest)) w.PullRequestSync = pullHook(form.Events, string(webhook_module.HookEventPullRequestSync)) if err := w.UpdateEvent(); err != nil { diff --git a/routers/web/repo/webhook.go b/routers/web/repo/webhook.go index a21b405c9676e..b746e1e87a22d 100644 --- a/routers/web/repo/webhook.go +++ b/routers/web/repo/webhook.go @@ -177,6 +177,7 @@ func ParseHookEvent(form forms.WebhookForm) *webhook_module.HookEvent { PullRequestComment: form.PullRequestComment, PullRequestReview: form.PullRequestReview, PullRequestSync: form.PullRequestSync, + PullReviewRequest: form.PullReviewRequest, Wiki: form.Wiki, Repository: form.Repository, Package: form.Package, diff --git a/services/convert/issue.go b/services/convert/issue.go index 6d31a123bd9fc..dfa18ab2dde38 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -83,6 +83,14 @@ func ToAPIIssue(ctx context.Context, issue *issues_model.Issue) *api.Issue { } apiIssue.Assignee = ToUser(ctx, issue.Assignees[0], nil) // For compatibility, we're keeping the first assignee as `apiIssue.Assignee` } + if err := issue.LoadRequestedReviewers(ctx); err != nil { + return &api.Issue{} + } + if len(issue.RequestedReviewers) > 0 { + for _, reviewers := range issue.RequestedReviewers { + apiIssue.RequestedReviewers = append(apiIssue.RequestedReviewers, ToUser(ctx, reviewers, nil)) + } + } if issue.IsPull { if err := issue.LoadPullRequest(ctx); err != nil { return &api.Issue{} diff --git a/services/convert/pull.go b/services/convert/pull.go index 4989e82cd46b9..bac7033aecc39 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -50,28 +50,29 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u } apiPullRequest := &api.PullRequest{ - ID: pr.ID, - URL: pr.Issue.HTMLURL(), - Index: pr.Index, - Poster: apiIssue.Poster, - Title: apiIssue.Title, - Body: apiIssue.Body, - Labels: apiIssue.Labels, - Milestone: apiIssue.Milestone, - Assignee: apiIssue.Assignee, - Assignees: apiIssue.Assignees, - State: apiIssue.State, - IsLocked: apiIssue.IsLocked, - Comments: apiIssue.Comments, - HTMLURL: pr.Issue.HTMLURL(), - DiffURL: pr.Issue.DiffURL(), - PatchURL: pr.Issue.PatchURL(), - HasMerged: pr.HasMerged, - MergeBase: pr.MergeBase, - Mergeable: pr.Mergeable(), - Deadline: apiIssue.Deadline, - Created: pr.Issue.CreatedUnix.AsTimePtr(), - Updated: pr.Issue.UpdatedUnix.AsTimePtr(), + ID: pr.ID, + URL: pr.Issue.HTMLURL(), + Index: pr.Index, + Poster: apiIssue.Poster, + Title: apiIssue.Title, + Body: apiIssue.Body, + Labels: apiIssue.Labels, + Milestone: apiIssue.Milestone, + Assignee: apiIssue.Assignee, + Assignees: apiIssue.Assignees, + RequestedReviewers: apiIssue.RequestedReviewers, + State: apiIssue.State, + IsLocked: apiIssue.IsLocked, + Comments: apiIssue.Comments, + HTMLURL: pr.Issue.HTMLURL(), + DiffURL: pr.Issue.DiffURL(), + PatchURL: pr.Issue.PatchURL(), + HasMerged: pr.HasMerged, + MergeBase: pr.MergeBase, + Mergeable: pr.Mergeable(), + Deadline: apiIssue.Deadline, + Created: pr.Issue.CreatedUnix.AsTimePtr(), + Updated: pr.Issue.UpdatedUnix.AsTimePtr(), AllowMaintainerEdit: pr.AllowMaintainerEdit, diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 41d7dc7d2b0a9..2db112db3fbca 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -246,6 +246,7 @@ type WebhookForm struct { PullRequestComment bool PullRequestReview bool PullRequestSync bool + PullReviewRequest bool Wiki bool Repository bool Package bool diff --git a/services/webhook/notifier.go b/services/webhook/notifier.go index 1f7cb8d988a38..8b94561147e94 100644 --- a/services/webhook/notifier.go +++ b/services/webhook/notifier.go @@ -719,6 +719,34 @@ func (m *webhookNotifier) NotifyPullRequestReview(ctx context.Context, pr *issue } } +func (m *webhookNotifier) NotifyPullReviewRequest(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, reviewer *user_model.User, isRequest bool, comment *issues_model.Comment) { + if issue.IsPull { + log.Warn("NotifyPullReviewRequest: issue is not a pull request: %v", issue.ID) + return + } + mode, _ := access_model.AccessLevelUnit(ctx, doer, issue.Repo, unit.TypePullRequests) + if err := issue.LoadPullRequest(ctx); err != nil { + log.Error("LoadPullRequest failed: %v", err) + return + } + apiPullRequest := &api.PullRequestPayload{ + Index: issue.Index, + PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, nil), + RequestedReviewer: convert.ToUser(ctx, reviewer, nil), + Repository: convert.ToRepo(ctx, issue.Repo, mode), + Sender: convert.ToUser(ctx, doer, nil), + } + if isRequest { + apiPullRequest.Action = api.HookIssueReviewRequested + } else { + apiPullRequest.Action = api.HookIssueReviewRequestRemoved + } + if err := PrepareWebhooks(ctx, EventSource{Repository: issue.Repo}, webhook_module.HookEventPullReviewRequest, apiPullRequest); err != nil { + log.Error("PrepareWebhooks [review_requested: %v]: %v", isRequest, err) + return + } +} + func (m *webhookNotifier) NotifyCreateRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refType, refFullName, refID string) { apiPusher := convert.ToUser(ctx, pusher, nil) apiRepo := convert.ToRepo(ctx, repo, perm.AccessModeNone) From 4882266d5a702ebd773cba1abf97d6a168ac79c6 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Tue, 2 May 2023 16:45:08 +0800 Subject: [PATCH 02/22] fix: not issue trigger --- services/webhook/notifier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/webhook/notifier.go b/services/webhook/notifier.go index 8b94561147e94..c0bdf74feed08 100644 --- a/services/webhook/notifier.go +++ b/services/webhook/notifier.go @@ -720,7 +720,7 @@ func (m *webhookNotifier) NotifyPullRequestReview(ctx context.Context, pr *issue } func (m *webhookNotifier) NotifyPullReviewRequest(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, reviewer *user_model.User, isRequest bool, comment *issues_model.Comment) { - if issue.IsPull { + if !issue.IsPull { log.Warn("NotifyPullReviewRequest: issue is not a pull request: %v", issue.ID) return } From 22f0ae53bcd4267447e6f3ba02e27ac8cefb1bb9 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 3 May 2023 00:15:45 +0800 Subject: [PATCH 03/22] fix: dup reviewer --- models/issues/assignees.go | 21 ----------- models/issues/issue.go | 70 +++++++++++++++++++++++------------- models/issues/review_test.go | 27 ++++++++++++++ 3 files changed, 72 insertions(+), 46 deletions(-) diff --git a/models/issues/assignees.go b/models/issues/assignees.go index ec8366e3c2e63..fdd0d6f2274b6 100644 --- a/models/issues/assignees.go +++ b/models/issues/assignees.go @@ -44,27 +44,6 @@ func (issue *Issue) LoadAssignees(ctx context.Context) (err error) { return err } -// LoadRequestedReviewers load requestedReviewers of this issue. -func (issue *Issue) LoadRequestedReviewers(ctx context.Context) (err error) { - // Reset maybe preexisting reviewers - issue.RequestedReviewers = []*user_model.User{} - issue.RequestedReviewer = nil - - err = db.GetEngine(ctx).Table("`user`"). - Join("INNER", "review", "reviewer_id = `user`.id"). - Where("review.issue_id = ?", issue.ID). - Find(&issue.RequestedReviewers) - if err != nil { - return err - } - - // Check if we have at least one reviewer and if yes put it in as `RequestedReviewer` - if len(issue.RequestedReviewers) > 0 { - issue.RequestedReviewer = issue.RequestedReviewers[0] - } - return err -} - // GetAssigneeIDsByIssue returns the IDs of users assigned to an issue // but skips joining with `user` for performance reasons. // User permissions must be verified elsewhere if required. diff --git a/models/issues/issue.go b/models/issues/issue.go index 68377856341c2..d4fe790000bb9 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -103,31 +103,30 @@ func (err ErrIssueWasClosed) Error() string { // Issue represents an issue or pull request of repository. type Issue struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"` - Repo *repo_model.Repository `xorm:"-"` - Index int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository. - PosterID int64 `xorm:"INDEX"` - Poster *user_model.User `xorm:"-"` - OriginalAuthor string - OriginalAuthorID int64 `xorm:"index"` - Title string `xorm:"name"` - Content string `xorm:"LONGTEXT"` - RenderedContent string `xorm:"-"` - Labels []*Label `xorm:"-"` - MilestoneID int64 `xorm:"INDEX"` - Milestone *Milestone `xorm:"-"` - Project *project_model.Project `xorm:"-"` - Priority int - AssigneeID int64 `xorm:"-"` - Assignee *user_model.User `xorm:"-"` - RequestedReviewer *user_model.User `xorm:"-"` - IsClosed bool `xorm:"INDEX"` - IsRead bool `xorm:"-"` - IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not. - PullRequest *PullRequest `xorm:"-"` - NumComments int - Ref string + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"` + Repo *repo_model.Repository `xorm:"-"` + Index int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository. + PosterID int64 `xorm:"INDEX"` + Poster *user_model.User `xorm:"-"` + OriginalAuthor string + OriginalAuthorID int64 `xorm:"index"` + Title string `xorm:"name"` + Content string `xorm:"LONGTEXT"` + RenderedContent string `xorm:"-"` + Labels []*Label `xorm:"-"` + MilestoneID int64 `xorm:"INDEX"` + Milestone *Milestone `xorm:"-"` + Project *project_model.Project `xorm:"-"` + Priority int + AssigneeID int64 `xorm:"-"` + Assignee *user_model.User `xorm:"-"` + IsClosed bool `xorm:"INDEX"` + IsRead bool `xorm:"-"` + IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not. + PullRequest *PullRequest `xorm:"-"` + NumComments int + Ref string DeadlineUnix timeutil.TimeStamp `xorm:"INDEX"` @@ -270,6 +269,27 @@ func (issue *Issue) LoadPullRequest(ctx context.Context) (err error) { return nil } +// LoadRequestedReviewers load requestedReviewers of this issue. +func (issue *Issue) LoadRequestedReviewers(ctx context.Context) (err error) { + // Reset maybe preexisting reviewers + issue.RequestedReviewers = []*user_model.User{} + + reviews, err := GetReviewersByIssueID(issue.ID) + if err != nil { + return err + } + if len(reviews) > 0 { + for _, review := range reviews { + if err = review.LoadReviewer(ctx); err != nil { + return err + } + issue.RequestedReviewers = append(issue.RequestedReviewers, review.Reviewer) + } + } + + return err +} + func (issue *Issue) loadComments(ctx context.Context) (err error) { return issue.loadCommentsByType(ctx, CommentTypeUndefined) } diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 3221496577a90..5e7a4b52f2200 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -146,6 +146,33 @@ func TestGetReviewersByIssueID(t *testing.T) { } } +func TestLoadRequestedReviewers(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + assert.NoError(t, pull.LoadIssue(db.DefaultContext)) + issue := pull.Issue + assert.NoError(t, issue.LoadRepo(db.DefaultContext)) + assert.Len(t, issue.RequestedReviewers, 0) + + user1, err := user_model.GetUserByID(db.DefaultContext, 1) + assert.NoError(t, err) + + comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{}) + assert.NoError(t, err) + assert.NotNil(t, comment) + + assert.NoError(t, issue.LoadRequestedReviewers(db.DefaultContext)) + assert.Len(t, issue.RequestedReviewers, 1) + + comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{}) + assert.NoError(t, err) + assert.NotNil(t, comment) + + assert.NoError(t, issue.LoadRequestedReviewers(db.DefaultContext)) + assert.Empty(t, issue.RequestedReviewers) +} + func TestDismissReview(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) From 2617c4c70e1bb2e98b8b417cfae0b8b5347baf93 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 3 May 2023 00:16:39 +0800 Subject: [PATCH 04/22] fix: update swagger --- templates/swagger/v1_json.tmpl | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 51d123866de53..f9f44db891577 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -18587,6 +18587,13 @@ "repository": { "$ref": "#/definitions/RepositoryMeta" }, + "requested_reviewers": { + "type": "array", + "items": { + "$ref": "#/definitions/User" + }, + "x-go-name": "RequestedReviewers" + }, "state": { "$ref": "#/definitions/StateType" }, @@ -19822,6 +19829,13 @@ "type": "string", "x-go-name": "PatchURL" }, + "requested_reviewers": { + "type": "array", + "items": { + "$ref": "#/definitions/User" + }, + "x-go-name": "RequestedReviewers" + }, "state": { "$ref": "#/definitions/StateType" }, @@ -22353,4 +22367,4 @@ "TOTPHeader": [] } ] -} +} \ No newline at end of file From c5a9f4b1556ba26f87b0183be42a94238c60bdda Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 3 May 2023 00:29:11 +0800 Subject: [PATCH 05/22] fix: no newline at end of file --- templates/swagger/v1_json.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index f9f44db891577..84d403b919b58 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -22367,4 +22367,4 @@ "TOTPHeader": [] } ] -} \ No newline at end of file +} From 439a847afbe43e0f5a189d39aeaa641c8238512a Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 3 May 2023 00:38:14 +0800 Subject: [PATCH 06/22] feat: convertPayloader --- services/webhook/payloader.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/webhook/payloader.go b/services/webhook/payloader.go index 9eff25628bce8..ed4b3da3737bc 100644 --- a/services/webhook/payloader.go +++ b/services/webhook/payloader.go @@ -43,7 +43,7 @@ func convertPayloader(s PayloadConvertor, p api.Payloader, event webhook_module. case webhook_module.HookEventPush: return s.Push(p.(*api.PushPayload)) case webhook_module.HookEventPullRequest, webhook_module.HookEventPullRequestAssign, webhook_module.HookEventPullRequestLabel, - webhook_module.HookEventPullRequestMilestone, webhook_module.HookEventPullRequestSync: + webhook_module.HookEventPullRequestMilestone, webhook_module.HookEventPullRequestSync, webhook_module.HookEventPullReviewRequest: return s.PullRequest(p.(*api.PullRequestPayload)) case webhook_module.HookEventPullRequestReviewApproved, webhook_module.HookEventPullRequestReviewRejected, webhook_module.HookEventPullRequestReviewComment: return s.Review(p.(*api.PullRequestPayload), event) From ee0187d62ae9171d61b6f924eda00169f0b41b24 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 3 May 2023 01:09:27 +0800 Subject: [PATCH 07/22] feat: template button --- templates/repo/settings/webhook/settings.tmpl | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/templates/repo/settings/webhook/settings.tmpl b/templates/repo/settings/webhook/settings.tmpl index 57745c0401449..a41f0de7d5f70 100644 --- a/templates/repo/settings/webhook/settings.tmpl +++ b/templates/repo/settings/webhook/settings.tmpl @@ -238,6 +238,16 @@ + +
+
+
+ + + {{.locale.Tr "repo.settings.event_pull_review_request_desc"}} +
+
+
From 988c136f722a474f5b3bb7518ad07abcbeaef1e0 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Tue, 9 May 2023 23:09:38 +0800 Subject: [PATCH 08/22] fix: move reviewers field to pull --- models/issues/issue.go | 36 +++++------------------------------- models/issues/pull.go | 26 +++++++++++++++++++++++--- models/issues/pull_test.go | 28 ++++++++++++++++++++++++++++ models/issues/review_test.go | 27 --------------------------- services/convert/issue.go | 9 +-------- services/convert/pull.go | 10 ++++++++++ 6 files changed, 67 insertions(+), 69 deletions(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index d4fe790000bb9..8c173433f2546 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -134,12 +134,11 @@ type Issue struct { UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` ClosedUnix timeutil.TimeStamp `xorm:"INDEX"` - Attachments []*repo_model.Attachment `xorm:"-"` - Comments []*Comment `xorm:"-"` - Reactions ReactionList `xorm:"-"` - TotalTrackedTime int64 `xorm:"-"` - Assignees []*user_model.User `xorm:"-"` - RequestedReviewers []*user_model.User `xorm:"-"` + Attachments []*repo_model.Attachment `xorm:"-"` + Comments []*Comment `xorm:"-"` + Reactions ReactionList `xorm:"-"` + TotalTrackedTime int64 `xorm:"-"` + Assignees []*user_model.User `xorm:"-"` // IsLocked limits commenting abilities to users on an issue // with write access @@ -269,27 +268,6 @@ func (issue *Issue) LoadPullRequest(ctx context.Context) (err error) { return nil } -// LoadRequestedReviewers load requestedReviewers of this issue. -func (issue *Issue) LoadRequestedReviewers(ctx context.Context) (err error) { - // Reset maybe preexisting reviewers - issue.RequestedReviewers = []*user_model.User{} - - reviews, err := GetReviewersByIssueID(issue.ID) - if err != nil { - return err - } - if len(reviews) > 0 { - for _, review := range reviews { - if err = review.LoadReviewer(ctx); err != nil { - return err - } - issue.RequestedReviewers = append(issue.RequestedReviewers, review.Reviewer) - } - } - - return err -} - func (issue *Issue) loadComments(ctx context.Context) (err error) { return issue.loadCommentsByType(ctx, CommentTypeUndefined) } @@ -381,10 +359,6 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return } - if err = issue.LoadRequestedReviewers(ctx); err != nil { - return - } - if err = issue.LoadPullRequest(ctx); err != nil && !IsErrPullRequestNotExist(err) { // It is possible pull request is not yet created. return err diff --git a/models/issues/pull.go b/models/issues/pull.go index a15ebec0b52a3..2729ebfcc49bf 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -175,9 +175,10 @@ type PullRequest struct { ChangedProtectedFiles []string `xorm:"TEXT JSON"` - IssueID int64 `xorm:"INDEX"` - Issue *Issue `xorm:"-"` - Index int64 + IssueID int64 `xorm:"INDEX"` + Issue *Issue `xorm:"-"` + Index int64 + RequestedReviewers []*user_model.User `xorm:"-"` HeadRepoID int64 `xorm:"INDEX"` HeadRepo *repo_model.Repository `xorm:"-"` @@ -315,6 +316,25 @@ func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { return nil } +func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) (err error) { + // Reset maybe preexisting reviewers + pr.RequestedReviewers = []*user_model.User{} + + reviews, err := GetReviewersByIssueID(pr.Issue.ID) + if err != nil { + return err + } + if len(reviews) > 0 { + for _, review := range reviews { + if err = review.LoadReviewer(ctx); err != nil { + return err + } + pr.RequestedReviewers = append(pr.RequestedReviewers, review.Reviewer) + } + } + return nil +} + // LoadBaseRepo loads the target repository. ErrRepoNotExist may be returned. func (pr *PullRequest) LoadBaseRepo(ctx context.Context) (err error) { if pr.BaseRepo != nil { diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 8d99b2667ca68..4e29caa94d88e 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -4,6 +4,7 @@ package issues_test import ( + user_model "code.gitea.io/gitea/models/user" "testing" "code.gitea.io/gitea/models/db" @@ -74,6 +75,33 @@ func TestPullRequestsNewest(t *testing.T) { } } +func TestLoadRequestedReviewers(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) + assert.NoError(t, pull.LoadIssue(db.DefaultContext)) + issue := pull.Issue + assert.NoError(t, issue.LoadRepo(db.DefaultContext)) + assert.Len(t, pull.RequestedReviewers, 0) + + user1, err := user_model.GetUserByID(db.DefaultContext, 1) + assert.NoError(t, err) + + comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{}) + assert.NoError(t, err) + assert.NotNil(t, comment) + + assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext)) + assert.Len(t, pull.RequestedReviewers, 1) + + comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{}) + assert.NoError(t, err) + assert.NotNil(t, comment) + + assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext)) + assert.Empty(t, pull.RequestedReviewers) +} + func TestPullRequestsOldest(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) prs, count, err := issues_model.PullRequests(1, &issues_model.PullRequestsOptions{ diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 5e7a4b52f2200..3221496577a90 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -146,33 +146,6 @@ func TestGetReviewersByIssueID(t *testing.T) { } } -func TestLoadRequestedReviewers(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 1}) - assert.NoError(t, pull.LoadIssue(db.DefaultContext)) - issue := pull.Issue - assert.NoError(t, issue.LoadRepo(db.DefaultContext)) - assert.Len(t, issue.RequestedReviewers, 0) - - user1, err := user_model.GetUserByID(db.DefaultContext, 1) - assert.NoError(t, err) - - comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{}) - assert.NoError(t, err) - assert.NotNil(t, comment) - - assert.NoError(t, issue.LoadRequestedReviewers(db.DefaultContext)) - assert.Len(t, issue.RequestedReviewers, 1) - - comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{}) - assert.NoError(t, err) - assert.NotNil(t, comment) - - assert.NoError(t, issue.LoadRequestedReviewers(db.DefaultContext)) - assert.Empty(t, issue.RequestedReviewers) -} - func TestDismissReview(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/services/convert/issue.go b/services/convert/issue.go index dfa18ab2dde38..ee5a6fa5fcfbe 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -83,14 +83,7 @@ func ToAPIIssue(ctx context.Context, issue *issues_model.Issue) *api.Issue { } apiIssue.Assignee = ToUser(ctx, issue.Assignees[0], nil) // For compatibility, we're keeping the first assignee as `apiIssue.Assignee` } - if err := issue.LoadRequestedReviewers(ctx); err != nil { - return &api.Issue{} - } - if len(issue.RequestedReviewers) > 0 { - for _, reviewers := range issue.RequestedReviewers { - apiIssue.RequestedReviewers = append(apiIssue.RequestedReviewers, ToUser(ctx, reviewers, nil)) - } - } + if issue.IsPull { if err := issue.LoadPullRequest(ctx); err != nil { return &api.Issue{} diff --git a/services/convert/pull.go b/services/convert/pull.go index bac7033aecc39..f4b7eae36ea4a 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -43,6 +43,16 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u return nil } + if err := pr.LoadRequestedReviewers(ctx); err != nil { + log.Error("LoadRequestedReviewers[%d]: %v", pr.ID, err) + return nil + } + if len(pr.RequestedReviewers) > 0 { + for _, reviewer := range pr.RequestedReviewers { + apiIssue.RequestedReviewers = append(apiIssue.RequestedReviewers, ToUser(ctx, reviewer, nil)) + } + } + p, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer) if err != nil { log.Error("GetUserRepoPermission[%d]: %v", pr.BaseRepoID, err) From 9c9dbde40943f7ec6b6c0e0f857ad49c3d80ac19 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Tue, 9 May 2023 23:16:54 +0800 Subject: [PATCH 09/22] fix: remove reviewers in issue --- modules/structs/issue.go | 5 ++- services/convert/issue.go | 1 - services/convert/pull.go | 70 +++++++++++++++++++-------------------- 3 files changed, 36 insertions(+), 40 deletions(-) diff --git a/modules/structs/issue.go b/modules/structs/issue.go index 8a059a15df3ad..04e169df84197 100644 --- a/modules/structs/issue.go +++ b/modules/structs/issue.go @@ -55,9 +55,8 @@ type Issue struct { Labels []*Label `json:"labels"` Milestone *Milestone `json:"milestone"` // deprecated - Assignee *User `json:"assignee"` - Assignees []*User `json:"assignees"` - RequestedReviewers []*User `json:"requested_reviewers"` + Assignee *User `json:"assignee"` + Assignees []*User `json:"assignees"` // Whether the issue is open or closed // // type: string diff --git a/services/convert/issue.go b/services/convert/issue.go index ee5a6fa5fcfbe..6d31a123bd9fc 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -83,7 +83,6 @@ func ToAPIIssue(ctx context.Context, issue *issues_model.Issue) *api.Issue { } apiIssue.Assignee = ToUser(ctx, issue.Assignees[0], nil) // For compatibility, we're keeping the first assignee as `apiIssue.Assignee` } - if issue.IsPull { if err := issue.LoadPullRequest(ctx); err != nil { return &api.Issue{} diff --git a/services/convert/pull.go b/services/convert/pull.go index f4b7eae36ea4a..4eeeac2d73758 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -4,9 +4,6 @@ package convert import ( - "context" - "fmt" - issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" @@ -14,6 +11,8 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" + "context" + "fmt" ) // ToAPIPullRequest assumes following fields have been assigned with valid values: @@ -43,16 +42,6 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u return nil } - if err := pr.LoadRequestedReviewers(ctx); err != nil { - log.Error("LoadRequestedReviewers[%d]: %v", pr.ID, err) - return nil - } - if len(pr.RequestedReviewers) > 0 { - for _, reviewer := range pr.RequestedReviewers { - apiIssue.RequestedReviewers = append(apiIssue.RequestedReviewers, ToUser(ctx, reviewer, nil)) - } - } - p, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer) if err != nil { log.Error("GetUserRepoPermission[%d]: %v", pr.BaseRepoID, err) @@ -60,29 +49,28 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u } apiPullRequest := &api.PullRequest{ - ID: pr.ID, - URL: pr.Issue.HTMLURL(), - Index: pr.Index, - Poster: apiIssue.Poster, - Title: apiIssue.Title, - Body: apiIssue.Body, - Labels: apiIssue.Labels, - Milestone: apiIssue.Milestone, - Assignee: apiIssue.Assignee, - Assignees: apiIssue.Assignees, - RequestedReviewers: apiIssue.RequestedReviewers, - State: apiIssue.State, - IsLocked: apiIssue.IsLocked, - Comments: apiIssue.Comments, - HTMLURL: pr.Issue.HTMLURL(), - DiffURL: pr.Issue.DiffURL(), - PatchURL: pr.Issue.PatchURL(), - HasMerged: pr.HasMerged, - MergeBase: pr.MergeBase, - Mergeable: pr.Mergeable(), - Deadline: apiIssue.Deadline, - Created: pr.Issue.CreatedUnix.AsTimePtr(), - Updated: pr.Issue.UpdatedUnix.AsTimePtr(), + ID: pr.ID, + URL: pr.Issue.HTMLURL(), + Index: pr.Index, + Poster: apiIssue.Poster, + Title: apiIssue.Title, + Body: apiIssue.Body, + Labels: apiIssue.Labels, + Milestone: apiIssue.Milestone, + Assignee: apiIssue.Assignee, + Assignees: apiIssue.Assignees, + State: apiIssue.State, + IsLocked: apiIssue.IsLocked, + Comments: apiIssue.Comments, + HTMLURL: pr.Issue.HTMLURL(), + DiffURL: pr.Issue.DiffURL(), + PatchURL: pr.Issue.PatchURL(), + HasMerged: pr.HasMerged, + MergeBase: pr.MergeBase, + Mergeable: pr.Mergeable(), + Deadline: apiIssue.Deadline, + Created: pr.Issue.CreatedUnix.AsTimePtr(), + Updated: pr.Issue.UpdatedUnix.AsTimePtr(), AllowMaintainerEdit: pr.AllowMaintainerEdit, @@ -99,6 +87,16 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u }, } + if err := pr.LoadRequestedReviewers(ctx); err != nil { + log.Error("LoadRequestedReviewers[%d]: %v", pr.ID, err) + return nil + } + if len(pr.RequestedReviewers) > 0 { + for _, reviewer := range pr.RequestedReviewers { + apiPullRequest.RequestedReviewers = append(apiPullRequest.RequestedReviewers, ToUser(ctx, reviewer, nil)) + } + } + if pr.Issue.ClosedUnix != 0 { apiPullRequest.Closed = pr.Issue.ClosedUnix.AsTimePtr() } From cc11f8fa53e7aa3439bf2a59d7e11f8fd3c9050f Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Tue, 9 May 2023 23:40:12 +0800 Subject: [PATCH 10/22] fix: update v1_json.tmpl --- templates/swagger/v1_json.tmpl | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 84d403b919b58..721f610457218 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -18587,13 +18587,6 @@ "repository": { "$ref": "#/definitions/RepositoryMeta" }, - "requested_reviewers": { - "type": "array", - "items": { - "$ref": "#/definitions/User" - }, - "x-go-name": "RequestedReviewers" - }, "state": { "$ref": "#/definitions/StateType" }, @@ -22367,4 +22360,4 @@ "TOTPHeader": [] } ] -} +} \ No newline at end of file From 57964f4bb241d8380c1db4dc0288baf939ba0d08 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 10 May 2023 00:42:33 +0800 Subject: [PATCH 11/22] fix: ci lint error --- models/issues/pull_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 4e29caa94d88e..0c2849aa80258 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -4,9 +4,10 @@ package issues_test import ( - user_model "code.gitea.io/gitea/models/user" "testing" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" From e2a743c8d3b5bc6d412cc2cd00e04521e2ba5c08 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 10 May 2023 00:45:05 +0800 Subject: [PATCH 12/22] fix: ci lint error --- services/convert/pull.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/convert/pull.go b/services/convert/pull.go index 4eeeac2d73758..d9436278ef106 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -4,6 +4,9 @@ package convert import ( + "context" + "fmt" + issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" @@ -11,8 +14,6 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" - "context" - "fmt" ) // ToAPIPullRequest assumes following fields have been assigned with valid values: From 3568067592073c446b21e8db926bb6089feb7438 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 10 May 2023 23:22:47 +0800 Subject: [PATCH 13/22] fix: ci lint error --- templates/swagger/v1_json.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 721f610457218..945c379ea3084 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -22360,4 +22360,4 @@ "TOTPHeader": [] } ] -} \ No newline at end of file +} From 13dad91a193eb618914a5dfc1912f71db720df33 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Fri, 12 May 2023 01:03:08 +0800 Subject: [PATCH 14/22] fix: load reviewers return if existed --- models/issues/pull.go | 8 +++++--- models/issues/pull_test.go | 1 + services/convert/pull.go | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 2729ebfcc49bf..8dd77b802846f 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -316,14 +316,16 @@ func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { return nil } -func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) (err error) { - // Reset maybe preexisting reviewers - pr.RequestedReviewers = []*user_model.User{} +func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { + if len(pr.RequestedReviewers) > 0 { + return nil + } reviews, err := GetReviewersByIssueID(pr.Issue.ID) if err != nil { return err } + if len(reviews) > 0 { for _, review := range reviews { if err = review.LoadReviewer(ctx); err != nil { diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 0c2849aa80258..97e5f2bdb78c0 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -99,6 +99,7 @@ func TestLoadRequestedReviewers(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, comment) + pull.RequestedReviewers = nil assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext)) assert.Empty(t, pull.RequestedReviewers) } diff --git a/services/convert/pull.go b/services/convert/pull.go index d9436278ef106..7345117afeabc 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -88,7 +88,7 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u }, } - if err := pr.LoadRequestedReviewers(ctx); err != nil { + if err = pr.LoadRequestedReviewers(ctx); err != nil { log.Error("LoadRequestedReviewers[%d]: %v", pr.ID, err) return nil } From c5a553f43804a49382bd80abd36d694b7732fcb4 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Fri, 12 May 2023 01:45:19 +0800 Subject: [PATCH 15/22] fix: rename event --- models/webhook/webhook.go | 8 +-- models/webhook/webhook_test.go | 2 +- modules/webhook/structs.go | 42 ++++++++-------- modules/webhook/type.go | 4 +- routers/api/v1/utils/hook.go | 42 ++++++++-------- routers/web/repo/webhook.go | 42 ++++++++-------- services/forms/repo_form.go | 50 +++++++++---------- services/webhook/notifier.go | 2 +- services/webhook/payloader.go | 2 +- templates/repo/settings/webhook/settings.tmpl | 6 +-- 10 files changed, 100 insertions(+), 100 deletions(-) diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index 875d5ddb09397..fc2bbed083ab5 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -298,10 +298,10 @@ func (w *Webhook) HasPackageEvent() bool { (w.ChooseEvents && w.HookEvents.Package) } -// HasPullReviewRequestEvent returns true if hook enabled pull request review request event. -func (w *Webhook) HasPullReviewRequestEvent() bool { +// HasPullRequestReviewRequestEvent returns true if hook enabled pull request review request event. +func (w *Webhook) HasPullRequestReviewRequestEvent() bool { return w.SendEverything || - (w.ChooseEvents && w.HookEvents.PullReviewRequest) + (w.ChooseEvents && w.HookEvents.PullRequestReviewRequest) } // EventCheckers returns event checkers @@ -335,7 +335,7 @@ func (w *Webhook) EventCheckers() []struct { {w.HasRepositoryEvent, webhook_module.HookEventRepository}, {w.HasReleaseEvent, webhook_module.HookEventRelease}, {w.HasPackageEvent, webhook_module.HookEventPackage}, - {w.HasPullReviewRequestEvent, webhook_module.HookEventPullReviewRequest}, + {w.HasPullRequestReviewRequestEvent, webhook_module.HookEventPullRequestReviewRequest}, } } diff --git a/models/webhook/webhook_test.go b/models/webhook/webhook_test.go index 070af821fac90..2f4b77f134c26 100644 --- a/models/webhook/webhook_test.go +++ b/models/webhook/webhook_test.go @@ -72,7 +72,7 @@ func TestWebhook_EventsArray(t *testing.T) { "pull_request", "pull_request_assign", "pull_request_label", "pull_request_milestone", "pull_request_comment", "pull_request_review_approved", "pull_request_review_rejected", "pull_request_review_comment", "pull_request_sync", "wiki", "repository", "release", - "package", "pull_review_request", + "package", "pull_request_review_request", }, (&Webhook{ HookEvent: &webhook_module.HookEvent{SendEverything: true}, diff --git a/modules/webhook/structs.go b/modules/webhook/structs.go index 6030940c52e6a..927a91a74c545 100644 --- a/modules/webhook/structs.go +++ b/modules/webhook/structs.go @@ -5,27 +5,27 @@ package webhook // HookEvents is a set of web hook events type HookEvents struct { - Create bool `json:"create"` - Delete bool `json:"delete"` - Fork bool `json:"fork"` - Issues bool `json:"issues"` - IssueAssign bool `json:"issue_assign"` - IssueLabel bool `json:"issue_label"` - IssueMilestone bool `json:"issue_milestone"` - IssueComment bool `json:"issue_comment"` - Push bool `json:"push"` - PullRequest bool `json:"pull_request"` - PullRequestAssign bool `json:"pull_request_assign"` - PullRequestLabel bool `json:"pull_request_label"` - PullRequestMilestone bool `json:"pull_request_milestone"` - PullRequestComment bool `json:"pull_request_comment"` - PullRequestReview bool `json:"pull_request_review"` - PullRequestSync bool `json:"pull_request_sync"` - PullReviewRequest bool `json:"pull_review_request"` - Wiki bool `json:"wiki"` - Repository bool `json:"repository"` - Release bool `json:"release"` - Package bool `json:"package"` + Create bool `json:"create"` + Delete bool `json:"delete"` + Fork bool `json:"fork"` + Issues bool `json:"issues"` + IssueAssign bool `json:"issue_assign"` + IssueLabel bool `json:"issue_label"` + IssueMilestone bool `json:"issue_milestone"` + IssueComment bool `json:"issue_comment"` + Push bool `json:"push"` + PullRequest bool `json:"pull_request"` + PullRequestAssign bool `json:"pull_request_assign"` + PullRequestLabel bool `json:"pull_request_label"` + PullRequestMilestone bool `json:"pull_request_milestone"` + PullRequestComment bool `json:"pull_request_comment"` + PullRequestReview bool `json:"pull_request_review"` + PullRequestSync bool `json:"pull_request_sync"` + PullRequestReviewRequest bool `json:"pull_request_review_request"` + Wiki bool `json:"wiki"` + Repository bool `json:"repository"` + Release bool `json:"release"` + Package bool `json:"package"` } // HookEvent represents events that will delivery hook. diff --git a/modules/webhook/type.go b/modules/webhook/type.go index 9a72de8eb4307..7042d391b7eb4 100644 --- a/modules/webhook/type.go +++ b/modules/webhook/type.go @@ -26,7 +26,7 @@ const ( HookEventPullRequestReviewRejected HookEventType = "pull_request_review_rejected" HookEventPullRequestReviewComment HookEventType = "pull_request_review_comment" HookEventPullRequestSync HookEventType = "pull_request_sync" - HookEventPullReviewRequest HookEventType = "pull_review_request" + HookEventPullRequestReviewRequest HookEventType = "pull_request_review_request" HookEventWiki HookEventType = "wiki" HookEventRepository HookEventType = "repository" HookEventRelease HookEventType = "release" @@ -47,7 +47,7 @@ func (h HookEventType) Event() string { case HookEventIssues, HookEventIssueAssign, HookEventIssueLabel, HookEventIssueMilestone: return "issues" case HookEventPullRequest, HookEventPullRequestAssign, HookEventPullRequestLabel, HookEventPullRequestMilestone, - HookEventPullRequestSync, HookEventPullReviewRequest: + HookEventPullRequestSync, HookEventPullRequestReviewRequest: return "pull_request" case HookEventIssueComment, HookEventPullRequestComment: return "issue_comment" diff --git a/routers/api/v1/utils/hook.go b/routers/api/v1/utils/hook.go index 268db2903c4e7..b62d20a18a617 100644 --- a/routers/api/v1/utils/hook.go +++ b/routers/api/v1/utils/hook.go @@ -179,26 +179,26 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI HookEvent: &webhook_module.HookEvent{ ChooseEvents: true, HookEvents: webhook_module.HookEvents{ - Create: util.SliceContainsString(form.Events, string(webhook_module.HookEventCreate), true), - Delete: util.SliceContainsString(form.Events, string(webhook_module.HookEventDelete), true), - Fork: util.SliceContainsString(form.Events, string(webhook_module.HookEventFork), true), - Issues: issuesHook(form.Events, "issues_only"), - IssueAssign: issuesHook(form.Events, string(webhook_module.HookEventIssueAssign)), - IssueLabel: issuesHook(form.Events, string(webhook_module.HookEventIssueLabel)), - IssueMilestone: issuesHook(form.Events, string(webhook_module.HookEventIssueMilestone)), - IssueComment: issuesHook(form.Events, string(webhook_module.HookEventIssueComment)), - Push: util.SliceContainsString(form.Events, string(webhook_module.HookEventPush), true), - PullRequest: pullHook(form.Events, "pull_request_only"), - PullRequestAssign: pullHook(form.Events, string(webhook_module.HookEventPullRequestAssign)), - PullRequestLabel: pullHook(form.Events, string(webhook_module.HookEventPullRequestLabel)), - PullRequestMilestone: pullHook(form.Events, string(webhook_module.HookEventPullRequestMilestone)), - PullRequestComment: pullHook(form.Events, string(webhook_module.HookEventPullRequestComment)), - PullRequestReview: pullHook(form.Events, "pull_request_review"), - PullReviewRequest: pullHook(form.Events, string(webhook_module.HookEventPullReviewRequest)), - PullRequestSync: pullHook(form.Events, string(webhook_module.HookEventPullRequestSync)), - Wiki: util.SliceContainsString(form.Events, string(webhook_module.HookEventWiki), true), - Repository: util.SliceContainsString(form.Events, string(webhook_module.HookEventRepository), true), - Release: util.SliceContainsString(form.Events, string(webhook_module.HookEventRelease), true), + Create: util.SliceContainsString(form.Events, string(webhook_module.HookEventCreate), true), + Delete: util.SliceContainsString(form.Events, string(webhook_module.HookEventDelete), true), + Fork: util.SliceContainsString(form.Events, string(webhook_module.HookEventFork), true), + Issues: issuesHook(form.Events, "issues_only"), + IssueAssign: issuesHook(form.Events, string(webhook_module.HookEventIssueAssign)), + IssueLabel: issuesHook(form.Events, string(webhook_module.HookEventIssueLabel)), + IssueMilestone: issuesHook(form.Events, string(webhook_module.HookEventIssueMilestone)), + IssueComment: issuesHook(form.Events, string(webhook_module.HookEventIssueComment)), + Push: util.SliceContainsString(form.Events, string(webhook_module.HookEventPush), true), + PullRequest: pullHook(form.Events, "pull_request_only"), + PullRequestAssign: pullHook(form.Events, string(webhook_module.HookEventPullRequestAssign)), + PullRequestLabel: pullHook(form.Events, string(webhook_module.HookEventPullRequestLabel)), + PullRequestMilestone: pullHook(form.Events, string(webhook_module.HookEventPullRequestMilestone)), + PullRequestComment: pullHook(form.Events, string(webhook_module.HookEventPullRequestComment)), + PullRequestReview: pullHook(form.Events, "pull_request_review"), + PullRequestReviewRequest: pullHook(form.Events, string(webhook_module.HookEventPullRequestReviewRequest)), + PullRequestSync: pullHook(form.Events, string(webhook_module.HookEventPullRequestSync)), + Wiki: util.SliceContainsString(form.Events, string(webhook_module.HookEventWiki), true), + Repository: util.SliceContainsString(form.Events, string(webhook_module.HookEventRepository), true), + Release: util.SliceContainsString(form.Events, string(webhook_module.HookEventRelease), true), }, BranchFilter: form.BranchFilter, }, @@ -380,7 +380,7 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh w.PullRequestMilestone = pullHook(form.Events, string(webhook_module.HookEventPullRequestMilestone)) w.PullRequestComment = pullHook(form.Events, string(webhook_module.HookEventPullRequestComment)) w.PullRequestReview = pullHook(form.Events, "pull_request_review") - w.PullReviewRequest = pullHook(form.Events, string(webhook_module.HookEventPullReviewRequest)) + w.PullRequestReviewRequest = pullHook(form.Events, string(webhook_module.HookEventPullRequestReviewRequest)) w.PullRequestSync = pullHook(form.Events, string(webhook_module.HookEventPullRequestSync)) if err := w.UpdateEvent(); err != nil { diff --git a/routers/web/repo/webhook.go b/routers/web/repo/webhook.go index b746e1e87a22d..82d8b565a2b38 100644 --- a/routers/web/repo/webhook.go +++ b/routers/web/repo/webhook.go @@ -160,27 +160,27 @@ func ParseHookEvent(form forms.WebhookForm) *webhook_module.HookEvent { SendEverything: form.SendEverything(), ChooseEvents: form.ChooseEvents(), HookEvents: webhook_module.HookEvents{ - Create: form.Create, - Delete: form.Delete, - Fork: form.Fork, - Issues: form.Issues, - IssueAssign: form.IssueAssign, - IssueLabel: form.IssueLabel, - IssueMilestone: form.IssueMilestone, - IssueComment: form.IssueComment, - Release: form.Release, - Push: form.Push, - PullRequest: form.PullRequest, - PullRequestAssign: form.PullRequestAssign, - PullRequestLabel: form.PullRequestLabel, - PullRequestMilestone: form.PullRequestMilestone, - PullRequestComment: form.PullRequestComment, - PullRequestReview: form.PullRequestReview, - PullRequestSync: form.PullRequestSync, - PullReviewRequest: form.PullReviewRequest, - Wiki: form.Wiki, - Repository: form.Repository, - Package: form.Package, + Create: form.Create, + Delete: form.Delete, + Fork: form.Fork, + Issues: form.Issues, + IssueAssign: form.IssueAssign, + IssueLabel: form.IssueLabel, + IssueMilestone: form.IssueMilestone, + IssueComment: form.IssueComment, + Release: form.Release, + Push: form.Push, + PullRequest: form.PullRequest, + PullRequestAssign: form.PullRequestAssign, + PullRequestLabel: form.PullRequestLabel, + PullRequestMilestone: form.PullRequestMilestone, + PullRequestComment: form.PullRequestComment, + PullRequestReview: form.PullRequestReview, + PullRequestSync: form.PullRequestSync, + PullRequestReviewRequest: form.PullRequestReviewRequest, + Wiki: form.Wiki, + Repository: form.Repository, + Package: form.Package, }, BranchFilter: form.BranchFilter, } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 2db112db3fbca..71f91c144950d 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -228,31 +228,31 @@ func (f *ProtectBranchForm) Validate(req *http.Request, errs binding.Errors) bin // WebhookForm form for changing web hook type WebhookForm struct { - Events string - Create bool - Delete bool - Fork bool - Issues bool - IssueAssign bool - IssueLabel bool - IssueMilestone bool - IssueComment bool - Release bool - Push bool - PullRequest bool - PullRequestAssign bool - PullRequestLabel bool - PullRequestMilestone bool - PullRequestComment bool - PullRequestReview bool - PullRequestSync bool - PullReviewRequest bool - Wiki bool - Repository bool - Package bool - Active bool - BranchFilter string `binding:"GlobPattern"` - AuthorizationHeader string + Events string + Create bool + Delete bool + Fork bool + Issues bool + IssueAssign bool + IssueLabel bool + IssueMilestone bool + IssueComment bool + Release bool + Push bool + PullRequest bool + PullRequestAssign bool + PullRequestLabel bool + PullRequestMilestone bool + PullRequestComment bool + PullRequestReview bool + PullRequestSync bool + PullRequestReviewRequest bool + Wiki bool + Repository bool + Package bool + Active bool + BranchFilter string `binding:"GlobPattern"` + AuthorizationHeader string } // PushOnly if the hook will be triggered when push diff --git a/services/webhook/notifier.go b/services/webhook/notifier.go index c0bdf74feed08..bd25e20805e59 100644 --- a/services/webhook/notifier.go +++ b/services/webhook/notifier.go @@ -741,7 +741,7 @@ func (m *webhookNotifier) NotifyPullReviewRequest(ctx context.Context, doer *use } else { apiPullRequest.Action = api.HookIssueReviewRequestRemoved } - if err := PrepareWebhooks(ctx, EventSource{Repository: issue.Repo}, webhook_module.HookEventPullReviewRequest, apiPullRequest); err != nil { + if err := PrepareWebhooks(ctx, EventSource{Repository: issue.Repo}, webhook_module.HookEventPullRequestReviewRequest, apiPullRequest); err != nil { log.Error("PrepareWebhooks [review_requested: %v]: %v", isRequest, err) return } diff --git a/services/webhook/payloader.go b/services/webhook/payloader.go index ed4b3da3737bc..d53e65fa5ee47 100644 --- a/services/webhook/payloader.go +++ b/services/webhook/payloader.go @@ -43,7 +43,7 @@ func convertPayloader(s PayloadConvertor, p api.Payloader, event webhook_module. case webhook_module.HookEventPush: return s.Push(p.(*api.PushPayload)) case webhook_module.HookEventPullRequest, webhook_module.HookEventPullRequestAssign, webhook_module.HookEventPullRequestLabel, - webhook_module.HookEventPullRequestMilestone, webhook_module.HookEventPullRequestSync, webhook_module.HookEventPullReviewRequest: + webhook_module.HookEventPullRequestMilestone, webhook_module.HookEventPullRequestSync, webhook_module.HookEventPullRequestReviewRequest: return s.PullRequest(p.(*api.PullRequestPayload)) case webhook_module.HookEventPullRequestReviewApproved, webhook_module.HookEventPullRequestReviewRejected, webhook_module.HookEventPullRequestReviewComment: return s.Review(p.(*api.PullRequestPayload), event) diff --git a/templates/repo/settings/webhook/settings.tmpl b/templates/repo/settings/webhook/settings.tmpl index a41f0de7d5f70..8b03a3c959792 100644 --- a/templates/repo/settings/webhook/settings.tmpl +++ b/templates/repo/settings/webhook/settings.tmpl @@ -242,9 +242,9 @@
- - - {{.locale.Tr "repo.settings.event_pull_review_request_desc"}} + + + {{.locale.Tr "repo.settings.event_pull_request_review_request_desc"}}
From 161270ced5d4c185c0e6f9bcccaba85ee95f0ab3 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Fri, 12 May 2023 02:05:36 +0800 Subject: [PATCH 16/22] fix: ci lint error --- models/issues/pull_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index abdda192a7a4a..7a183ac3121c2 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -6,11 +6,10 @@ package issues_test import ( "testing" - user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "github.com/stretchr/testify/assert" ) From d22436d45bfc56ad70e94fdf68d918f88393279d Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 24 May 2023 22:19:22 +0800 Subject: [PATCH 17/22] fix: batch load reviewers --- models/issues/pull.go | 7 +++--- models/issues/review.go | 20 ++++++++++++++++++ models/issues/review_test.go | 15 +++++++++++-- models/user/user.go | 41 ++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index efa447a643ea2..8128f3abe52e5 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -327,10 +327,11 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { } if len(reviews) > 0 { + err = LoadReviewers(ctx, reviews) + if err != nil { + return err + } for _, review := range reviews { - if err = review.LoadReviewer(ctx); err != nil { - return err - } pr.RequestedReviewers = append(pr.RequestedReviewers, review.Reviewer) } } diff --git a/models/issues/review.go b/models/issues/review.go index ed30bce149a3b..d050e2d6e7f7b 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -162,6 +162,26 @@ func (r *Review) LoadReviewer(ctx context.Context) (err error) { return err } +func LoadReviewers(ctx context.Context, reviews []*Review) (err error) { + reviewerIds := make([]int64, len(reviews)) + for i := 0; i < len(reviews); i++ { + reviewerIds[i] = reviews[i].ReviewerID + } + reviewers, err := user_model.GetPossibleUserByIDs(ctx, reviewerIds) + if err != nil { + return err + } + + userMap := make(map[int64]*user_model.User, len(reviewers)) + for _, reviewer := range reviewers { + userMap[reviewer.ID] = reviewer + } + for _, review := range reviews { + review.Reviewer = userMap[review.ReviewerID] + } + return nil +} + // LoadReviewerTeam loads reviewer team func (r *Review) LoadReviewerTeam(ctx context.Context) (err error) { if r.ReviewerTeamID == 0 || r.ReviewerTeam != nil { diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 3221496577a90..7fba8ca437dae 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -133,10 +133,21 @@ func TestGetReviewersByIssueID(t *testing.T) { }) allReviews, err := issues_model.GetReviewersByIssueID(issue.ID) - for _, reviewer := range allReviews { - assert.NoError(t, reviewer.LoadReviewer(db.DefaultContext)) + assert.NoError(t, err) + for _, review := range allReviews { + assert.NoError(t, review.LoadReviewer(db.DefaultContext)) } + if assert.Len(t, allReviews, 3) { + for i, review := range allReviews { + assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer) + assert.Equal(t, expectedReviews[i].Type, review.Type) + assert.Equal(t, expectedReviews[i].UpdatedUnix, review.UpdatedUnix) + } + } + + allReviews, err = issues_model.GetReviewersByIssueID(issue.ID) assert.NoError(t, err) + assert.NoError(t, issues_model.LoadReviewers(db.DefaultContext, allReviews)) if assert.Len(t, allReviews, 3) { for i, review := range allReviews { assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer) diff --git a/models/user/user.go b/models/user/user.go index 46c4440e5f07e..824a322499d4a 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -954,6 +954,15 @@ func GetUserByID(ctx context.Context, id int64) (*User, error) { return u, nil } +// GetUserByIDs returns the user objects by given IDs if exists. +func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { + users := make([]*User, 0, len(ids)) + err := db.GetEngine(ctx).In("id", ids). + Table("user"). + Find(&users) + return users, err +} + // GetPossibleUserByID returns the user if id > 0 or return system usrs if id < 0 func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) { switch id { @@ -968,6 +977,38 @@ func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) { } } +// GetPossibleUserByIDs returns the users if id > 0 or return system users if id < 0 +func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { + uniqueIDs := make(map[int64]struct{}) + users := make([]*User, 0, len(ids)) + for _, id := range ids { + uniqueIDs[id] = struct{}{} + } + if _, ok := uniqueIDs[0]; ok { + return nil, ErrUserNotExist{} + } + + if _, ok := uniqueIDs[-1]; ok { + users = append(users, NewGhostUser()) + delete(uniqueIDs, -1) + } + if _, ok := uniqueIDs[ActionsUserID]; ok { + users = append(users, NewActionsUser()) + delete(uniqueIDs, ActionsUserID) + } + + needFindIds := make([]int64, 0, len(uniqueIDs)) + for id := range uniqueIDs { + needFindIds = append(needFindIds, id) + } + res, err := GetUserByIDs(ctx, needFindIds) + if err != nil { + return nil, err + } + users = append(users, res...) + return users, nil +} + // GetUserByNameCtx returns user by given name. func GetUserByName(ctx context.Context, name string) (*User, error) { if len(name) == 0 { From ff20eee04159f3047448bc343f5748085780e1f3 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 24 May 2023 22:31:01 +0800 Subject: [PATCH 18/22] docs: add comments --- models/issues/pull.go | 1 + models/issues/review.go | 1 + 2 files changed, 2 insertions(+) diff --git a/models/issues/pull.go b/models/issues/pull.go index 8128f3abe52e5..9647f485c2490 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -316,6 +316,7 @@ func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { return nil } +// LoadRequestedReviewers loads the requested reviewers. func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { if len(pr.RequestedReviewers) > 0 { return nil diff --git a/models/issues/review.go b/models/issues/review.go index d050e2d6e7f7b..6cfd52411898d 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -162,6 +162,7 @@ func (r *Review) LoadReviewer(ctx context.Context) (err error) { return err } +// LoadReviewers loads reviewers func LoadReviewers(ctx context.Context, reviews []*Review) (err error) { reviewerIds := make([]int64, len(reviews)) for i := 0; i < len(reviews); i++ { From 018d95de84532c96b0aff88f39d9e0c8e101a4de Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 24 May 2023 23:37:24 +0800 Subject: [PATCH 19/22] fix: rename GetReviewersByIssueID to GetReviewsByIssueID --- models/issues/pull.go | 2 +- models/issues/review.go | 4 ++-- models/issues/review_test.go | 4 ++-- routers/web/repo/issue.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 9647f485c2490..58bc64cc0ba59 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -322,7 +322,7 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { return nil } - reviews, err := GetReviewersByIssueID(pr.Issue.ID) + reviews, err := GetReviewsByIssueID(pr.Issue.ID) if err != nil { return err } diff --git a/models/issues/review.go b/models/issues/review.go index 6cfd52411898d..06cf132a48a3a 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -541,8 +541,8 @@ func GetReviews(ctx context.Context, opts *GetReviewOptions) ([]*Review, error) return reviews, sess.Find(&reviews) } -// GetReviewersByIssueID gets the latest review of each reviewer for a pull request -func GetReviewersByIssueID(issueID int64) ([]*Review, error) { +// GetReviewsByIssueID gets the latest review of each reviewer for a pull request +func GetReviewsByIssueID(issueID int64) ([]*Review, error) { reviews := make([]*Review, 0, 10) sess := db.GetEngine(db.DefaultContext) diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 7fba8ca437dae..0cb621812c159 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -132,7 +132,7 @@ func TestGetReviewersByIssueID(t *testing.T) { UpdatedUnix: 946684814, }) - allReviews, err := issues_model.GetReviewersByIssueID(issue.ID) + allReviews, err := issues_model.GetReviewsByIssueID(issue.ID) assert.NoError(t, err) for _, review := range allReviews { assert.NoError(t, review.LoadReviewer(db.DefaultContext)) @@ -145,7 +145,7 @@ func TestGetReviewersByIssueID(t *testing.T) { } } - allReviews, err = issues_model.GetReviewersByIssueID(issue.ID) + allReviews, err = issues_model.GetReviewsByIssueID(issue.ID) assert.NoError(t, err) assert.NoError(t, issues_model.LoadReviewers(db.DefaultContext, allReviews)) if assert.Len(t, allReviews, 3) { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index c2f30a01f4736..f0347e3a6e2cc 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -573,7 +573,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is } ctx.Data["OriginalReviews"] = originalAuthorReviews - reviews, err := issues_model.GetReviewersByIssueID(issue.ID) + reviews, err := issues_model.GetReviewsByIssueID(issue.ID) if err != nil { ctx.ServerError("GetReviewersByIssueID", err) return From d5f9b9df68ca8d2d1fdb643334417346f6d33323 Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 24 May 2023 23:43:49 +0800 Subject: [PATCH 20/22] fix --- models/user/user.go | 10 +++++----- services/convert/pull.go | 6 ++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 824a322499d4a..a7f1a20bad847 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -21,6 +21,7 @@ import ( "code.gitea.io/gitea/modules/auth/openid" "code.gitea.io/gitea/modules/auth/password/hash" "code.gitea.io/gitea/modules/base" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -979,15 +980,14 @@ func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) { // GetPossibleUserByIDs returns the users if id > 0 or return system users if id < 0 func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { - uniqueIDs := make(map[int64]struct{}) + uniqueIDs := make(container.Set[int64], 32) + uniqueIDs.AddMultiple(ids...) + users := make([]*User, 0, len(ids)) - for _, id := range ids { - uniqueIDs[id] = struct{}{} - } + if _, ok := uniqueIDs[0]; ok { return nil, ErrUserNotExist{} } - if _, ok := uniqueIDs[-1]; ok { users = append(users, NewGhostUser()) delete(uniqueIDs, -1) diff --git a/services/convert/pull.go b/services/convert/pull.go index 7345117afeabc..598187ca6e0f2 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -92,10 +92,8 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u log.Error("LoadRequestedReviewers[%d]: %v", pr.ID, err) return nil } - if len(pr.RequestedReviewers) > 0 { - for _, reviewer := range pr.RequestedReviewers { - apiPullRequest.RequestedReviewers = append(apiPullRequest.RequestedReviewers, ToUser(ctx, reviewer, nil)) - } + for _, reviewer := range pr.RequestedReviewers { + apiPullRequest.RequestedReviewers = append(apiPullRequest.RequestedReviewers, ToUser(ctx, reviewer, nil)) } if pr.Issue.ClosedUnix != 0 { From 49b88884a9b99831e21fcdfa05e148562a9e295c Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Wed, 24 May 2023 23:58:56 +0800 Subject: [PATCH 21/22] fix: add translation keys --- options/locale/locale_en-US.ini | 2 ++ 1 file changed, 2 insertions(+) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index ccac8faf995a3..25c98836d9ef4 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2107,6 +2107,8 @@ settings.event_pull_request_review = Pull Request Reviewed settings.event_pull_request_review_desc = Pull request approved, rejected, or review comment. settings.event_pull_request_sync = Pull Request Synchronized settings.event_pull_request_sync_desc = Pull request synchronized. +settings.event_pull_request_review_request = Pull Request Review Requested +settings.event_pull_request_review_request_desc = Pull request review requested or review request removed. settings.event_pull_request_approvals = Pull Request Approvals settings.event_pull_request_merge = Pull Request Merge settings.event_package = Package From 063c813ed888b62346e0fdac76b37142fb7d428f Mon Sep 17 00:00:00 2001 From: xiaodongzhang Date: Thu, 25 May 2023 00:16:42 +0800 Subject: [PATCH 22/22] fix --- models/user/user.go | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index a7f1a20bad847..8d4ac55265f59 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -980,28 +980,16 @@ func GetPossibleUserByID(ctx context.Context, id int64) (*User, error) { // GetPossibleUserByIDs returns the users if id > 0 or return system users if id < 0 func GetPossibleUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { - uniqueIDs := make(container.Set[int64], 32) - uniqueIDs.AddMultiple(ids...) - + uniqueIDs := container.SetOf(ids...) users := make([]*User, 0, len(ids)) - - if _, ok := uniqueIDs[0]; ok { - return nil, ErrUserNotExist{} - } - if _, ok := uniqueIDs[-1]; ok { + _ = uniqueIDs.Remove(0) + if uniqueIDs.Remove(-1) { users = append(users, NewGhostUser()) - delete(uniqueIDs, -1) } - if _, ok := uniqueIDs[ActionsUserID]; ok { + if uniqueIDs.Remove(ActionsUserID) { users = append(users, NewActionsUser()) - delete(uniqueIDs, ActionsUserID) - } - - needFindIds := make([]int64, 0, len(uniqueIDs)) - for id := range uniqueIDs { - needFindIds = append(needFindIds, id) } - res, err := GetUserByIDs(ctx, needFindIds) + res, err := GetUserByIDs(ctx, uniqueIDs.Values()) if err != nil { return nil, err }