-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
search-suggestions: Remove is:dm suggestion from get_operator_suggestions #30330
Conversation
@@ -23,6 +23,7 @@ run_test("phrase_match", () => { | |||
assert.ok(common.phrase_match("Tes", "test")); | |||
assert.ok(common.phrase_match("Tes", "Test")); | |||
assert.ok(common.phrase_match("tes", "Stream Test")); | |||
assert.ok(common.phrase_match("", "test")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer I move this change to a separate commit? I can do that if so. I added this to make it clear phrase_match
matches on empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to ask you to split this so late in review for how much benefit it'll have, but yeah probably it'd have been better to do this in its own commit, with the commit explaining this change.
Thanks for the PR @joyhchen! @pratik-pc would you be up for reviewing this? I think you've been working in that code path recently. |
a44483d
to
5212f46
Compare
5212f46
to
dc3e8ca
Compare
dc3e8ca
to
e69bc2a
Compare
…ions Previously, is: searches as a spectator would suggest Direct messages even though DMs aren't accessible for spectators. This commit removes the is:dm suggestion from operator suggestions and adds the is:dm suggestion to get_is_filter_suggestions, which only runs for non-spectators
e69bc2a
to
f24d410
Compare
@pratik-pc moved the code to |
This looks good to me now @timabbott I think this is ready now |
@pratik-pc Have you manually tested this PR? |
The commit message isn't properly formatted. Check out our GitHub guide and commit guidelines for more details. |
Merged, thanks @joyhchen and @pratik-pc, after testing manually and rewriting the commit message. I think this may not be the best possible implementation, though; it feels like the ideal setup would be for us to be able to declare
|
Fixes: #30320
Currently, there are 2 lists of filterers
When you type
is:
in the search bar as a spectator, it currently suggests "Direct messages" (Issue 30320) because get_operator_suggestions is adding the suggestion.This PR
is:dm
suggestion fromget_operator_suggestions
get_is_operator_suggestions
doesn't apply to spectators, we no longer suggest "Direct messages" when the user searches "is:" (see screenshot)is:dm
suggestion will be populated fromget_is_filter_suggestions
Alternatives considered
visible_to_spectators
on theSuggestion
object. This would fix the core problem that suggestions that spectators can't access still appear to them.Screenshots and screen captures:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: