Refactor sidebar label selector (#32460)

Introduce `issueSidebarLabelsData` to handle all sidebar labels related data.

Author: wxiaoguang

routers/web/repo/compare.go

@@ -788,14 +788,22 @@ func CompareDiff(ctx *context.Context) {
 
 		if !nothingToCompare {
 			// Setup information for new form.
-			RetrieveRepoMetas(ctx, ctx.Repo.Repository, true)
+			retrieveRepoMetasForIssueWriter(ctx, ctx.Repo.Repository, true)
+			if ctx.Written() {
+				return
+			}
+			labelsData := retrieveRepoLabels(ctx, ctx.Repo.Repository, 0, true)
 			if ctx.Written() {
 				return
 			}
 			RetrieveRepoReviewers(ctx, ctx.Repo.Repository, nil, true)
 			if ctx.Written() {
 				return
 			}
+			_, templateErrs := setTemplateIfExists(ctx, pullRequestTemplateKey, pullRequestTemplateCandidates, labelsData)
+			if len(templateErrs) > 0 {
+				ctx.Flash.Warning(renderErrorOfTemplates(ctx, templateErrs), true)
+			}
 		}
 	}
 	beforeCommitID := ctx.Data["BeforeCommitID"].(string)
@@ -808,11 +816,6 @@ func CompareDiff(ctx *context.Context) {
 	ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + separator + base.ShortSha(afterCommitID)
 
 	ctx.Data["IsDiffCompare"] = true
-	_, templateErrs := setTemplateIfExists(ctx, pullRequestTemplateKey, pullRequestTemplateCandidates)
-
-	if len(templateErrs) > 0 {
-		ctx.Flash.Warning(renderErrorOfTemplates(ctx, templateErrs), true)
-	}
 
 	if content, ok := ctx.Data["content"].(string); ok && content != "" {
 		// If a template content is set, prepend the "content". In this case that's only

routers/web/repo/issue.go

@@ -870,51 +870,112 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 	ctx.Data["IssueSidebarReviewersData"] = data
 }
 
-// RetrieveRepoMetas find all the meta information of a repository
-func RetrieveRepoMetas(ctx *context.Context, repo *repo_model.Repository, isPull bool) []*issues_model.Label {
-	if !ctx.Repo.CanWriteIssuesOrPulls(isPull) {
-		return nil
+type issueSidebarLabelsData struct {
+	Repository       *repo_model.Repository
+	RepoLink         string
+	IssueID          int64
+	IsPullRequest    bool
+	AllLabels        []*issues_model.Label
+	RepoLabels       []*issues_model.Label
+	OrgLabels        []*issues_model.Label
+	SelectedLabelIDs string
+}
+
+func makeSelectedStringIDs[KeyType, ItemType comparable](
+	allLabels []*issues_model.Label, candidateKey func(candidate *issues_model.Label) KeyType,
+	selectedItems []ItemType, selectedKey func(selected ItemType) KeyType,
+) string {
+	selectedIDSet := make(container.Set[string])
+	allLabelMap := map[KeyType]*issues_model.Label{}
+	for _, label := range allLabels {
+		allLabelMap[candidateKey(label)] = label
+	}
+	for _, item := range selectedItems {
+		if label, ok := allLabelMap[selectedKey(item)]; ok {
+			label.IsChecked = true
+			selectedIDSet.Add(strconv.FormatInt(label.ID, 10))
+		}
+	}
+	ids := selectedIDSet.Values()
+	sort.Strings(ids)
+	return strings.Join(ids, ",")
+}
+
+func (d *issueSidebarLabelsData) SetSelectedLabels(labels []*issues_model.Label) {
+	d.SelectedLabelIDs = makeSelectedStringIDs(
+		d.AllLabels, func(label *issues_model.Label) int64 { return label.ID },
+		labels, func(label *issues_model.Label) int64 { return label.ID },
+	)
+}
+
+func (d *issueSidebarLabelsData) SetSelectedLabelNames(labelNames []string) {
+	d.SelectedLabelIDs = makeSelectedStringIDs(
+		d.AllLabels, func(label *issues_model.Label) string { return strings.ToLower(label.Name) },
+		labelNames, strings.ToLower,
+	)
+}
+
+func (d *issueSidebarLabelsData) SetSelectedLabelIDs(labelIDs []int64) {
+	d.SelectedLabelIDs = makeSelectedStringIDs(
+		d.AllLabels, func(label *issues_model.Label) int64 { return label.ID },
+		labelIDs, func(labelID int64) int64 { return labelID },
+	)
+}
+
+func retrieveRepoLabels(ctx *context.Context, repo *repo_model.Repository, issueID int64, isPull bool) *issueSidebarLabelsData {
+	labelsData := &issueSidebarLabelsData{
+		Repository:    repo,
+		RepoLink:      ctx.Repo.RepoLink,
+		IssueID:       issueID,
+		IsPullRequest: isPull,
 	}
+	ctx.Data["IssueSidebarLabelsData"] = labelsData
 
 	labels, err := issues_model.GetLabelsByRepoID(ctx, repo.ID, "", db.ListOptions{})
 	if err != nil {
 		ctx.ServerError("GetLabelsByRepoID", err)
 		return nil
 	}
-	ctx.Data["Labels"] = labels
+	labelsData.RepoLabels = labels
+
 	if repo.Owner.IsOrganization() {
 		orgLabels, err := issues_model.GetLabelsByOrgID(ctx, repo.Owner.ID, ctx.FormString("sort"), db.ListOptions{})
 		if err != nil {
 			return nil
 		}
+		labelsData.OrgLabels = orgLabels
+	}
+	labelsData.AllLabels = append(labelsData.AllLabels, labelsData.RepoLabels...)
+	labelsData.AllLabels = append(labelsData.AllLabels, labelsData.OrgLabels...)
+	return labelsData
+}
 
-		ctx.Data["OrgLabels"] = orgLabels
-		labels = append(labels, orgLabels...)
+// retrieveRepoMetasForIssueWriter finds some the meta information of a repository for an issue/pr writer
+func retrieveRepoMetasForIssueWriter(ctx *context.Context, repo *repo_model.Repository, isPull bool) {
+	if !ctx.Repo.CanWriteIssuesOrPulls(isPull) {
+		return
 	}
 
 	RetrieveRepoMilestonesAndAssignees(ctx, repo)
 	if ctx.Written() {
-		return nil
+		return
 	}
 
 	retrieveProjects(ctx, repo)
 	if ctx.Written() {
-		return nil
+		return
 	}
 
 	PrepareBranchList(ctx)
 	if ctx.Written() {
-		return nil
+		return
 	}
-
 	// Contains true if the user can create issue dependencies
 	ctx.Data["CanCreateIssueDependencies"] = ctx.Repo.CanCreateIssueDependencies(ctx, ctx.Doer, isPull)
-
-	return labels
 }
 
 // Tries to load and set an issue template. The first return value indicates if a template was loaded.
-func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleFiles []string) (bool, map[string]error) {
+func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleFiles []string, labelsData *issueSidebarLabelsData) (bool, map[string]error) {
 	commit, err := ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.Repository.DefaultBranch)
 	if err != nil {
 		return false, nil
@@ -951,26 +1012,9 @@ func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleFiles
 			ctx.Data["Fields"] = template.Fields
 			ctx.Data["TemplateFile"] = template.FileName
 		}
-		labelIDs := make([]string, 0, len(template.Labels))
-		if repoLabels, err := issues_model.GetLabelsByRepoID(ctx, ctx.Repo.Repository.ID, "", db.ListOptions{}); err == nil {
-			ctx.Data["Labels"] = repoLabels
-			if ctx.Repo.Owner.IsOrganization() {
-				if orgLabels, err := issues_model.GetLabelsByOrgID(ctx, ctx.Repo.Owner.ID, ctx.FormString("sort"), db.ListOptions{}); err == nil {
-					ctx.Data["OrgLabels"] = orgLabels
-					repoLabels = append(repoLabels, orgLabels...)
-				}
-			}
 
-			for _, metaLabel := range template.Labels {
-				for _, repoLabel := range repoLabels {
-					if strings.EqualFold(repoLabel.Name, metaLabel) {
-						repoLabel.IsChecked = true
-						labelIDs = append(labelIDs, strconv.FormatInt(repoLabel.ID, 10))
-						break
-					}
-				}
-			}
-		}
+		labelsData.SetSelectedLabelNames(template.Labels)
+
 		selectedAssigneeIDs := make([]int64, 0, len(template.Assignees))
 		selectedAssigneeIDStrings := make([]string, 0, len(template.Assignees))
 		if userIDs, err := user_model.GetUserIDsByNames(ctx, template.Assignees, false); err == nil {
@@ -983,8 +1027,7 @@ func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleFiles
 		if template.Ref != "" && !strings.HasPrefix(template.Ref, "refs/") { // Assume that the ref intended is always a branch - for tags users should use refs/tags/<ref>
 			template.Ref = git.BranchPrefix + template.Ref
 		}
-		ctx.Data["HasSelectedLabel"] = len(labelIDs) > 0
-		ctx.Data["label_ids"] = strings.Join(labelIDs, ",")
+
 		ctx.Data["HasSelectedAssignee"] = len(selectedAssigneeIDs) > 0
 		ctx.Data["assignee_ids"] = strings.Join(selectedAssigneeIDStrings, ",")
 		ctx.Data["SelectedAssigneeIDs"] = selectedAssigneeIDs
@@ -1042,8 +1085,14 @@ func NewIssue(ctx *context.Context) {
 		}
 	}
 
-	RetrieveRepoMetas(ctx, ctx.Repo.Repository, false)
-
+	retrieveRepoMetasForIssueWriter(ctx, ctx.Repo.Repository, false)
+	if ctx.Written() {
+		return
+	}
+	labelsData := retrieveRepoLabels(ctx, ctx.Repo.Repository, 0, false)
+	if ctx.Written() {
+		return
+	}
 	tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID)
 	if err != nil {
 		ctx.ServerError("GetTagNamesByRepoID", err)
@@ -1052,7 +1101,7 @@ func NewIssue(ctx *context.Context) {
 	ctx.Data["Tags"] = tags
 
 	ret := issue_service.ParseTemplatesFromDefaultBranch(ctx.Repo.Repository, ctx.Repo.GitRepo)
-	templateLoaded, errs := setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates)
+	templateLoaded, errs := setTemplateIfExists(ctx, issueTemplateKey, IssueTemplateCandidates, labelsData)
 	for k, v := range errs {
 		ret.TemplateErrors[k] = v
 	}
@@ -1161,34 +1210,25 @@ func ValidateRepoMetas(ctx *context.Context, form forms.CreateIssueForm, isPull
 		err  error
 	)
 
-	labels := RetrieveRepoMetas(ctx, ctx.Repo.Repository, isPull)
+	retrieveRepoMetasForIssueWriter(ctx, ctx.Repo.Repository, isPull)
+	if ctx.Written() {
+		return ret
+	}
+	labelsData := retrieveRepoLabels(ctx, ctx.Repo.Repository, 0, isPull)
 	if ctx.Written() {
 		return ret
 	}
 
 	var labelIDs []int64
-	hasSelected := false
 	// Check labels.
 	if len(form.LabelIDs) > 0 {
 		labelIDs, err = base.StringsToInt64s(strings.Split(form.LabelIDs, ","))
 		if err != nil {
 			return ret
 		}
-		labelIDMark := make(container.Set[int64])
-		labelIDMark.AddMultiple(labelIDs...)
-
-		for i := range labels {
-			if labelIDMark.Contains(labels[i].ID) {
-				labels[i].IsChecked = true
-				hasSelected = true
-			}
-		}
+		labelsData.SetSelectedLabelIDs(labelIDs)
 	}
 
-	ctx.Data["Labels"] = labels
-	ctx.Data["HasSelectedLabel"] = hasSelected
-	ctx.Data["label_ids"] = form.LabelIDs
-
 	// Check milestone.
 	milestoneID := form.MilestoneID
 	if milestoneID > 0 {
@@ -1579,38 +1619,15 @@ func ViewIssue(ctx *context.Context) {
 		}
 	}
 
-	// Metas.
-	// Check labels.
-	labelIDMark := make(container.Set[int64])
-	for _, label := range issue.Labels {
-		labelIDMark.Add(label.ID)
-	}
-	labels, err := issues_model.GetLabelsByRepoID(ctx, repo.ID, "", db.ListOptions{})
-	if err != nil {
-		ctx.ServerError("GetLabelsByRepoID", err)
+	retrieveRepoMetasForIssueWriter(ctx, repo, issue.IsPull)
+	if ctx.Written() {
 		return
 	}
-	ctx.Data["Labels"] = labels
-
-	if repo.Owner.IsOrganization() {
-		orgLabels, err := issues_model.GetLabelsByOrgID(ctx, repo.Owner.ID, ctx.FormString("sort"), db.ListOptions{})
-		if err != nil {
-			ctx.ServerError("GetLabelsByOrgID", err)
-			return
-		}
-		ctx.Data["OrgLabels"] = orgLabels
-
-		labels = append(labels, orgLabels...)
-	}
-
-	hasSelected := false
-	for i := range labels {
-		if labelIDMark.Contains(labels[i].ID) {
-			labels[i].IsChecked = true
-			hasSelected = true
-		}
+	labelsData := retrieveRepoLabels(ctx, repo, issue.ID, issue.IsPull)
+	if ctx.Written() {
+		return
 	}
-	ctx.Data["HasSelectedLabel"] = hasSelected
+	labelsData.SetSelectedLabels(issue.Labels)
 
 	// Check milestone and assignee.
 	if ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {

routers/web/repo/issue_label.go

@@ -53,11 +53,11 @@ func InitializeLabels(ctx *context.Context) {
 	ctx.Redirect(ctx.Repo.RepoLink + "/labels")
 }
 
-// RetrieveLabels find all the labels of a repository and organization
-func RetrieveLabels(ctx *context.Context) {
+// RetrieveLabelsForList find all the labels of a repository and organization, it is only used by "/labels" page to list all labels
+func RetrieveLabelsForList(ctx *context.Context) {
 	labels, err := issues_model.GetLabelsByRepoID(ctx, ctx.Repo.Repository.ID, ctx.FormString("sort"), db.ListOptions{})
 	if err != nil {
-		ctx.ServerError("RetrieveLabels.GetLabels", err)
+		ctx.ServerError("RetrieveLabelsForList.GetLabels", err)
 		return
 	}
 

routers/web/repo/issue_label_test.go

@@ -62,7 +62,7 @@ func TestRetrieveLabels(t *testing.T) {
 		contexttest.LoadUser(t, ctx, 2)
 		contexttest.LoadRepo(t, ctx, testCase.RepoID)
 		ctx.Req.Form.Set("sort", testCase.Sort)
-		RetrieveLabels(ctx)
+		RetrieveLabelsForList(ctx)
 		assert.False(t, ctx.Written())
 		labels, ok := ctx.Data["Labels"].([]*issues_model.Label)
 		assert.True(t, ok)

routers/web/web.go

@@ -1163,7 +1163,7 @@ func registerRoutes(m *web.Router) {
 		m.Get("/issues/posters", repo.IssuePosters) // it can't use {type:issues|pulls} because it would conflict with other routes like "/pulls/{index}"
 		m.Get("/pulls/posters", repo.PullPosters)
 		m.Get("/comments/{id}/attachments", repo.GetCommentAttachments)
-		m.Get("/labels", repo.RetrieveLabels, repo.Labels)
+		m.Get("/labels", repo.RetrieveLabelsForList, repo.Labels)
 		m.Get("/milestones", repo.Milestones)
 		m.Get("/milestone/{id}", context.RepoRef(), repo.MilestoneIssuesAndPulls)
 		m.Group("/{type:issues|pulls}", func() {