feat(issue-states): Add ability to search for substatus #48289

snigdhas · 2023-05-01T21:54:28Z

Adds substatuses to the approved values for the is: search filter.

WOR-2898

codecov · 2023-05-01T22:41:04Z

Codecov Report

Merging #48289 (382320e) into master (f086416) will increase coverage by 0.01%.
The diff coverage is 97.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #48289      +/-   ##
==========================================
+ Coverage   80.88%   80.90%   +0.01%     
==========================================
  Files        4770     4777       +7     
  Lines      201632   201877     +245     
  Branches    11557    11548       -9     
==========================================
+ Hits       163096   163324     +228     
- Misses      38280    38298      +18     
+ Partials      256      255       -1

Impacted Files	Coverage Δ
src/sentry/api/serializers/models/group.py	`97.03% <ø> (ø)`
static/app/utils/fields/index.ts	`62.50% <ø> (ø)`
static/app/views/replays/detail/network/index.tsx	`0.00% <ø> (ø)`
src/sentry/api/issue_search.py	`97.11% <95.00%> (-0.54%)`	⬇️
src/sentry/models/group.py	`93.68% <100.00%> (+0.01%)`	⬆️
src/sentry/search/utils.py	`88.96% <100.00%> (+0.17%)`	⬆️
src/sentry/types/group.py	`100.00% <100.00%> (ø)`

... and 161 files with indirect coverage changes

lobsterkatie

A few small questions and stylistic nits - feel free to do with them what you will.

Someone more familiar with this part of the codebase should also approve this, but just taking what's here at face value, overall LGTM!

lobsterkatie · 2023-05-02T05:13:23Z

src/sentry/api/issue_search.py

 for status_key, status_value in STATUS_QUERY_CHOICES.items():
    is_filter_translation[status_key] = ("status", status_value)

+for substatus_key, substatus_value in SUBSTATUS_UPDATE_CHOICES.items():


I'm not familiar enough with this stuff to know: is there a reason one of these is query choices and one is update choices? Do they actually serve different purposes?

I think I just ended up naming SUBSTATUS_UPDATE_CHOICES based on what I was using it for at the time. They should serve the same purpose.

lobsterkatie · 2023-05-02T05:13:31Z

src/sentry/api/issue_search.py

+def convert_substatus_value(
+    value: Iterable[Union[str, int]],
+    projects: Sequence[Project],
+    user: User,
+    environments: Optional[Sequence[Environment]],
+) -> List[int]:


Suggested change

def convert_substatus_value(

value: Iterable[Union[str, int]],

projects: Sequence[Project],

user: User,

environments: Optional[Sequence[Environment]],

) -> List[int]:

def convert_substatus_value(

value: Iterable[str | int],

projects: Sequence[Project],

user: User,

environments: Sequence[Environment] | None,

) -> list[int]:

Supposedly the new-fangled way is preferred, though it's making me wonder - do you need a =None on environments if you use | None instead of Optional? The docs don't say so (and I know in JS you wouldn't) but do you in Python?

I think I agree with this answer - the Optional typing + default argument is easier to read than X | None = None

I personally like Optional vs <type> | None, but I've seen it done both ways throughout the codebase.

src/sentry/api/issue_search.py

src/sentry/search/utils.py

tests/sentry/api/test_issue_search.py

tests/snuba/search/test_backend.py

barkbarkimashark · 2023-05-02T19:53:00Z

tests/snuba/search/test_backend.py

+            InvalidSearchQuery, match="The substatus filter is not supported for this organization"
+        ):
+            self.make_query(search_filter_query="is:ongoing")
+


Is it legal to filter on incompatible status and substatus values, i.e. `search_filter_query="is:ongoing is:resolved"?

What about compatible combinations: search_filter_query="is:ongoing is:unresolved"?

I wonder what will happen with: search_filter_query="is:ongoing is:forever"?

This ticket should address that

barkbarkimashark · 2023-05-02T19:57:05Z

src/sentry/api/issue_search.py

+            if search_filter.key.name == "substatus":
+                if not features.has("organizations:issue-states", organization):
+                    raise InvalidSearchQuery(
+                        "The substatus filter is not supported for this organization"


Sine this is behind a feature flag, should be return the same error message as if the filtering on substatus values doesn't exist:

Either way is fine with me - @robinrendle do you have a preference here?

I think it should be easy enough to change the predicate to:

if search_filter.key.name == "substatus" and features.has("organizations:issue-states", organization): # do logic

and just let it fall through if the above is False.

barkbarkimashark · 2023-05-02T20:02:07Z

src/sentry/api/issue_search.py

+                    )
+
+                status = GROUP_SUBSTATUS_TO_STATUS_MAP.get(
+                    new_value[0] if isinstance(new_value, list) else new_value


Hmm, not sure if grabbing the first substatus value here would be good UX. Should we raise an error instead or let the query go through?

Although, not sure if the is: query syntax even allows list values so we might be ok here.

new_value is [0] when searching for is:unresolved. Not sure why..

Added error handling if the length is >1 though.

wedamija

Tbh I wonder if we should have indexed on project, substatus, ... in retrospect. Each substatus only ever has one status, and it'd simplify this pr a bit

wedamija · 2023-05-03T00:15:29Z

src/sentry/search/utils.py

-    if value in STATUS_QUERY_CHOICES.values():
-        return int(value)
+def parse_status_value(status: Union[str, int]) -> int:
+    if status in STATUS_QUERY_CHOICES.keys():


Better off not using .keys here, this changes this from an O(1) operation to O(N)

(wip) add substatuses to search

dde6e2a

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 1, 2023

vercel bot deployed to Preview May 1, 2023 21:57 View deployment

missing a thing

0d6f6af

vercel bot deployed to Preview May 1, 2023 22:11 View deployment

Add tests for substatus

035ab4d

snigdhas changed the title ~~(wip) add substatuses to search~~ feat(issue-states): Add ability to search for substatus May 1, 2023

snigdhas added this to the Issue States and Filters milestone May 1, 2023

snigdhas requested review from barkbarkimashark and a team May 1, 2023 22:34

vercel bot deployed to Preview May 1, 2023 22:36 View deployment

More types

8a6b474

vercel bot deployed to Preview May 1, 2023 23:10 View deployment

snigdhas marked this pull request as ready for review May 2, 2023 00:12

snigdhas requested a review from a team May 2, 2023 00:12

snigdhas requested a review from a team as a code owner May 2, 2023 00:12

snigdhas requested a review from a team May 2, 2023 00:12

lobsterkatie approved these changes May 2, 2023

View reviewed changes

Address review comments

daaab32

vercel bot deployed to Preview May 2, 2023 17:56 View deployment

barkbarkimashark reviewed May 2, 2023

View reviewed changes

Add error handling

782255f

snigdhas requested a review from barkbarkimashark May 3, 2023 00:16

wedamija reviewed May 3, 2023

View reviewed changes

vercel bot deployed to Preview May 3, 2023 00:18 View deployment

No keys

382320e

vercel bot deployed to Preview May 3, 2023 00:29 View deployment

snigdhas merged commit 484b4cf into master May 3, 2023

snigdhas deleted the snigdha/search-ongoing branch May 3, 2023 21:28

github-actions bot locked and limited conversation to collaborators Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(issue-states): Add ability to search for substatus #48289

feat(issue-states): Add ability to search for substatus #48289

snigdhas commented May 1, 2023 •

edited

Loading

codecov bot commented May 1, 2023 •

edited

Loading

lobsterkatie left a comment

lobsterkatie May 2, 2023

snigdhas May 2, 2023

lobsterkatie May 2, 2023

snigdhas May 2, 2023

barkbarkimashark May 2, 2023 •

edited

Loading

barkbarkimashark May 2, 2023

snigdhas May 2, 2023

barkbarkimashark May 2, 2023

snigdhas May 2, 2023

barkbarkimashark May 3, 2023 •

edited

Loading

barkbarkimashark May 2, 2023

snigdhas May 3, 2023

wedamija left a comment

wedamija May 3, 2023

feat(issue-states): Add ability to search for substatus #48289

feat(issue-states): Add ability to search for substatus #48289

Conversation

snigdhas commented May 1, 2023 • edited Loading

codecov bot commented May 1, 2023 • edited Loading

Codecov Report

lobsterkatie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barkbarkimashark May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barkbarkimashark May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wedamija left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snigdhas commented May 1, 2023 •

edited

Loading

codecov bot commented May 1, 2023 •

edited

Loading

barkbarkimashark May 2, 2023 •

edited

Loading

barkbarkimashark May 3, 2023 •

edited

Loading