From 18aeca53203adba7b4fb3b7311f0e77bef92e266 Mon Sep 17 00:00:00 2001 From: Calvin K <70356237+CalK16@users.noreply.github.com> Date: Sat, 9 Nov 2024 12:48:31 +0800 Subject: [PATCH] Add reviewers selection to new pull request (#32403) Users could add reviewers when creating new PRs. --------- Co-authored-by: splitt3r Co-authored-by: Sebastian Sauer Co-authored-by: bb-ben <70356237+bboerben@users.noreply.github.com> Co-authored-by: wxiaoguang --- modules/structs/pull.go | 4 +- options/locale/locale_en-US.ini | 2 +- routers/api/v1/repo/pull.go | 14 +- routers/api/v1/repo/pull_review.go | 108 +++++----- routers/web/repo/compare.go | 4 + routers/web/repo/issue.go | 186 ++++++++++++------ routers/web/repo/pull.go | 17 +- services/agit/agit.go | 8 +- services/forms/repo_form.go | 1 + services/issue/assignee.go | 28 ++- services/mailer/mailer.go | 2 +- services/pull/pull.go | 27 ++- templates/repo/issue/new_form.tmpl | 6 +- .../repo/issue/sidebar/reviewer_list.tmpl | 174 ++++++++-------- templates/repo/issue/sidebar/wip_switch.tmpl | 2 +- .../repo/issue/view_content/sidebar.tmpl | 2 +- templates/swagger/v1_json.tmpl | 14 ++ tests/integration/actions_trigger_test.go | 6 +- tests/integration/api_pull_review_test.go | 5 +- tests/integration/pull_merge_test.go | 3 +- tests/integration/pull_update_test.go | 3 +- web_src/css/base.css | 1 + web_src/css/repo.css | 9 + .../features/repo-issue-sidebar-combolist.ts | 89 +++++++++ web_src/js/features/repo-issue-sidebar.ts | 41 ++-- web_src/js/features/repo-issue.ts | 12 -- 26 files changed, 500 insertions(+), 268 deletions(-) create mode 100644 web_src/js/features/repo-issue-sidebar-combolist.ts diff --git a/modules/structs/pull.go b/modules/structs/pull.go index ab627666c9..55831e642c 100644 --- a/modules/structs/pull.go +++ b/modules/structs/pull.go @@ -86,7 +86,9 @@ type CreatePullRequestOption struct { Milestone int64 `json:"milestone"` Labels []int64 `json:"labels"` // swagger:strfmt date-time - Deadline *time.Time `json:"due_date"` + Deadline *time.Time `json:"due_date"` + Reviewers []string `json:"reviewers"` + TeamReviewers []string `json:"team_reviewers"` } // EditPullRequestOption options when modify pull request diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 679e64b424..c3639fb72e 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1462,7 +1462,7 @@ issues.new.closed_milestone = Closed Milestones issues.new.assignees = Assignees issues.new.clear_assignees = Clear assignees issues.new.no_assignees = No Assignees -issues.new.no_reviewers = No reviewers +issues.new.no_reviewers = No Reviewers issues.new.blocked_user = Cannot create issue because you are blocked by the repository owner. issues.edit.already_changed = Unable to save changes to the issue. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes issues.edit.blocked_user = Cannot edit content because you are blocked by the poster or repository owner. diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 34ebcb42d5..28d7379f07 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -554,7 +554,19 @@ func CreatePullRequest(ctx *context.APIContext) { } } - if err := pull_service.NewPullRequest(ctx, repo, prIssue, labelIDs, []string{}, pr, assigneeIDs); err != nil { + prOpts := &pull_service.NewPullRequestOptions{ + Repo: repo, + Issue: prIssue, + LabelIDs: labelIDs, + PullRequest: pr, + AssigneeIDs: assigneeIDs, + } + prOpts.Reviewers, prOpts.TeamReviewers = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers) + if ctx.Written() { + return + } + + if err := pull_service.NewPullRequest(ctx, prOpts); err != nil { if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err) } else if errors.Is(err, user_model.ErrBlockedUser) { diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 34bbaf5600..def860eee8 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -656,6 +656,47 @@ func DeleteReviewRequests(ctx *context.APIContext) { apiReviewRequest(ctx, *opts, false) } +func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team) { + var err error + for _, r := range reviewerNames { + var reviewer *user_model.User + if strings.Contains(r, "@") { + reviewer, err = user_model.GetUserByEmail(ctx, r) + } else { + reviewer, err = user_model.GetUserByName(ctx, r) + } + + if err != nil { + if user_model.IsErrUserNotExist(err) { + ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r)) + return nil, nil + } + ctx.Error(http.StatusInternalServerError, "GetUser", err) + return nil, nil + } + + reviewers = append(reviewers, reviewer) + } + + 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) + 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 + } + + teamReviewers = append(teamReviewers, teamReviewer) + } + } + return reviewers, teamReviewers +} + func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions, isAdd bool) { pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64(":index")) if err != nil { @@ -672,42 +713,15 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions return } - reviewers := make([]*user_model.User, 0, len(opts.Reviewers)) - permDoer, err := access_model.GetUserRepoPermission(ctx, pr.Issue.Repo, ctx.Doer) if err != nil { ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) return } - for _, r := range opts.Reviewers { - var reviewer *user_model.User - if strings.Contains(r, "@") { - reviewer, err = user_model.GetUserByEmail(ctx, r) - } else { - reviewer, err = user_model.GetUserByName(ctx, r) - } - - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r)) - return - } - ctx.Error(http.StatusInternalServerError, "GetUser", err) - return - } - - err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, isAdd, pr.Issue, &permDoer) - if err != nil { - if issues_model.IsErrNotValidReviewRequest(err) { - ctx.Error(http.StatusUnprocessableEntity, "NotValidReviewRequest", err) - return - } - ctx.Error(http.StatusInternalServerError, "IsValidReviewRequest", err) - return - } - - reviewers = append(reviewers, reviewer) + reviewers, teamReviewers := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers) + if ctx.Written() { + return } var reviews []*issues_model.Review @@ -716,12 +730,16 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions } for _, reviewer := range reviewers { - comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, reviewer, isAdd) + comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd) 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, "ReviewRequest", err) return } @@ -736,35 +754,17 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions } if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { - teamReviewers := make([]*organization.Team, 0, len(opts.TeamReviewers)) - for _, t := range opts.TeamReviewers { - var teamReviewer *organization.Team - teamReviewer, err = organization.GetTeam(ctx, ctx.Repo.Owner.ID, t) + for _, teamReviewer := range teamReviewers { + comment, err := issue_service.TeamReviewRequest(ctx, pr.Issue, ctx.Doer, teamReviewer, isAdd) if err != nil { - if organization.IsErrTeamNotExist(err) { - ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t)) + if issues_model.IsErrReviewRequestOnClosedPR(err) { + ctx.Error(http.StatusForbidden, "", err) return } - ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) - return - } - - err = issue_service.IsValidTeamReviewRequest(ctx, teamReviewer, ctx.Doer, isAdd, pr.Issue) - if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { - ctx.Error(http.StatusUnprocessableEntity, "NotValidReviewRequest", err) + ctx.Error(http.StatusUnprocessableEntity, "", err) return } - ctx.Error(http.StatusInternalServerError, "IsValidTeamReviewRequest", err) - return - } - - teamReviewers = append(teamReviewers, teamReviewer) - } - - for _, teamReviewer := range teamReviewers { - comment, err := issue_service.TeamReviewRequest(ctx, pr.Issue, ctx.Doer, teamReviewer, isAdd) - if err != nil { ctx.ServerError("TeamReviewRequest", err) return } diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 3927972837..3477ba36e8 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -792,6 +792,10 @@ func CompareDiff(ctx *context.Context) { if ctx.Written() { return } + RetrieveRepoReviewers(ctx, ctx.Repo.Repository, nil, true) + if ctx.Written() { + return + } } } beforeCommitID := ctx.Data["BeforeCommitID"].(string) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index c4fc535446..7fa8d428d3 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -654,34 +654,66 @@ func retrieveProjects(ctx *context.Context, repo *repo_model.Repository) { // repoReviewerSelection items to bee shown type repoReviewerSelection struct { - IsTeam bool - Team *organization.Team - User *user_model.User - Review *issues_model.Review - CanChange bool - Checked bool - ItemID int64 + IsTeam bool + Team *organization.Team + User *user_model.User + Review *issues_model.Review + CanBeDismissed bool + CanChange bool + Requested bool + ItemID int64 } -// RetrieveRepoReviewers find all reviewers of a repository +type issueSidebarReviewersData struct { + Repository *repo_model.Repository + RepoOwnerName string + RepoLink string + IssueID int64 + CanChooseReviewer bool + OriginalReviews issues_model.ReviewList + TeamReviewers []*repoReviewerSelection + Reviewers []*repoReviewerSelection + CurrentPullReviewers []*repoReviewerSelection +} + +// RetrieveRepoReviewers find all reviewers of a repository. If issue is nil, it means the doer is creating a new PR. func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, issue *issues_model.Issue, canChooseReviewer bool) { - ctx.Data["CanChooseReviewer"] = canChooseReviewer + data := &issueSidebarReviewersData{} + data.RepoLink = ctx.Repo.RepoLink + data.Repository = repo + data.RepoOwnerName = repo.OwnerName + data.CanChooseReviewer = canChooseReviewer - originalAuthorReviews, err := issues_model.GetReviewersFromOriginalAuthorsByIssueID(ctx, issue.ID) - if err != nil { - ctx.ServerError("GetReviewersFromOriginalAuthorsByIssueID", err) - return - } - ctx.Data["OriginalReviews"] = originalAuthorReviews + var posterID int64 + var isClosed bool + var reviews issues_model.ReviewList - reviews, err := issues_model.GetReviewsByIssueID(ctx, issue.ID) - if err != nil { - ctx.ServerError("GetReviewersByIssueID", err) - return - } + if issue == nil { + posterID = ctx.Doer.ID + } else { + posterID = issue.PosterID + if issue.OriginalAuthorID > 0 { + posterID = 0 // for migrated PRs, no poster ID + } - if len(reviews) == 0 && !canChooseReviewer { - return + data.IssueID = issue.ID + isClosed = issue.IsClosed || issue.PullRequest.HasMerged + + originalAuthorReviews, err := issues_model.GetReviewersFromOriginalAuthorsByIssueID(ctx, issue.ID) + if err != nil { + ctx.ServerError("GetReviewersFromOriginalAuthorsByIssueID", err) + return + } + data.OriginalReviews = originalAuthorReviews + + reviews, err = issues_model.GetReviewsByIssueID(ctx, issue.ID) + if err != nil { + ctx.ServerError("GetReviewersByIssueID", err) + return + } + if len(reviews) == 0 && !canChooseReviewer { + return + } } var ( @@ -693,11 +725,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is ) if canChooseReviewer { - posterID := issue.PosterID - if issue.OriginalAuthorID > 0 { - posterID = 0 - } - + var err error reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID) if err != nil { ctx.ServerError("GetReviewers", err) @@ -723,9 +751,9 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is for _, review := range reviews { tmp := &repoReviewerSelection{ - Checked: review.Type == issues_model.ReviewTypeRequest, - Review: review, - ItemID: review.ReviewerID, + Requested: review.Type == issues_model.ReviewTypeRequest, + Review: review, + ItemID: review.ReviewerID, } if review.ReviewerTeamID > 0 { tmp.IsTeam = true @@ -756,7 +784,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is currentPullReviewers := make([]*repoReviewerSelection, 0, len(pullReviews)) for _, item := range pullReviews { if item.Review.ReviewerID > 0 { - if err = item.Review.LoadReviewer(ctx); err != nil { + if err := item.Review.LoadReviewer(ctx); err != nil { if user_model.IsErrUserNotExist(err) { continue } @@ -765,7 +793,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is } item.User = item.Review.Reviewer } else if item.Review.ReviewerTeamID > 0 { - if err = item.Review.LoadReviewerTeam(ctx); err != nil { + if err := item.Review.LoadReviewerTeam(ctx); err != nil { if organization.IsErrTeamNotExist(err) { continue } @@ -776,10 +804,11 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is } else { continue } - + item.CanBeDismissed = ctx.Repo.Permission.IsAdmin() && !isClosed && + (item.Review.Type == issues_model.ReviewTypeApprove || item.Review.Type == issues_model.ReviewTypeReject) currentPullReviewers = append(currentPullReviewers, item) } - ctx.Data["PullReviewers"] = currentPullReviewers + data.CurrentPullReviewers = currentPullReviewers } if canChooseReviewer && reviewersResult != nil { @@ -807,7 +836,7 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is }) } - ctx.Data["Reviewers"] = reviewersResult + data.Reviewers = reviewersResult } if canChooseReviewer && teamReviewersResult != nil { @@ -835,8 +864,10 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is }) } - ctx.Data["TeamReviewers"] = teamReviewersResult + data.TeamReviewers = teamReviewersResult } + + ctx.Data["IssueSidebarReviewersData"] = data } // RetrieveRepoMetas find all the meta information of a repository @@ -1117,7 +1148,14 @@ func DeleteIssue(ctx *context.Context) { } // ValidateRepoMetas check and returns repository's meta information -func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull bool) ([]int64, []int64, int64, int64) { +func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull bool) (ret struct { + LabelIDs, AssigneeIDs []int64 + MilestoneID, ProjectID int64 + + Reviewers []*user_model.User + TeamReviewers []*organization.Team +}, +) { var ( repo = ctx.Repo.Repository err error @@ -1125,7 +1163,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull labels := RetrieveRepoMetas(ctx, ctx.Repo.Repository, isPull) if ctx.Written() { - return nil, nil, 0, 0 + return ret } var labelIDs []int64 @@ -1134,7 +1172,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull if len(form.LabelIDs) > 0 { labelIDs, err = base.StringsToInt64s(strings.Split(form.LabelIDs, ",")) if err != nil { - return nil, nil, 0, 0 + return ret } labelIDMark := make(container.Set[int64]) labelIDMark.AddMultiple(labelIDs...) @@ -1157,11 +1195,11 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull milestone, err := issues_model.GetMilestoneByRepoID(ctx, ctx.Repo.Repository.ID, milestoneID) if err != nil { ctx.ServerError("GetMilestoneByID", err) - return nil, nil, 0, 0 + return ret } if milestone.RepoID != repo.ID { ctx.ServerError("GetMilestoneByID", err) - return nil, nil, 0, 0 + return ret } ctx.Data["Milestone"] = milestone ctx.Data["milestone_id"] = milestoneID @@ -1171,11 +1209,11 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull p, err := project_model.GetProjectByID(ctx, form.ProjectID) if err != nil { ctx.ServerError("GetProjectByID", err) - return nil, nil, 0, 0 + return ret } if p.RepoID != ctx.Repo.Repository.ID && p.OwnerID != ctx.Repo.Repository.OwnerID { ctx.NotFound("", nil) - return nil, nil, 0, 0 + return ret } ctx.Data["Project"] = p @@ -1187,7 +1225,7 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull if len(form.AssigneeIDs) > 0 { assigneeIDs, err = base.StringsToInt64s(strings.Split(form.AssigneeIDs, ",")) if err != nil { - return nil, nil, 0, 0 + return ret } // Check if the passed assignees actually exists and is assignable @@ -1195,18 +1233,18 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull assignee, err := user_model.GetUserByID(ctx, aID) if err != nil { ctx.ServerError("GetUserByID", err) - return nil, nil, 0, 0 + return ret } valid, err := access_model.CanBeAssigned(ctx, assignee, repo, isPull) if err != nil { ctx.ServerError("CanBeAssigned", err) - return nil, nil, 0, 0 + return ret } if !valid { ctx.ServerError("canBeAssigned", repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name}) - return nil, nil, 0, 0 + return ret } } } @@ -1216,7 +1254,39 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull assigneeIDs = append(assigneeIDs, form.AssigneeID) } - return labelIDs, assigneeIDs, milestoneID, form.ProjectID + // Check reviewers + var reviewers []*user_model.User + var teamReviewers []*organization.Team + if isPull && len(form.ReviewerIDs) > 0 { + reviewerIDs, err := base.StringsToInt64s(strings.Split(form.ReviewerIDs, ",")) + if err != nil { + return ret + } + // Check if the passed reviewers (user/team) actually exist + for _, rID := range reviewerIDs { + // negative reviewIDs represent team requests + if rID < 0 { + teamReviewer, err := organization.GetTeamByID(ctx, -rID) + if err != nil { + ctx.ServerError("GetTeamByID", err) + return ret + } + teamReviewers = append(teamReviewers, teamReviewer) + continue + } + + reviewer, err := user_model.GetUserByID(ctx, rID) + if err != nil { + ctx.ServerError("GetUserByID", err) + return ret + } + reviewers = append(reviewers, reviewer) + } + } + + ret.LabelIDs, ret.AssigneeIDs, ret.MilestoneID, ret.ProjectID = labelIDs, assigneeIDs, milestoneID, form.ProjectID + ret.Reviewers, ret.TeamReviewers = reviewers, teamReviewers + return ret } // NewIssuePost response for creating new issue @@ -1234,11 +1304,13 @@ func NewIssuePost(ctx *context.Context) { attachments []string ) - labelIDs, assigneeIDs, milestoneID, projectID := ValidateRepoMetas(ctx, *form, false) + validateRet := ValidateRepoMetas(ctx, *form, false) if ctx.Written() { return } + labelIDs, assigneeIDs, milestoneID, projectID := validateRet.LabelIDs, validateRet.AssigneeIDs, validateRet.MilestoneID, validateRet.ProjectID + if projectID > 0 { if !ctx.Repo.CanRead(unit.TypeProjects) { // User must also be able to see the project. @@ -2479,7 +2551,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - err = issue_service.IsValidTeamReviewRequest(ctx, team, ctx.Doer, action == "attach", issue) + _, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -2490,12 +2562,6 @@ func UpdatePullReviewRequest(ctx *context.Context) { ctx.Status(http.StatusForbidden) return } - ctx.ServerError("IsValidTeamReviewRequest", err) - return - } - - _, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") - if err != nil { ctx.ServerError("TeamReviewRequest", err) return } @@ -2517,7 +2583,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, action == "attach", issue, nil) + _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -2528,12 +2594,6 @@ func UpdatePullReviewRequest(ctx *context.Context) { ctx.Status(http.StatusForbidden) return } - ctx.ServerError("isValidReviewRequest", err) - return - } - - _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, reviewer, action == "attach") - if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Status(http.StatusForbidden) return diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index cc554a71ff..dd9671efbe 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1269,11 +1269,13 @@ func CompareAndPullRequestPost(ctx *context.Context) { return } - labelIDs, assigneeIDs, milestoneID, projectID := ValidateRepoMetas(ctx, *form, true) + validateRet := ValidateRepoMetas(ctx, *form, true) if ctx.Written() { return } + labelIDs, assigneeIDs, milestoneID, projectID := validateRet.LabelIDs, validateRet.AssigneeIDs, validateRet.MilestoneID, validateRet.ProjectID + if setting.Attachment.Enabled { attachments = form.Files } @@ -1318,8 +1320,17 @@ func CompareAndPullRequestPost(ctx *context.Context) { } // FIXME: check error in the case two people send pull request at almost same time, give nice error prompt // instead of 500. - - if err := pull_service.NewPullRequest(ctx, repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs); err != nil { + prOpts := &pull_service.NewPullRequestOptions{ + Repo: repo, + Issue: pullIssue, + LabelIDs: labelIDs, + AttachmentUUIDs: attachments, + PullRequest: pullRequest, + AssigneeIDs: assigneeIDs, + Reviewers: validateRet.Reviewers, + TeamReviewers: validateRet.TeamReviewers, + } + if err := pull_service.NewPullRequest(ctx, prOpts); err != nil { switch { case repo_model.IsErrUserDoesNotHaveAccessToRepo(err): ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err.Error()) diff --git a/services/agit/agit.go b/services/agit/agit.go index 82aa2791aa..83b12dfcdb 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -137,8 +137,12 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. Type: issues_model.PullRequestGitea, Flow: issues_model.PullRequestFlowAGit, } - - if err := pull_service.NewPullRequest(ctx, repo, prIssue, []int64{}, []string{}, pr, []int64{}); err != nil { + prOpts := &pull_service.NewPullRequestOptions{ + Repo: repo, + Issue: prIssue, + PullRequest: pr, + } + if err := pull_service.NewPullRequest(ctx, prOpts); err != nil { return nil, err } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index ddd07a64cb..83f2dd6caa 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -447,6 +447,7 @@ type CreateIssueForm struct { Title string `binding:"Required;MaxSize(255)"` LabelIDs string `form:"label_ids"` AssigneeIDs string `form:"assignee_ids"` + ReviewerIDs string `form:"reviewer_ids"` Ref string `form:"ref"` MilestoneID int64 ProjectID int64 diff --git a/services/issue/assignee.go b/services/issue/assignee.go index a0aa5a339b..52ee9f2b22 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -61,7 +61,12 @@ 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, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { +func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { + err = isValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) + if err != nil { + return nil, err + } + if isAdd { comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) } else { @@ -79,8 +84,8 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewe return comment, err } -// IsValidReviewRequest Check permission for ReviewRequest -func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { +// 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", @@ -109,7 +114,7 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } - lastreview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) + lastReview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) if err != nil && !issues_model.IsErrReviewNotExist(err) { return err } @@ -137,7 +142,7 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, return nil } - if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != issues_model.ReviewTypeRequest { + if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastReview != nil && lastReview.Type != issues_model.ReviewTypeRequest { return nil } @@ -152,7 +157,7 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, return nil } - if lastreview != nil && lastreview.Type == issues_model.ReviewTypeRequest && lastreview.ReviewerID == doer.ID { + if lastReview != nil && lastReview.Type == issues_model.ReviewTypeRequest && lastReview.ReviewerID == doer.ID { return nil } @@ -163,8 +168,8 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, } } -// 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 { +// 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", @@ -212,6 +217,10 @@ 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) + if err != nil { + return nil, err + } if isAdd { comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) } else { @@ -268,6 +277,9 @@ func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doe // 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, issue *issues_model.Issue) bool { + if repo.IsArchived { + return false + } // The poster of the PR can change the reviewers if doer.ID == issue.PosterID { return true diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index c5846e6104..5cb6d03521 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -382,7 +382,7 @@ func (s *dummySender) Send(from string, to []string, msg io.WriterTo) error { if _, err := msg.WriteTo(&buf); err != nil { return err } - log.Info("Mail From: %s To: %v Body: %s", from, to, buf.String()) + log.Debug("Mail From: %s To: %v Body: %s", from, to, buf.String()) return nil } diff --git a/services/pull/pull.go b/services/pull/pull.go index bab4e49998..3362cb97ff 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -17,6 +17,7 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/organization" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -41,8 +42,20 @@ func getPullWorkingLockKey(prID int64) string { return fmt.Sprintf("pull_working_%d", prID) } +type NewPullRequestOptions struct { + Repo *repo_model.Repository + Issue *issues_model.Issue + LabelIDs []int64 + AttachmentUUIDs []string + PullRequest *issues_model.PullRequest + AssigneeIDs []int64 + Reviewers []*user_model.User + TeamReviewers []*organization.Team +} + // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { +func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { + repo, issue, labelIDs, uuids, pr, assigneeIDs := opts.Repo, opts.Issue, opts.LabelIDs, opts.AttachmentUUIDs, opts.PullRequest, opts.AssigneeIDs if err := issue.LoadPoster(ctx); err != nil { return err } @@ -197,7 +210,17 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID]) } - + permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) + for _, reviewer := range opts.Reviewers { + if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { + return err + } + } + for _, teamReviewer := range opts.TeamReviewers { + if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil { + return err + } + } return nil } diff --git a/templates/repo/issue/new_form.tmpl b/templates/repo/issue/new_form.tmpl index 6f1bebc032..190d52cf47 100644 --- a/templates/repo/issue/new_form.tmpl +++ b/templates/repo/issue/new_form.tmpl @@ -47,7 +47,11 @@
- {{template "repo/issue/branch_selector_field" .}} + {{template "repo/issue/branch_selector_field" $}} + {{if .PageIsComparePull}} + {{template "repo/issue/sidebar/reviewer_list" dict "IssueSidebarReviewersData" $.IssueSidebarReviewersData}} +
+ {{end}} {{template "repo/issue/labels/labels_selector_field" .}} diff --git a/templates/repo/issue/sidebar/reviewer_list.tmpl b/templates/repo/issue/sidebar/reviewer_list.tmpl index cf4d067c0f..2d3218e927 100644 --- a/templates/repo/issue/sidebar/reviewer_list.tmpl +++ b/templates/repo/issue/sidebar/reviewer_list.tmpl @@ -1,95 +1,79 @@ - -
- - {{ctx.Locale.Tr "repo.issues.review.reviewers"}} - {{if and .CanChooseReviewer (not .Repository.IsArchived)}} - {{svg "octicon-gear" 16 "tw-ml-1"}} - {{end}} - -
- {{if .Reviewers}} -
- {{svg "octicon-search" 16}} - -
- {{end}} - {{if .Reviewers}} - {{range .Reviewers}} +{{$data := .IssueSidebarReviewersData}} +{{$hasCandidates := or $data.Reviewers $data.TeamReviewers}} +
+ {{/* match CreateIssueForm */}} +
+ + {{ctx.Locale.Tr "repo.issues.review.reviewers"}} {{if and $data.CanChooseReviewer}}{{svg "octicon-gear"}}{{end}} + +
+ {{if $hasCandidates}} +
+ {{svg "octicon-search"}} + +
+ {{end}} + {{range $data.Reviewers}} {{if .User}} - - {{svg "octicon-check"}} - - {{ctx.AvatarUtils.Avatar .User 28 "tw-mr-2"}}{{template "repo/search_name" .User}} - + + {{svg "octicon-check"}} {{ctx.AvatarUtils.Avatar .User 20}} {{template "repo/search_name" .User}} {{end}} {{end}} - {{end}} - {{if .TeamReviewers}} - {{if .Reviewers}} -
- {{end}} - {{range .TeamReviewers}} - {{if .Team}} - - {{svg "octicon-check" 16}} - - {{svg "octicon-people" 16 "tw-ml-4 tw-mr-1"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}} - - + {{if $data.TeamReviewers}} + {{if $data.Reviewers}}
{{end}} + {{range $data.TeamReviewers}} + {{if .Team}} + + {{svg "octicon-check"}} {{svg "octicon-people" 20}} {{$data.RepoOwnerName}}/{{.Team.Name}} + + {{end}} {{end}} {{end}} - {{end}} +
-
-
- {{ctx.Locale.Tr "repo.issues.new.no_reviewers"}} -
- {{range .PullReviewers}} -
-
+
+ + {{ctx.Locale.Tr "repo.issues.new.no_reviewers"}} + + {{range $data.CurrentPullReviewers}} +
+
{{if .User}} - {{ctx.AvatarUtils.Avatar .User 20 "tw-mr-2"}}{{.User.GetDisplayName}} + {{ctx.AvatarUtils.Avatar .User 20}} {{.User.GetDisplayName}} {{else if .Team}} - {{svg "octicon-people" 20 "tw-mr-2"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}} + {{svg "octicon-people" 20}} {{$data.RepoOwnerName}}/{{.Team.Name}} {{end}}
-
- {{if (and $.Permission.IsAdmin (or (eq .Review.Type 1) (eq .Review.Type 3)) (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged))}} - +
+ {{if .CanBeDismissed}} + {{svg "octicon-x" 20}} - {{end}} {{if .Review.Stale}} - - {{svg "octicon-hourglass" 16}} - + {{svg "octicon-hourglass" 16}} {{end}} - {{if and .CanChange (or .Checked (and (not $.Issue.IsClosed) (not $.Issue.PullRequest.HasMerged)))}} - {{svg (Iif .Checked "octicon-trash" "octicon-sync")}} + {{if and .CanChange $data.CanChooseReviewer}} + {{if .Requested}} + + {{svg "octicon-trash"}} + + {{else}} + + {{svg "octicon-sync"}} + + {{end}} {{end}} {{svg (printf "octicon-%s" .Review.Type.Icon) 16 (printf "text %s" (.Review.HTMLTypeColorName))}} @@ -97,15 +81,16 @@
{{end}} - {{range .OriginalReviews}} -
-
- - {{svg (MigrationIcon $.Repository.GetOriginalURLHostname) 20 "tw-mr-2"}} - {{.OriginalAuthor}} + {{range $data.OriginalReviews}} +
+
+ {{$originalURLHostname := $data.Repository.GetOriginalURLHostname}} + {{$originalURL := $data.Repository.OriginalURL}} + + {{svg (MigrationIcon $originalURLHostname) 20}} {{.OriginalAuthor}}
-
+
{{svg (printf "octicon-%s" .Type.Icon) 16 (printf "text %s" (.HTMLTypeColorName))}} @@ -113,4 +98,29 @@
{{end}}
+ + {{if $data.CurrentPullReviewers}} + + {{end}}
diff --git a/templates/repo/issue/sidebar/wip_switch.tmpl b/templates/repo/issue/sidebar/wip_switch.tmpl index 2f8994673e..06a3be0d8f 100644 --- a/templates/repo/issue/sidebar/wip_switch.tmpl +++ b/templates/repo/issue/sidebar/wip_switch.tmpl @@ -1,5 +1,5 @@ {{if and (or .HasIssuesOrPullsWritePermission .IsIssuePoster) (not .HasMerged) (not .Issue.IsClosed) (not .IsPullWorkInProgress)}} -
+
{{ctx.Locale.Tr "repo.pulls.still_in_progress"}} {{ctx.Locale.Tr "repo.pulls.add_prefix" (index .PullRequestWorkInProgressPrefixes 0)}} diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 7afb76968a..7a40274759 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -2,7 +2,7 @@ {{template "repo/issue/branch_selector_field" $}} {{if .Issue.IsPull}} - {{template "repo/issue/sidebar/reviewer_list" $}} + {{template "repo/issue/sidebar/reviewer_list" dict "IssueSidebarReviewersData" $.IssueSidebarReviewersData}} {{template "repo/issue/sidebar/wip_switch" $}}
{{end}} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index b9fb1910a3..8fed15b516 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -20095,6 +20095,20 @@ "format": "int64", "x-go-name": "Milestone" }, + "reviewers": { + "type": "array", + "items": { + "type": "string" + }, + "x-go-name": "Reviewers" + }, + "team_reviewers": { + "type": "array", + "items": { + "type": "string" + }, + "x-go-name": "TeamReviewers" + }, "title": { "type": "string", "x-go-name": "Title" diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index c254c90958..4718aa7e73 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -145,7 +145,8 @@ func TestPullRequestTargetEvent(t *testing.T) { BaseRepo: baseRepo, Type: issues_model.PullRequestGitea, } - err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + prOpts := &pull_service.NewPullRequestOptions{Repo: baseRepo, Issue: pullIssue, PullRequest: pullRequest} + err = pull_service.NewPullRequest(git.DefaultContext, prOpts) assert.NoError(t, err) // load and compare ActionRun @@ -199,7 +200,8 @@ func TestPullRequestTargetEvent(t *testing.T) { BaseRepo: baseRepo, Type: issues_model.PullRequestGitea, } - err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + prOpts = &pull_service.NewPullRequestOptions{Repo: baseRepo, Issue: pullIssue, PullRequest: pullRequest} + err = pull_service.NewPullRequest(git.DefaultContext, prOpts) assert.NoError(t, err) // the new pull request cannot trigger actions, so there is still only 1 record diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index cadb0765c3..ba6b62d0d7 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" + access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -422,7 +423,9 @@ func TestAPIPullReviewStayDismissed(t *testing.T) { pullIssue.ID, user8.ID, 1, 1, 2, false) // user8 dismiss review - _, err = issue_service.ReviewRequest(db.DefaultContext, pullIssue, user8, user8, false) + 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, err) reviewsCountCheck(t, diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 43210e852e..2351e169bc 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -520,7 +520,8 @@ func TestConflictChecking(t *testing.T) { BaseRepo: baseRepo, Type: issues_model.PullRequestGitea, } - err = pull.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + prOpts := &pull.NewPullRequestOptions{Repo: baseRepo, Issue: pullIssue, PullRequest: pullRequest} + err = pull.NewPullRequest(git.DefaultContext, prOpts) assert.NoError(t, err) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"}) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 5ae241f3af..dfe47c1053 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -173,7 +173,8 @@ func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_mod BaseRepo: baseRepo, Type: issues_model.PullRequestGitea, } - err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil) + prOpts := &pull_service.NewPullRequestOptions{Repo: baseRepo, Issue: pullIssue, PullRequest: pullRequest} + err = pull_service.NewPullRequest(git.DefaultContext, prOpts) assert.NoError(t, err) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "Test Pull -to-update-"}) diff --git a/web_src/css/base.css b/web_src/css/base.css index 8d9f810ef8..b5a39c7af6 100644 --- a/web_src/css/base.css +++ b/web_src/css/base.css @@ -1381,6 +1381,7 @@ table th[data-sortt-desc] .svg { align-items: stretch; } +.ui.list.flex-items-block > .item, .flex-items-block > .item, .flex-text-block { display: flex; diff --git a/web_src/css/repo.css b/web_src/css/repo.css index 61aa99d531..185a5f6f55 100644 --- a/web_src/css/repo.css +++ b/web_src/css/repo.css @@ -50,6 +50,15 @@ width: 300px; } +.issue-sidebar-combo .ui.dropdown .item:not(.checked) svg.octicon-check { + visibility: hidden; +} +/* ideally, we should move these styles to ".ui.dropdown .menu.flex-items-menu > .item ...", could be done later */ +.issue-sidebar-combo .ui.dropdown .menu > .item > img, +.issue-sidebar-combo .ui.dropdown .menu > .item > svg { + margin: 0; +} + .issue-content-right .dropdown > .menu { max-width: 270px; min-width: 0; diff --git a/web_src/js/features/repo-issue-sidebar-combolist.ts b/web_src/js/features/repo-issue-sidebar-combolist.ts new file mode 100644 index 0000000000..d541615988 --- /dev/null +++ b/web_src/js/features/repo-issue-sidebar-combolist.ts @@ -0,0 +1,89 @@ +import {fomanticQuery} from '../modules/fomantic/base.ts'; +import {POST} from '../modules/fetch.ts'; +import {queryElemChildren, toggleElem} from '../utils/dom.ts'; + +// if there are draft comments, confirm before reloading, to avoid losing comments +export function issueSidebarReloadConfirmDraftComment() { + const commentTextareas = [ + document.querySelector('.edit-content-zone:not(.tw-hidden) textarea'), + document.querySelector('#comment-form textarea'), + ]; + for (const textarea of commentTextareas) { + // Most users won't feel too sad if they lose a comment with 10 chars, they can re-type these in seconds. + // But if they have typed more (like 50) chars and the comment is lost, they will be very unhappy. + if (textarea && textarea.value.trim().length > 10) { + textarea.parentElement.scrollIntoView(); + if (!window.confirm('Page will be reloaded, but there are draft comments. Continuing to reload will discard the comments. Continue?')) { + return; + } + break; + } + } + window.location.reload(); +} + +function collectCheckedValues(elDropdown: HTMLElement) { + return Array.from(elDropdown.querySelectorAll('.menu > .item.checked'), (el) => el.getAttribute('data-value')); +} + +export function initIssueSidebarComboList(container: HTMLElement) { + if (!container) return; + + const updateUrl = container.getAttribute('data-update-url'); + const elDropdown = container.querySelector(':scope > .ui.dropdown'); + const elList = container.querySelector(':scope > .ui.list'); + const elComboValue = container.querySelector(':scope > .combo-value'); + const initialValues = collectCheckedValues(elDropdown); + + elDropdown.addEventListener('click', (e) => { + const elItem = (e.target as HTMLElement).closest('.item'); + if (!elItem) return; + e.preventDefault(); + if (elItem.getAttribute('data-can-change') !== 'true') return; + elItem.classList.toggle('checked'); + elComboValue.value = collectCheckedValues(elDropdown).join(','); + }); + + const updateToBackend = async (changedValues) => { + let changed = false; + for (const value of initialValues) { + if (!changedValues.includes(value)) { + await POST(updateUrl, {data: new URLSearchParams({action: 'detach', id: value})}); + changed = true; + } + } + for (const value of changedValues) { + if (!initialValues.includes(value)) { + await POST(updateUrl, {data: new URLSearchParams({action: 'attach', id: value})}); + changed = true; + } + } + if (changed) issueSidebarReloadConfirmDraftComment(); + }; + + const syncList = (changedValues) => { + const elEmptyTip = elList.querySelector('.item.empty-list'); + queryElemChildren(elList, '.item:not(.empty-list)', (el) => el.remove()); + for (const value of changedValues) { + const el = elDropdown.querySelector(`.menu > .item[data-value="${value}"]`); + const listItem = el.cloneNode(true) as HTMLElement; + listItem.querySelector('svg.octicon-check')?.remove(); + elList.append(listItem); + } + const hasItems = Boolean(elList.querySelector('.item:not(.empty-list)')); + toggleElem(elEmptyTip, !hasItems); + }; + + fomanticQuery(elDropdown).dropdown({ + action: 'nothing', // do not hide the menu if user presses Enter + fullTextSearch: 'exact', + async onHide() { + const changedValues = collectCheckedValues(elDropdown); + if (updateUrl) { + await updateToBackend(changedValues); // send requests to backend and reload the page + } else { + syncList(changedValues); // only update the list in the sidebar + } + }, + }); +} diff --git a/web_src/js/features/repo-issue-sidebar.ts b/web_src/js/features/repo-issue-sidebar.ts index 0d30d8103c..4a1ef02aab 100644 --- a/web_src/js/features/repo-issue-sidebar.ts +++ b/web_src/js/features/repo-issue-sidebar.ts @@ -4,26 +4,7 @@ import {updateIssuesMeta} from './repo-common.ts'; import {svg} from '../svg.ts'; import {htmlEscape} from 'escape-goat'; import {toggleElem} from '../utils/dom.ts'; - -// if there are draft comments, confirm before reloading, to avoid losing comments -function reloadConfirmDraftComment() { - const commentTextareas = [ - document.querySelector('.edit-content-zone:not(.tw-hidden) textarea'), - document.querySelector('#comment-form textarea'), - ]; - for (const textarea of commentTextareas) { - // Most users won't feel too sad if they lose a comment with 10 chars, they can re-type these in seconds. - // But if they have typed more (like 50) chars and the comment is lost, they will be very unhappy. - if (textarea && textarea.value.trim().length > 10) { - textarea.parentElement.scrollIntoView(); - if (!window.confirm('Page will be reloaded, but there are draft comments. Continuing to reload will discard the comments. Continue?')) { - return; - } - break; - } - } - window.location.reload(); -} +import {initIssueSidebarComboList, issueSidebarReloadConfirmDraftComment} from './repo-issue-sidebar-combolist.ts'; function initBranchSelector() { const elSelectBranch = document.querySelector('.ui.dropdown.select-branch'); @@ -78,7 +59,7 @@ function initListSubmits(selector, outerSelector) { ); } if (itemEntries.length) { - reloadConfirmDraftComment(); + issueSidebarReloadConfirmDraftComment(); } } }, @@ -142,7 +123,7 @@ function initListSubmits(selector, outerSelector) { // TODO: Which thing should be done for choosing review requests // to make chosen items be shown on time here? - if (selector === 'select-reviewers-modify' || selector === 'select-assignees-modify') { + if (selector === 'select-assignees-modify') { return false; } @@ -173,7 +154,7 @@ function initListSubmits(selector, outerSelector) { $listMenu.data('issue-id'), '', ); - reloadConfirmDraftComment(); + issueSidebarReloadConfirmDraftComment(); })(); } @@ -182,7 +163,7 @@ function initListSubmits(selector, outerSelector) { $(this).find('.octicon-check').addClass('tw-invisible'); }); - if (selector === 'select-reviewers-modify' || selector === 'select-assignees-modify') { + if (selector === 'select-assignees-modify') { return false; } @@ -213,7 +194,7 @@ function selectItem(select_id, input_id) { $menu.data('issue-id'), $(this).data('id'), ); - reloadConfirmDraftComment(); + issueSidebarReloadConfirmDraftComment(); })(); } @@ -249,7 +230,7 @@ function selectItem(select_id, input_id) { $menu.data('issue-id'), $(this).data('id'), ); - reloadConfirmDraftComment(); + issueSidebarReloadConfirmDraftComment(); })(); } @@ -276,14 +257,14 @@ export function initRepoIssueSidebar() { initBranchSelector(); initRepoIssueDue(); - // Init labels and assignees + // TODO: refactor the legacy initListSubmits&selectItem to initIssueSidebarComboList initListSubmits('select-label', 'labels'); initListSubmits('select-assignees', 'assignees'); initListSubmits('select-assignees-modify', 'assignees'); - initListSubmits('select-reviewers-modify', 'assignees'); - - // Milestone, Assignee, Project selectItem('.select-project', '#project_id'); selectItem('.select-milestone', '#milestone_id'); selectItem('.select-assignee', '#assignee_id'); + + // init the combo list: a dropdown for selecting reviewers, and a list for showing selected reviewers and related actions + initIssueSidebarComboList(document.querySelector('.issue-sidebar-combo[data-sidebar-combo-for="reviewers"]')); } diff --git a/web_src/js/features/repo-issue.ts b/web_src/js/features/repo-issue.ts index 721a746aa2..92916ec8d7 100644 --- a/web_src/js/features/repo-issue.ts +++ b/web_src/js/features/repo-issue.ts @@ -8,7 +8,6 @@ import {parseIssuePageInfo, toAbsoluteUrl} from '../utils.ts'; import {GET, POST} from '../modules/fetch.ts'; import {showErrorToast} from '../modules/toast.ts'; import {initRepoIssueSidebar} from './repo-issue-sidebar.ts'; -import {updateIssuesMeta} from './repo-common.ts'; const {appSubUrl} = window.config; @@ -326,17 +325,6 @@ export function initRepoIssueWipTitle() { export function initRepoIssueComments() { if (!$('.repository.view.issue .timeline').length) return; - $('.re-request-review').on('click', async function (e) { - e.preventDefault(); - const url = this.getAttribute('data-update-url'); - const issueId = this.getAttribute('data-issue-id'); - const id = this.getAttribute('data-id'); - const isChecked = this.classList.contains('checked'); - - await updateIssuesMeta(url, isChecked ? 'detach' : 'attach', issueId, id); - window.location.reload(); - }); - document.addEventListener('click', (e) => { const urlTarget = document.querySelector(':target'); if (!urlTarget) return; -- 2.39.5