Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor issue filter (labels, poster, assignee) #32771

Merged
merged 6 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions modules/templates/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func NewFuncMap() template.FuncMap {
"HTMLFormat": htmlutil.HTMLFormat,
"HTMLEscape": htmlEscape,
"QueryEscape": queryEscape,
"QueryBuild": queryBuild,
"QueryBuild": QueryBuild,
"JSEscape": jsEscapeSafe,
"SanitizeHTML": SanitizeHTML,
"URLJoin": util.URLJoin,
Expand Down Expand Up @@ -294,24 +294,27 @@ func timeEstimateString(timeSec any) string {
return util.TimeEstimateString(v)
}

func queryBuild(a ...any) template.URL {
// QueryBuild builds a query string from a list of key-value pairs.
// It omits the nil and empty strings, but it doesn't omit other zero values,
// because the zero value of number types may have a meaning.
func QueryBuild(a ...any) template.URL {
var s string
if len(a)%2 == 1 {
if v, ok := a[0].(string); ok {
if v == "" || (v[0] != '?' && v[0] != '&') {
panic("queryBuild: invalid argument")
panic("QueryBuild: invalid argument")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to panic? Is it possible to recover from a panic if necessary?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to panic?

Yes. Golang also does so.

Is it possible to recover from a panic if necessary?

No

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user's perspective, no one wants to cause the entire system to panic because of a query. I think this is unreasonable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user's perspective, no one wants to cause the entire system to panic because of a query. I think this is unreasonable.

There are a variety of operations in Golang that may cause a panic. For this case, I think it is the developer's responsibility to check and ensure that the query does not cause a panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user's perspective, no one wants to cause the entire system to panic because of a query. I think this is unreasonable.

No, it won't panic from "user's perspective". It only panics for developer's mistakes.

It is designed carefully and intentionally, I won't change it unless you could show a real case why end users would be affected.

}
s = v
} else if v, ok := a[0].(template.URL); ok {
s = string(v)
} else {
panic("queryBuild: invalid argument")
panic("QueryBuild: invalid argument")
}
}
for i := len(a) % 2; i < len(a); i += 2 {
k, ok := a[i].(string)
if !ok {
panic("queryBuild: invalid argument")
panic("QueryBuild: invalid argument")
}
var v string
if va, ok := a[i+1].(string); ok {
Expand Down
5 changes: 3 additions & 2 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ delete_preexisting_success = Deleted unadopted files in %s
blame_prior = View blame prior to this change
blame.ignore_revs = Ignoring revisions in <a href="%s">.git-blame-ignore-revs</a>. Click <a href="%s">here to bypass</a> and see the normal blame view.
blame.ignore_revs.failed = Failed to ignore revisions in <a href="%s">.git-blame-ignore-revs</a>.
author_search_tooltip = Shows a maximum of 30 users
user_search_tooltip = Shows a maximum of 30 users

tree_path_not_found_commit = Path %[1]s doesn't exist in commit %[2]s
tree_path_not_found_branch = Path %[1]s doesn't exist in branch %[2]s
Expand Down Expand Up @@ -1529,7 +1529,8 @@ issues.filter_assignee = Assignee
issues.filter_assginee_no_select = All assignees
issues.filter_assginee_no_assignee = No assignee
issues.filter_poster = Author
issues.filter_poster_no_select = All authors
issues.filter_user_placeholder = Search users
issues.filter_user_no_select = All users
issues.filter_type = Type
issues.filter_type.all_issues = All issues
issues.filter_type.assigned_to_you = Assigned to you
Expand Down
7 changes: 1 addition & 6 deletions routers/web/org/projects.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,12 +339,7 @@ func ViewProject(ctx *context.Context) {
// 0 means issues with no label
// blank means labels will not be filtered for issues
selectLabels := ctx.FormString("labels")
if selectLabels == "" {
ctx.Data["AllLabels"] = true
} else if selectLabels == "0" {
ctx.Data["NoLabel"] = true
}
if len(selectLabels) > 0 {
if selectLabels != "" {
labelIDs, err = base.StringsToInt64s(strings.Split(selectLabels, ","))
if err != nil {
ctx.Flash.Error(ctx.Tr("invalid_data", selectLabels), true)
Expand Down
29 changes: 7 additions & 22 deletions routers/web/repo/issue_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"bytes"
"fmt"
"net/http"
"net/url"
"strconv"
"strings"

Expand Down Expand Up @@ -531,12 +530,7 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt
// 0 means issues with no label
// blank means labels will not be filtered for issues
selectLabels := ctx.FormString("labels")
if selectLabels == "" {
ctx.Data["AllLabels"] = true
} else if selectLabels == "0" {
ctx.Data["NoLabel"] = true
}
if len(selectLabels) > 0 {
if selectLabels != "" {
labelIDs, err = base.StringsToInt64s(strings.Split(selectLabels, ","))
if err != nil {
ctx.Flash.Error(ctx.Tr("invalid_data", selectLabels), true)
Expand Down Expand Up @@ -616,8 +610,6 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt
ctx.Data["TotalTrackedTime"] = totalTrackedTime
}

archived := ctx.FormBool("archived")

page := ctx.FormInt("page")
if page <= 1 {
page = 1
Expand Down Expand Up @@ -792,28 +784,21 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt
return
}

showArchivedLabels := ctx.FormBool("archived_labels")
ctx.Data["ShowArchivedLabels"] = showArchivedLabels
ctx.Data["PinnedIssues"] = pinned
ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.IsAdmin() || ctx.Doer.IsAdmin)
ctx.Data["IssueStats"] = issueStats
ctx.Data["OpenCount"] = issueStats.OpenCount
ctx.Data["ClosedCount"] = issueStats.ClosedCount
linkStr := "%s?q=%s&type=%s&sort=%s&state=%s&labels=%s&milestone=%d&project=%d&assignee=%d&poster=%v&archived=%t"
ctx.Data["AllStatesLink"] = fmt.Sprintf(linkStr, ctx.Link,
url.QueryEscape(keyword), url.QueryEscape(viewType), url.QueryEscape(sortType), "all", url.QueryEscape(selectLabels),
milestoneID, projectID, assigneeID, url.QueryEscape(posterUsername), archived)
ctx.Data["OpenLink"] = fmt.Sprintf(linkStr, ctx.Link,
url.QueryEscape(keyword), url.QueryEscape(viewType), url.QueryEscape(sortType), "open", url.QueryEscape(selectLabels),
milestoneID, projectID, assigneeID, url.QueryEscape(posterUsername), archived)
ctx.Data["ClosedLink"] = fmt.Sprintf(linkStr, ctx.Link,
url.QueryEscape(keyword), url.QueryEscape(viewType), url.QueryEscape(sortType), "closed", url.QueryEscape(selectLabels),
milestoneID, projectID, assigneeID, url.QueryEscape(posterUsername), archived)
ctx.Data["SelLabelIDs"] = labelIDs
ctx.Data["SelectLabels"] = selectLabels
ctx.Data["ViewType"] = viewType
ctx.Data["SortType"] = sortType
ctx.Data["MilestoneID"] = milestoneID
ctx.Data["ProjectID"] = projectID
ctx.Data["AssigneeID"] = assigneeID
ctx.Data["PosterUserID"] = posterUserID
ctx.Data["PosterUsername"] = posterUsername
ctx.Data["Keyword"] = keyword
ctx.Data["IsShowClosed"] = isShowClosed
Expand All @@ -825,7 +810,6 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt
default:
ctx.Data["State"] = "open"
}
ctx.Data["ShowArchivedLabels"] = archived

pager.AddParamString("q", keyword)
pager.AddParamString("type", viewType)
Expand All @@ -836,8 +820,9 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt
pager.AddParamString("project", fmt.Sprint(projectID))
pager.AddParamString("assignee", fmt.Sprint(assigneeID))
pager.AddParamString("poster", posterUsername)
pager.AddParamString("archived", fmt.Sprint(archived))

if showArchivedLabels {
pager.AddParamString("archived_labels", "true")
}
ctx.Data["Page"] = pager
}

Expand Down
6 changes: 0 additions & 6 deletions routers/web/repo/milestone.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,6 @@ func Milestones(ctx *context.Context) {
}
ctx.Data["OpenCount"] = stats.OpenCount
ctx.Data["ClosedCount"] = stats.ClosedCount
linkStr := "%s/milestones?state=%s&q=%s&sort=%s"
ctx.Data["OpenLink"] = fmt.Sprintf(linkStr, ctx.Repo.RepoLink, "open",
url.QueryEscape(keyword), url.QueryEscape(sortType))
ctx.Data["ClosedLink"] = fmt.Sprintf(linkStr, ctx.Repo.RepoLink, "closed",
url.QueryEscape(keyword), url.QueryEscape(sortType))

if ctx.Repo.Repository.IsTimetrackerEnabled(ctx) {
if err := issues_model.MilestoneList(miles).LoadTotalTrackedTimes(ctx); err != nil {
ctx.ServerError("LoadTotalTrackedTimes", err)
Expand Down
7 changes: 1 addition & 6 deletions routers/web/repo/projects.go
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,7 @@ func ViewProject(ctx *context.Context) {
// 0 means issues with no label
// blank means labels will not be filtered for issues
selectLabels := ctx.FormString("labels")
if selectLabels == "" {
ctx.Data["AllLabels"] = true
} else if selectLabels == "0" {
ctx.Data["NoLabel"] = true
}
if len(selectLabels) > 0 {
if selectLabels != "" {
labelIDs, err = base.StringsToInt64s(strings.Split(selectLabels, ","))
if err != nil {
ctx.Flash.Error(ctx.Tr("invalid_data", selectLabels), true)
Expand Down
84 changes: 12 additions & 72 deletions templates/projects/view.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -5,79 +5,19 @@
<h2 class="tw-mb-0 tw-flex-1 tw-break-anywhere">{{.Project.Title}}</h2>
<div class="project-toolbar-right">
<div class="ui secondary filter menu labels">
<!-- Label -->
<div class="ui {{if not .Labels}}disabled{{end}} dropdown jump item label-filter">
<span class="text">
{{ctx.Locale.Tr "repo.issues.filter_label"}}
</span>
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
<div class="ui icon search input">
<i class="icon">{{svg "octicon-search" 16}}</i>
<input type="text" placeholder="{{ctx.Locale.Tr "repo.issues.filter_label"}}">
</div>
<div class="ui checkbox compact archived-label-filter">
<input name="archived" type="checkbox"
id="archived-filter-checkbox"
{{if .ShowArchivedLabels}}checked{{end}}
>
<label for="archived-filter-checkbox">
{{ctx.Locale.Tr "repo.issues.label_archived_filter"}}
<i class="tw-ml-1" data-tooltip-content={{ctx.Locale.Tr "repo.issues.label_archive_tooltip"}}>
{{svg "octicon-info"}}
</i>
</label>
</div>
<span class="info">{{ctx.Locale.Tr "repo.issues.filter_label_exclude"}}</span>
<div class="divider"></div>
<a class="{{if .AllLabels}}active selected {{end}}item" href="?assignee={{$.AssigneeID}}{{if $.ShowArchivedLabels}}&archived=true{{end}}">{{ctx.Locale.Tr "repo.issues.filter_label_no_select"}}</a>
<a class="{{if .NoLabel}}active selected {{end}}item" href="?assignee={{$.AssigneeID}}{{if $.ShowArchivedLabels}}&archived=true{{end}}">{{ctx.Locale.Tr "repo.issues.filter_label_select_no_label"}}</a>
{{$previousExclusiveScope := "_no_scope"}}
{{range .Labels}}
{{$exclusiveScope := .ExclusiveScope}}
{{if and (ne $previousExclusiveScope $exclusiveScope)}}
<div class="divider" data-scope="{{.ExclusiveScope}}"></div>
{{end}}
{{$previousExclusiveScope = $exclusiveScope}}
<a class="item label-filter-item tw-flex tw-items-center" data-label-id="{{.ID}}" data-scope="{{.ExclusiveScope}}" {{if .IsArchived}}data-is-archived{{end}}
href="?labels={{.QueryString}}&assignee={{$.AssigneeID}}{{if $.ShowArchivedLabels}}&archived=true{{end}}">
{{if .IsExcluded}}
{{svg "octicon-circle-slash"}}
{{else if .IsSelected}}
{{if $exclusiveScope}}
{{svg "octicon-dot-fill"}}
{{else}}
{{svg "octicon-check"}}
{{end}}
{{end}}
{{ctx.RenderUtils.RenderLabel .}}
<p class="tw-ml-auto">{{template "repo/issue/labels/label_archived" .}}</p>
</a>
{{end}}
</div>
</div>
{{$queryLink := QueryBuild "?" "labels" .SelectLabels "assignee" $.AssigneeID "archived_labels" (Iif $.ShowArchivedLabels "true")}}

<!-- Assignee -->
<div class="ui {{if not .Assignees}}disabled{{end}} dropdown jump item">
<span class="text">
{{ctx.Locale.Tr "repo.issues.filter_assignee"}}
</span>
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
<div class="ui icon search input">
<i class="icon">{{svg "octicon-search" 16}}</i>
<input type="text" placeholder="{{ctx.Locale.Tr "repo.issues.filter_assignee"}}">
</div>
<a class="{{if not .AssigneeID}}active selected {{end}}item" href="?labels={{.SelectLabels}}{{if $.ShowArchivedLabels}}&archived=true{{end}}">{{ctx.Locale.Tr "repo.issues.filter_assginee_no_select"}}</a>
<a class="{{if eq .AssigneeID -1}}active selected {{end}}item" href="?labels={{.SelectLabels}}&assignee=-1{{if $.ShowArchivedLabels}}&archived=true{{end}}">{{ctx.Locale.Tr "repo.issues.filter_assginee_no_assignee"}}</a>
<div class="divider"></div>
{{range .Assignees}}
<a class="{{if eq $.AssigneeID .ID}}active selected{{end}} item tw-flex" href="?labels={{$.SelectLabels}}&assignee={{.ID}}{{if $.ShowArchivedLabels}}&archived=true{{end}}">
{{ctx.AvatarUtils.Avatar . 20}}{{template "repo/search_name" .}}
</a>
{{end}}
</div>
</div>
{{template "repo/issue/filter_item_label" dict "Labels" .Labels "QueryLink" $queryLink "SupportArchivedLabel" true}}

{{template "repo/issue/filter_item_user_assign" dict
"QueryParamKey" "assignee"
"QueryLink" $queryLink
"UserSearchList" $.Assignees
"SelectedUserId" $.AssigneeID
"TextFilterTitle" (ctx.Locale.Tr "repo.issues.filter_assignee")
"TextZeroValue" (ctx.Locale.Tr "repo.issues.filter_assginee_no_select")
"TextNegativeOne" (ctx.Locale.Tr "repo.issues.filter_assginee_no_assignee")
}}
</div>
</div>
{{if $canWriteProject}}
Expand Down
45 changes: 45 additions & 0 deletions templates/repo/issue/filter_item_label.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{{/*
* "labels" from query string (needed by JS)
* QueryLink
* Labels
* SupportArchivedLabel, if true, then it needs "archived_labels" from query string
*/}}
{{$queryLink := .QueryLink}}
<div class="item ui dropdown jump {{if not .Labels}}disabled{{end}} label-filter">
<span class="text">{{ctx.Locale.Tr "repo.issues.filter_label"}}</span>
{{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu flex-items-menu">
<div class="ui icon search input">
<i class="icon">{{svg "octicon-search" 16}}</i>
<input type="text" placeholder="{{ctx.Locale.Tr "repo.issues.filter_label"}}">
</div>
{{if .SupportArchivedLabel}}{{/* this checkbox has a hard dependency with the "labels" and "archived_label" query parameter */}}
<label class="label-filter-archived-toggle flex-text-block">
<input type="checkbox"> {{ctx.Locale.Tr "repo.issues.label_archived_filter"}}
<span data-tooltip-content={{ctx.Locale.Tr "repo.issues.label_archive_tooltip"}}>{{svg "octicon-info"}}</span>
</label>
{{end}}
<span class="info">{{ctx.Locale.Tr "repo.issues.filter_label_exclude"}}</span>
<div class="divider"></div>
<a class="item label-filter-query-default" href="{{QueryBuild $queryLink "labels" NIL}}">{{ctx.Locale.Tr "repo.issues.filter_label_no_select"}}</a>
<a class="item label-filter-query-not-set" href="{{QueryBuild $queryLink "labels" 0}}">{{ctx.Locale.Tr "repo.issues.filter_label_select_no_label"}}</a>
{{$previousExclusiveScope := "_no_scope"}}
{{range .Labels}}
{{$exclusiveScope := .ExclusiveScope}}
{{if and (ne $previousExclusiveScope $exclusiveScope)}}
<div class="divider" data-scope="{{.ExclusiveScope}}"></div>
{{end}}
{{$previousExclusiveScope = $exclusiveScope}}
<a class="item label-filter-query-item" data-label-id="{{.ID}}" data-scope="{{.ExclusiveScope}}" {{if .IsArchived}}data-is-archived{{end}}
href="{{QueryBuild $queryLink "labels" .QueryString}}">
{{if .IsExcluded}}
{{svg "octicon-circle-slash"}}
{{else if .IsSelected}}
{{Iif $exclusiveScope (svg "octicon-dot-fill") (svg "octicon-check")}}
{{end}}
{{ctx.RenderUtils.RenderLabel .}}
<p class="tw-ml-auto">{{template "repo/issue/labels/label_archived" .}}</p>
</a>
{{end}}
</div>
</div>
31 changes: 31 additions & 0 deletions templates/repo/issue/filter_item_user_assign.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{{/* This is a user list for filter, the data is provided by a local variable assignment
* QueryParamKey: eg: "poster", "assignee"
* QueryLink
* UserSearchList
* SelectedUserId: 0 or empty means default, -1 means "no user is set"
* TextFilterTitle
* TextZeroValue: the text for "all issues"
* TextNegativeOne: the text for "issues with no assignee"
*/}}
{{$queryLink := .QueryLink}}
<div class="item ui dropdown jump {{if not .UserSearchList}}disabled{{end}}">
{{$.TextFilterTitle}} {{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
<div class="ui icon search input">
<i class="icon">{{svg "octicon-search" 16}}</i>
<input type="text" placeholder="{{ctx.Locale.Tr "repo.issues.filter_user_placeholder"}}">
</div>
{{if $.TextZeroValue}}
<a class="item {{if not .SelectedUserId}}selected{{end}}" href="{{QueryBuild $queryLink $.QueryParamKey NIL}}">{{$.TextZeroValue}}</a>
{{end}}
{{if $.TextNegativeOne}}
<a class="item {{if eq .SelectedUserId -1}}selected{{end}}" href="{{QueryBuild $queryLink $.QueryParamKey -1}}">{{$.TextNegativeOne}}</a>
{{end}}
<div class="divider"></div>
{{range .UserSearchList}}
<a class="item {{if eq $.SelectedUserId .ID}}selected{{end}}" href="{{QueryBuild $queryLink $.QueryParamKey .ID}}">
{{ctx.AvatarUtils.Avatar . 20}}{{template "repo/search_name" .}}
</a>
{{end}}
</div>
</div>
23 changes: 23 additions & 0 deletions templates/repo/issue/filter_item_user_fetch.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{{/* This is a user list for filter, the data is provided by a remote "fetch" request
* QueryParamKey: eg: "poster", "assignee"
* QueryLink
* UserSearchUrl
* SelectedUserId
* TextFilterTitle
*/}}
{{$queryLink := .QueryLink}}
<div class="item ui dropdown custom user-remote-search" data-tooltip-content="{{ctx.Locale.Tr "repo.user_search_tooltip"}}"
data-search-url="{{$.UserSearchUrl}}"
data-selected-user-id="{{$.SelectedUserId}}"
data-action-jump-url="{{QueryBuild $queryLink $.QueryParamKey NIL}}&{{$.QueryParamKey}}={username}"
>
{{$.TextFilterTitle}} {{svg "octicon-triangle-down" 14 "dropdown icon"}}
<div class="menu">
<div class="ui icon search input">
<i class="icon">{{svg "octicon-search" 16}}</i>
<input type="text" placeholder="{{ctx.Locale.Tr "repo.issues.filter_user_placeholder"}}">
</div>
<a class="item" data-value="">{{ctx.Locale.Tr "repo.issues.filter_user_no_select"}}</a>
<a class="item item-from-input tw-hidden"></a>
</div>
</div>
Loading
Loading