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

search: Show category-wise suggestions for has operator. #9453

Closed
wants to merge 3 commits into from

Conversation

shubham-padia
Copy link
Member

@shubham-padia shubham-padia commented May 18, 2018

Commit 1:
Default suggestion e.g messages with one or more abc as a suggestion for has:abc is not shown in a new suggestion. But if the has operator is already present before any other operator, the default message text will be used. e.g has:abc sender:abc@zulipchat.com will have all the suggestions with the prefix messages with one or more abc, sent by abc@zulipchat.com.

selection_122

selection_121

Commit 2:
has operator uses predefined categories. This commit displays an invalid operand message if the operand does not fall in to any of these categories and the has operator is not at the last.
e.g. has:abc sender:abc@zulipchat.com will have invalid abc operand for has operator, sent by abc@zulipchat.com as a prefix for all its suggestions.
selection_123

Commit 3:
Another issue was getting the default suggestion messages with one or more when getting the suggestions for chosing the operator as shown below:
screenshot from 2018-05-18 11-40-48

This commit takes care of that issue.
Results:
screenshot from 2018-05-18 12-21-27

Note: categories exist for the is operator, removal of default suggestion needs to be done.

Fixes zulip#9384.
Default suggestion e.g `messages with one or more abc` as a suggestion
for `has:abc` is not shown in a new suggestion. But if the has operator
is already present before any other operator, the default message text
will be used. e.g `has:abc sender:abc@zulipchat.com` will have all the
suggestions with the prefix `messages with one or more abc, sent by
abc@zulipchat.com`.
`has` operator uses predefined categories. This commit displays an
invalid operand message if the operand does not fall in to any of
these categories and the `has` operator is not at the last.
e.g. `has:abc sender:abc@zulipchat.com` will have `invalid abc
operand for has operator, sent by abc@zulipchat.com` as a prefix for
all its suggestions.
When suggesting operators to chose, category wise suggestions are
shown instead of a single default suggestion. e.g suggestions for
all the categories of has operator will be show instead of `Messages
with one or more` suggestion which did not make sense.
@zulipbot
Copy link
Member

Hello @zulip/server-search members, this pull request was labeled with the "area: search" label, so you may want to check it out!

});

run_test('has_operator_suggestions', () => {
// Checks that category wise suggestions are displayed instead of a single
Copy link
Member Author

Choose a reason for hiding this comment

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

Although it is checked in the test before this i.e empty_query_suggestions that category wise has operators are shown, changes to that test may push the has suggestions down as they have a low priority. That is why a second test was added.

@@ -583,6 +618,9 @@ exports.get_suggestions = function (query) {
suggestions = get_operator_suggestions(last);
attach_suggestions(result, base, suggestions);

suggestions = get_has_filter_suggestions(last, base_operators);
Copy link
Member Author

Choose a reason for hiding this comment

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

Seperate functions for is and has filters were created instead of adding the suggestion for has into special_filter_suggestions to take care of the different priorities of the suggestions for both the filters. has suggestions were previously obtained from get_operator_suggestions, therefore placing it directly below that. Also it makes sense for has filter to have a low priority than is as filters like private messages and starred messages maybe used a lot more than has operators.

@timabbott
Copy link
Sponsor Member

Merged, thanks @shubham-padia! I think there's a clear follow-up here, though, that we should do the same fix for limiting to just valid operators the is: filters as well. Can you take care of that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants