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

fix(search): hide feedback from issues search by default #76984

Merged
merged 9 commits into from
Sep 9, 2024

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Sep 5, 2024

Follow up to #59505 to filter feedbacks from the default search when querying Postgres. As before, filtering on issue.category:feedback or a negation (ex !issue.category:error) will still return feedback issues.

Fixes #60697
Fixes https://github.com/getsentry/team-replay/issues/470

@aliu39 aliu39 requested review from a team as code owners September 5, 2024 05:36
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 5, 2024
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #76984      +/-   ##
==========================================
+ Coverage   78.16%   78.24%   +0.08%     
==========================================
  Files        6910     6899      -11     
  Lines      307185   306905     -280     
  Branches    50342    50265      -77     
==========================================
+ Hits       240097   240143      +46     
+ Misses      60720    60402     -318     
+ Partials     6368     6360       -8     

else:
all_group_type_objs: list[GroupType] = [
gt_registry.get_by_type_id(id)
for id in gt_registry.get_all_group_type_ids()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do this like this, on line 1828

if not is_errors:
    visible_group_types = group_types_from(search_filters)
    if visible_group_types is None:
        # When no group types are specified, default to all default group types
        visible_group_types = [gt for gt in all_group_type_objs if not gt.in_default_search]
    # remove `raw_group_types is not None` check, since it'll never be none now, just potentially empty
    # Then just run the same logic we were running before

This keeps the logic shared which should reduce the chance of bugs. Since in your new branch you missed grouptype.registry.get_visible, and so we'd have started showing unreleased issue types to all users.

Reading this code, this might actually be a bug already... if no issue types are specified, we're not filtering to visible issue types only. So doing this will fix the bug

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, this line should be like this right?
[gt for gt in all_group_type_objs if not gt.in_default_search]

Copy link
Member Author

Choose a reason for hiding this comment

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

the visible filter seems to happen in the if not None case

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the logic to the base case of group_types_from. For some reason this causes test_snuba_perf_issue to fail, not sure why, it worked once locally so could be flakey.

Copy link
Member Author

@aliu39 aliu39 Sep 6, 2024

Choose a reason for hiding this comment

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

For the test, it's because the GroupType created by storing a transaction, 2001 ProfileFileIOGroupType, is not registered.

Not sure how GroupTypes are registered in prod, but this change makes it so by default, issues with unregistered GroupTypes are never listed (before, we would just skip the filter).

Copy link
Member Author

Choose a reason for hiding this comment

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

this change makes it so by default, issues with unregistered GroupTypes are never listed (before, we would just skip the filter).

Do we expect all group types to be registered in prod? Is it ok to manually register the 2001 type to pass this test?

Copy link
Member

Choose a reason for hiding this comment

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

Not totally sure what you mean by it not being registered? It might just be because the group type hasn't been marked as visible?

You can do that like

with self.feature([ProfileFileIOGroupType.build_visible_feature_name()]):

Copy link
Member Author

Choose a reason for hiding this comment

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

ohh got it, that was it, thanks!

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Nice, thanks for fixing this!

@aliu39 aliu39 merged commit f95b37b into master Sep 9, 2024
50 checks passed
@aliu39 aliu39 deleted the aliu/filter-feedback-from-issues branch September 9, 2024 18:03
c298lee pushed a commit that referenced this pull request Sep 10, 2024
Follow up to #59505 to filter
feedbacks from the default search when querying Postgres. As before,
filtering on issue.category:feedback or a negation (ex
!issue.category:error) will still return feedback issues.

Fixes #60697
Fixes getsentry/team-replay#470
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Issue only search doesn't correctly filter out disabled issue types
3 participants