tangled.org / core
Monorepo for Tangled tangled.org
fork atom

Implement tag-based filters for issues and pulls #1065

merged opened by octet-stream.net targeting master from octet-stream.net/core: thombles/search-filter-tags

Fixes issue #297

This changeset makes the search textfield on the "issues" and "pulls" pages much more powerful.

  • keyword and -keyword
  • "some phrase" and -"some phrase"
  • label:good-first-issue and -label:good-first-issue
  • author:octet-stream.net and -author:octet-stream.net
  • state:open, state:closed and state:merged
  • label:good-first-issue and label:documentation to require multiple labels

The open/closed/merged tab buttons now play a subordinate role to the textfield. When you first load the page it is populated with state:open to show the current state. If you click "closed" then this sends the current state of the query and on the server side the state tag will be replaced with state:closed for the re-rendered page. This works regardless of what other filters are currently configured and I believe I've made it all work equally with and without JS. If you wish you can manually select and delete the state: tag to see issues with different states in the same results listing.

If you are doing anything more advanced than a state: tag then bleve is used to perform the query.

This pull also fixes missing re-index triggers when labels are added/removed from issues and pulls.

Potentially controversial quirks:

  • The author: tag requires resolving the handle. This is fast if it's in the cache but a blocking operation if it's one we haven't seen before.
  • Clicking the "X" clears the textfield except for the state: tag. I feel that this is useful. The other logical option is to empty the field entirely so that we are looking at all issues in all states.
  • Issue and pull index mappings now contain a version that will be used to detect when the index needs to be rebuilt because the code has changed the schema.

Out of scope:

  • Any kind of UI to automate or guide the user to use the new tag types

Some screenshots to give you the idea...

Default view when you open the issues page:

Then you clear the textfield and press enter:

Label search:

Narrow it down with another label:

Label and a keyword together:

Labels

None yet.

assignee

None yet.

Participants 4
AT URI
at://did:plc:txurc6ueald5d7462bpvzdby/sh.tangled.repo.pull/3melcljdmh622
+50 -7
Interdiff #1 → #2
unified split
appview/indexer/bleve/query.go

This file has not been changed.

appview/indexer/issues/indexer.go

This file has not been changed.

appview/indexer/issues/indexer_test.go

This file has not been changed.

appview/indexer/notifier.go

This file has not been changed.

appview/indexer/pulls/indexer.go

This file has not been changed.

appview/issues/issues.go

This file has not been changed.

appview/labels/labels.go

This file has not been changed.

appview/models/label.go

This file has not been changed.

appview/models/search.go

This file has not been changed.

appview/notify/db/db.go

This file has not been changed.

appview/notify/logging_notifier.go

This file has not been changed.

appview/notify/merged_notifier.go

This file has not been changed.

appview/notify/notifier.go

This file has not been changed.

appview/pages/pages.go

This file has not been changed.

appview/pages/templates/fragments/tabSelector.html

This file has not been changed.

appview/pages/templates/repo/issues/issues.html

This file has not been changed.

appview/pages/templates/repo/pulls/pulls.html

This file has not been changed.

+3 -6
appview/pulls/pulls.go
··· 1988 1988 record := pull.AsRecord() 1989 1989 record.PatchBlob = blob.Blob 1990 1990 record.CreatedAt = time.Now().Format(time.RFC3339) 1991 - 1992 - if record.Source != nil { 1993 - record.Source.Sha = newSourceRev 1994 - } 1991 + record.Source.Sha = newSourceRev 1995 1992 1996 1993 _, err = comatproto.RepoPutRecord(r.Context(), client, &comatproto.RepoPutRecord_Input{ 1997 1994 Collection: tangled.RepoPullNSID, ··· 2555 2552 w.Close() 2556 2553 return &b 2557 2554 } 2558 - 2559 - func ptrPullState(s models.PullState) *models.PullState { return &s } 2560 2555 w.Close() 2561 2556 return &b 2562 2557 } 2558 + 2559 + func ptrPullState(s models.PullState) *models.PullState { return &s }
appview/searchquery/searchquery.go

This file has not been changed.

+46
appview/searchquery/searchquery_test.go
··· 204 204 q := Parse(" state:open keyword ") 205 205 assert.Equal(t, "state:open keyword", q.String()) 206 206 } 207 + 208 + func TestParseConsecutiveColons(t *testing.T) { 209 + q := Parse("foo:::bar") 210 + items := q.Items() 211 + assert.Equal(t, 1, len(items)) 212 + assert.Equal(t, KindTagValue, items[0].Kind) 213 + assert.Equal(t, "foo", items[0].Key) 214 + assert.Equal(t, "::bar", items[0].Value) 215 + } 216 + 217 + func TestParseEmptyQuotes(t *testing.T) { 218 + q := Parse(`""`) 219 + items := q.Items() 220 + assert.Equal(t, 1, len(items)) 221 + assert.Equal(t, KindQuoted, items[0].Kind) 222 + assert.Equal(t, `""`, items[0].Raw) 223 + assert.Equal(t, "", items[0].Value) 224 + } 225 + 226 + func TestParseBareDash(t *testing.T) { 227 + q := Parse("-") 228 + items := q.Items() 229 + assert.Equal(t, 1, len(items)) 230 + assert.Equal(t, KindKeyword, items[0].Kind) 231 + assert.False(t, items[0].Negated) 232 + assert.Equal(t, "-", items[0].Raw) 233 + } 234 + 235 + func TestParseDashColon(t *testing.T) { 236 + q := Parse("-:value") 237 + items := q.Items() 238 + assert.Equal(t, 1, len(items)) 239 + assert.Equal(t, KindKeyword, items[0].Kind) 240 + assert.True(t, items[0].Negated) 241 + assert.Equal(t, ":value", items[0].Value) 242 + } 243 + 244 + func TestParseDoubleHyphen(t *testing.T) { 245 + q := Parse("--label:bug") 246 + items := q.Items() 247 + assert.Equal(t, 1, len(items)) 248 + assert.Equal(t, KindTagValue, items[0].Kind) 249 + assert.True(t, items[0].Negated) 250 + assert.Equal(t, "-label", items[0].Key) 251 + assert.Equal(t, "bug", items[0].Value) 252 + }
appview/state/router.go

This file has not been changed.

+1 -1
spindle/engines/nixery/setup_steps.go
··· 17 17 18 18 // dependencyStep processes dependencies defined in the workflow. 19 19 // For dependencies using a custom registry (i.e. not nixpkgs), it collects 20 - // all packages and adds a single 'nix profile add' step to the 20 + // all packages and adds a single 'nix profile install' step to the 21 21 // beginning of the workflow's step list. 22 22 func dependencyStep(deps map[string][]string) *Step { 23 23 var customPackages []string

History

4 rounds 6 comments
sign up or login to add to the discussion
octet-stream.net submitted #3
diff interdiff
8 commits
expand
2ecf5ef1
appview/notify: fix entityType in NewIssueState notification
901ad387
appview/searchquery: add search query parser with negation support
337ba8ae
appview: add label name resolution to LabelState
54a563cb
appview: add search filters for issues and pulls
303e2fad
appview/indexer: force bleve index rebuild when mapping changes
6ce77b01
appview: re-index issues and pulls when labels are modified
9cdb9b52
appview: update tab counts to reflect search filters
bf6bfafb
appview/searchquery: test additional parser edge cases
expand 2 comments
octet-stream.net

Rebased on latest master

anirudh.fi

Thanks for your work on this!

pull request successfully merged
octet-stream.net submitted #2
interdiff
8 commits
expand
fbb955a9
appview/notify: fix entityType in NewIssueState notification
c33eb04d
appview/searchquery: add search query parser with negation support
21656a01
appview: add label name resolution to LabelState
787073bd
appview: add search filters for issues and pulls
b4b0dd23
appview/indexer: force bleve index rebuild when mapping changes
d2d9896e
appview: re-index issues and pulls when labels are modified
5ad31f33
appview: update tab counts to reflect search filters
124bee57
appview/searchquery: test additional parser edge cases
expand 0 comments
octet-stream.net submitted #1
diff interdiff
7 commits
expand
fbb955a9
appview/notify: fix entityType in NewIssueState notification
c33eb04d
appview/searchquery: add search query parser with negation support
21656a01
appview: add label name resolution to LabelState
787073bd
appview: add search filters for issues and pulls
b4b0dd23
appview/indexer: force bleve index rebuild when mapping changes
d2d9896e
appview: re-index issues and pulls when labels are modified
5ad31f33
appview: update tab counts to reflect search filters
expand 3 comments
octet-stream.net

Rebased on master to fix conflict. I dropped the changes from #1062 since it was incompatible with this branch and the pull counts are already correct.

boltless.me

Thank you for your amazing work! The implementation seems good to me.

Might be a nit pick, but I think we need more tests for malformed queries like state:foo, foo:::bar or "foo bar to ensure the behaviour is always consistent and work as expected.

Also note for other reviewers: this PR doesn't handle edits and thats on my fault not implementing reindex logic on updates. Will open an issue and self-assign myself.

octet-stream.net

I've added a handful of unit tests to the parser exploring edge cases, which I'll push as a revision in a moment. I'm satisfied with the current behaviour but writing out the tests is helpful to show that it's deliberate. 👍

Regarding your specific examples:

  • foo:::bar has a new test showing this will be interpreted as key foo with value ::bar. It could be argued that treating any number of colons as a separator is "nicer" but I don't expect it arise often.
  • "foo bar has an existing TestParseUnclosedQuote which shows that it will be auto-closed if it comes at the end of a query
  • state:foo is a different kettle of fish since it's downstream of the parser...

First, the behaviour. Currently the pulls and issues handlers try to match state against one of the specific words (open, closed, merged). If it doesn't match any then it will be ignored entirely, i.e. results will not be filtered by state after all. To me this seems more helpful than returning no results, even though that would be strictly correct. I'm open to alternative opinions here.

Second, testing. The logic I just described is spelt out in the issues and pulls handlers. It is technically possible to extract that into a helper function and unit test it but it would end up looking very trivial. Again, happy to explore testing further if this is desirable.

octet-stream.net submitted #0
diff
7 commits
expand
439d41be
appview/notify: fix entityType in NewIssueState notification
edf823b5
appview/searchquery: add search query parser with negation support
af261776
appview: add label name resolution to LabelState
b5a6d4cf
appview: add search filters for issues and pulls
f171e2c5
appview/indexer: force bleve index rebuild when mapping changes
64a1c37d
appview: re-index issues and pulls when labels are modified
c9ad56db
appview: update tab counts to reflect search filters
expand 1 comment
oppi.li

incredible work! will give this a test