From fe3b11b7d2fd2f4c21530760aa88803b043b97c0 Mon Sep 17 00:00:00 2001 From: Norwin Date: Fri, 1 Apr 2022 21:18:57 +0200 Subject: [PATCH 1/3] fix issue counts by repo when label filter is active --- routers/web/user/home.go | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/routers/web/user/home.go b/routers/web/user/home.go index 156cf0fa645bd..ae697482c01ce 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -496,25 +496,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { isShowClosed := ctx.FormString("state") == "closed" opts.IsClosed = util.OptionalBoolOf(isShowClosed) - // Filter repos and count issues in them. Count will be used later. - // USING NON-FINAL STATE OF opts FOR A QUERY. - var issueCountByRepo map[int64]int64 - if !forceEmpty { - issueCountByRepo, err = models.CountIssuesByRepo(opts) - if err != nil { - ctx.ServerError("CountIssuesByRepo", err) - return - } - } - - // Make sure page number is at least 1. Will be posted to ctx.Data. - page := ctx.FormInt("page") - if page <= 1 { - page = 1 - } - opts.Page = page - opts.PageSize = setting.UI.IssuePagingNum - // Get IDs for labels (a filter option for issues/pulls). // Required for IssuesOptions. var labelIDs []int64 @@ -535,6 +516,25 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { opts.RepoIDs = repoIDs } + // Filter repos and count issues in them. Count will be used later. + // NOTE: this needs to run before pagination is applied to opts! + var issueCountByRepo map[int64]int64 + if !forceEmpty { + issueCountByRepo, err = models.CountIssuesByRepo(opts) + if err != nil { + ctx.ServerError("CountIssuesByRepo", err) + return + } + } + + // Make sure page number is at least 1. Will be posted to ctx.Data. + page := ctx.FormInt("page") + if page <= 1 { + page = 1 + } + opts.Page = page + opts.PageSize = setting.UI.IssuePagingNum + // ------------------------------ // Get issues as defined by opts. // ------------------------------ From baa331b0b6b38a304339db2d955adc8ad9145fc0 Mon Sep 17 00:00:00 2001 From: Norwin Date: Fri, 1 Apr 2022 21:20:20 +0200 Subject: [PATCH 2/3] fix & refactor issue listing label filters - dedup & unify logic - make sure allow / deny list works --- models/issue.go | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/models/issue.go b/models/issue.go index cf5e4fd8b6fe9..c4327b9a11199 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1207,7 +1207,7 @@ type IssuesOptions struct { ProjectBoardID int64 IsClosed util.OptionalBool IsPull util.OptionalBool - LabelIDs []int64 + LabelIDs []int64 // negative IDs exclude the given label IncludedLabelNames []string ExcludedLabelNames []string IncludeMilestones []string @@ -1348,14 +1348,7 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) { } if opts.LabelIDs != nil { - for i, labelID := range opts.LabelIDs { - if labelID > 0 { - sess.Join("INNER", fmt.Sprintf("issue_label il%d", i), - fmt.Sprintf("issue.id = il%[1]d.issue_id AND il%[1]d.label_id = %[2]d", i, labelID)) - } else { - sess.Where("issue.id not in (select issue_id from issue_label where label_id = ?)", -labelID) - } - } + applyLabelIDCondition(sess, opts.LabelIDs) } if len(opts.IncludedLabelNames) > 0 { @@ -1440,6 +1433,27 @@ func applyReviewRequestedCondition(sess *xorm.Session, reviewRequestedID int64) reviewRequestedID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest, reviewRequestedID) } +func applyLabelIDCondition(sess *xorm.Session, labelIDs []int64) *xorm.Session { + includes := make([]int64, 0, len(labelIDs)) + excludes := make([]int64, 0, len(labelIDs)) + for _, labelID := range labelIDs { + if labelID > 0 { + includes = append(includes, labelID) + } else { + excludes = append(excludes, -labelID) + } + } + if len(includes) > 0 { + sess.In("issue.id", builder.Select("issue_id").From("issue_label"). + Where(builder.In("label_id", includes))) + } + if len(excludes) > 0 { + sess.NotIn("issue.id", builder.Select("issue_id").From("issue_label"). + Where(builder.In("label_id", excludes))) + } + return sess +} + // CountIssuesByRepo map from repoID to number of issues matching the options func CountIssuesByRepo(opts *IssuesOptions) (map[int64]int64, error) { e := db.GetEngine(db.DefaultContext) @@ -1664,14 +1678,7 @@ func getIssueStatsChunk(opts *IssueStatsOptions, issueIDs []int64) (*IssueStats, if err != nil { log.Warn("Malformed Labels argument: %s", opts.Labels) } else { - for i, labelID := range labelIDs { - if labelID > 0 { - sess.Join("INNER", fmt.Sprintf("issue_label il%d", i), - fmt.Sprintf("issue.id = il%[1]d.issue_id AND il%[1]d.label_id = %[2]d", i, labelID)) - } else { - sess.Where("issue.id NOT IN (SELECT issue_id FROM issue_label WHERE label_id = ?)", -labelID) - } - } + applyLabelIDCondition(sess, labelIDs) } } @@ -1757,8 +1764,7 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { sess := func(cond builder.Cond) *xorm.Session { s := db.GetEngine(db.DefaultContext).Where(cond) if len(opts.LabelIDs) > 0 { - s.Join("INNER", "issue_label", "issue_label.issue_id = issue.id"). - In("issue_label.label_id", opts.LabelIDs) + applyLabelIDCondition(s, opts.LabelIDs) } if opts.UserID > 0 || opts.IsArchived != util.OptionalBoolNone { s.Join("INNER", "repository", "issue.repo_id = repository.id") From bc29a39caf6cc025d06abe31f0e850550386d2a7 Mon Sep 17 00:00:00 2001 From: Norwin Date: Fri, 1 Apr 2022 21:23:00 +0200 Subject: [PATCH 3/3] issue dashboard: allow filtering by label name as the issue search query is complicated already, I added the label search as a separate query. It's also non-trivial, as a user can have access to various label sets, which may contain different labels with the same name. TODO: - Add a proper frontend for this. dropdown like on repo issue listing is not an option, as labels will be duplicated across repos. Maybe extend q= with lucene-style syntax `label:foo -label:bar` - Preserve &labels= and &label-names= on sidebar button links. --- models/issue_label.go | 24 ++++++++++++++++++++++++ routers/web/user/home.go | 16 ++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/models/issue_label.go b/models/issue_label.go index 453a0b14a9676..e26079bd6b09c 100644 --- a/models/issue_label.go +++ b/models/issue_label.go @@ -350,6 +350,30 @@ func GetLabelInRepoByID(repoID, labelID int64) (*Label, error) { return getLabelInRepoByID(db.GetEngine(db.DefaultContext), repoID, labelID) } +// GetAccessibleLabelIDs returns a list of label IDs matching one of the given names, +// when the given user has access to them via repos or orgs. +func GetAccessibleLabelIDsByName(names []string, userID int64) ([]int64, error) { + return getAccessibleLabelIDsByName(db.GetEngine(db.DefaultContext), names, userID) +} + +func getAccessibleLabelIDsByName(e db.Engine, names []string, userID int64) ([]int64, error) { + ids := make([]int64, 0, 10) + return ids, e. + Table("label"). + Where(builder.And( + builder.In("name", names), + // NOTE @perf: the following filters are intended to reduce the returned labels to a manageable set on + // instances with many repos/users/orgs. For few labels these extra queries will be a net negative. + builder.Or( + builder.In("org_id", builder.Select("org_id").From("org_user").Where(builder.Eq{"uid": userID})), + builder.In("repo_id", builder.Select("id").From("repository").Where(builder.Eq{"owner_id": userID})), + builder.In("repo_id", builder.Select("repo_id").From("access").Where(builder.Eq{"user_id": userID})), + ), + )). + Cols("id"). + Find(&ids) +} + // GetLabelsInRepoByIDs returns a list of labels by IDs in given repository, // it silently ignores label IDs that do not belong to the repository. func GetLabelsInRepoByIDs(repoID int64, labelIDs []int64) ([]*Label, error) { diff --git a/routers/web/user/home.go b/routers/web/user/home.go index ae697482c01ce..623ef6b67a9a1 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -509,6 +509,20 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { } opts.LabelIDs = labelIDs + // Labels can also be search for by name. Add them to the LabelID query. + labelNameList := ctx.FormString("label-names") + if len(labelNameList) != 0 { + labelNames := strings.Split(labelNameList, ",") + if len(labelNameList) > 0 { + labelIDs, err = models.GetAccessibleLabelIDsByName(labelNames, ctx.Doer.ID) + if err != nil { + ctx.ServerError("GetAccessibleLabelIDsByName", err) + return + } + } + opts.LabelIDs = append(opts.LabelIDs, labelIDs...) + } + // Parse ctx.FormString("repos") and remember matched repo IDs for later. // Gets set when clicking filters on the issues overview page. repoIDs := getRepoIDs(ctx.FormString("repos")) @@ -664,6 +678,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { ctx.Data["RepoIDs"] = repoIDs ctx.Data["IsShowClosed"] = isShowClosed ctx.Data["SelectLabels"] = selectedLabels + ctx.Data["SelectLabelNames"] = labelNameList if isShowClosed { ctx.Data["State"] = "closed" @@ -683,6 +698,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { pager.AddParam(ctx, "sort", "SortType") pager.AddParam(ctx, "state", "State") pager.AddParam(ctx, "labels", "SelectLabels") + pager.AddParam(ctx, "label-names", "SelectLabelNames") pager.AddParam(ctx, "milestone", "MilestoneID") pager.AddParam(ctx, "assignee", "AssigneeID") ctx.Data["Page"] = pager