diff --git a/web/src/search_suggestion.ts b/web/src/search_suggestion.ts index dae8519e67..e7a4b6d439 100644 --- a/web/src/search_suggestion.ts +++ b/web/src/search_suggestion.ts @@ -1,6 +1,8 @@ import Handlebars from "handlebars/runtime"; import assert from "minimalistic-assert"; +import render_user_pill from "../templates/user_pill.hbs"; + import * as common from "./common"; import * as direct_message_group_data from "./direct_message_group_data"; import {Filter} from "./filter"; @@ -27,9 +29,29 @@ type TermPattern = Omit & Partial { + return channels.map((channel) => { const prefix = "channel"; const highlighted_channel = highlight_query(regex, channel); const verb = last.negated ? "exclude " : ""; @@ -162,10 +188,8 @@ function get_channel_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Suggest negated: last.negated, }; const search_string = Filter.unparse([term]); - return {description_html, search_string}; + return {description_html, search_string, is_people: false}; }); - - return objs; } function get_group_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Suggestion[] { @@ -197,6 +221,22 @@ function get_group_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Suggestio // operand (not including the last part). const parts = [...all_but_last_part.split(","), people.my_current_email()]; + const all_users_but_last_part = []; + for (const email of all_but_last_part.split(",")) { + const user = people.get_by_email(email); + // Somehow an invalid email is showing up earlier in the group. + // This can happen if e.g. the user manually enters multiple emails. + // We won't have group suggestions built from an invalid user, so + // return an empty list. + if (user === undefined) { + return []; + } + all_users_but_last_part.push(user); + } + const user_pill_contexts = all_users_but_last_part.map((person) => + create_user_pill_context(person), + ); + const person_matcher = people.build_person_matcher(last_part); let persons = people.filter_all_persons((person) => { if (parts.includes(person.email)) { @@ -214,33 +254,30 @@ function get_group_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Suggestio const person_highlighter = make_person_highlighter(last_part); - const suggestions = persons.map((person) => { + return persons.map((person) => { const term = { operator: "dm", operand: all_but_last_part + "," + person.email, negated, }; - // Note that description_html won't contain the user's - // identity; that instead will be rendered in the separate - // user pill. - const description_html = - prefix + Handlebars.Utils.escapeExpression(" " + all_but_last_part + ","); - let terms: NarrowTerm[] = [term]; if (negated) { terms = [{operator: "is", operand: "dm"}, term]; } + const all_user_pill_contexts = [ + ...user_pill_contexts, + highlight_person(person, person_highlighter), + ]; + return { - description_html, + description_html: prefix, search_string: Filter.unparse(terms), - is_person: true, - user_pill_context: highlight_person(person, person_highlighter), + is_people: true, + users: all_user_pill_contexts.map((user_pill_context) => ({user_pill_context})), }; }); - - return suggestions; } function make_people_getter(last: NarrowTerm): () => User[] { @@ -322,7 +359,7 @@ function get_person_suggestions( const person_highlighter = make_person_highlighter(query); - const objs = persons.map((person) => { + return persons.map((person) => { const terms: NarrowTerm[] = [ { operator: autocomplete_operator, @@ -344,17 +381,19 @@ function get_person_suggestions( return { description_html: prefix, search_string: Filter.unparse(terms), - is_person: true, - user_pill_context: highlight_person(person, person_highlighter), + is_people: true, + users: [ + { + user_pill_context: highlight_person(person, person_highlighter), + }, + ], }; }); - - return objs; } function get_default_suggestion_line(terms: NarrowTerm[]): SuggestionLine { if (terms.length === 0) { - return [{description_html: "", search_string: ""}]; + return [{description_html: "", search_string: "", is_people: false}]; } return terms.map((term) => format_as_suggestion([term])); } @@ -512,10 +551,12 @@ function get_term_subset_suggestions(terms: NarrowTerm[]): Suggestion[] { return suggestions; } +type SuggestionAndIncompatiblePatterns = Suggestion & {incompatible_patterns: TermPattern[]}; + function get_special_filter_suggestions( last: NarrowTerm, terms: NarrowTerm[], - suggestions: (Suggestion & {incompatible_patterns: TermPattern[]})[], + suggestions: SuggestionAndIncompatiblePatterns[], ): Suggestion[] { const is_search_operand_negated = last.operator === "search" && last.operand.startsWith("-"); // Negating suggestions on is_search_operand_negated is required for @@ -525,6 +566,7 @@ function get_special_filter_suggestions( search_string: "-" + suggestion.search_string, description_html: "exclude " + suggestion.description_html, incompatible_patterns: suggestion.incompatible_patterns, + is_people: false, })); } @@ -560,10 +602,11 @@ function get_channels_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): if (last.operator === "search" && common.phrase_match(last.operand, "streams")) { search_string = "streams:public"; } - const suggestions = [ + const suggestions: SuggestionAndIncompatiblePatterns[] = [ { search_string, description_html: "All public channels in organization", + is_people: false, incompatible_patterns: [ {operator: "is", operand: "dm"}, {operator: "channel"}, @@ -577,10 +620,11 @@ function get_channels_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): return get_special_filter_suggestions(last, terms, suggestions); } function get_is_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Suggestion[] { - const suggestions = [ + const suggestions: SuggestionAndIncompatiblePatterns[] = [ { search_string: "is:dm", description_html: "direct messages", + is_people: false, incompatible_patterns: [ {operator: "is", operand: "dm"}, {operator: "is", operand: "resolved"}, @@ -592,16 +636,19 @@ function get_is_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Sugge { search_string: "is:starred", description_html: "starred messages", + is_people: false, incompatible_patterns: [{operator: "is", operand: "starred"}], }, { search_string: "is:mentioned", description_html: "@-mentions", + is_people: false, incompatible_patterns: [{operator: "is", operand: "mentioned"}], }, { search_string: "is:followed", description_html: "followed topics", + is_people: false, incompatible_patterns: [ {operator: "is", operand: "followed"}, {operator: "is", operand: "dm"}, @@ -612,16 +659,19 @@ function get_is_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Sugge { search_string: "is:alerted", description_html: "alerted messages", + is_people: false, incompatible_patterns: [{operator: "is", operand: "alerted"}], }, { search_string: "is:unread", description_html: "unread messages", + is_people: false, incompatible_patterns: [{operator: "is", operand: "unread"}], }, { search_string: "is:resolved", description_html: "topics marked as resolved", + is_people: false, incompatible_patterns: [ {operator: "is", operand: "resolved"}, {operator: "is", operand: "dm"}, @@ -644,25 +694,29 @@ function get_is_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Sugge } function get_has_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Suggestion[] { - const suggestions = [ + const suggestions: SuggestionAndIncompatiblePatterns[] = [ { search_string: "has:link", description_html: "messages that contain links", + is_people: false, incompatible_patterns: [{operator: "has", operand: "link"}], }, { search_string: "has:image", description_html: "messages that contain images", + is_people: false, incompatible_patterns: [{operator: "has", operand: "image"}], }, { search_string: "has:attachment", description_html: "messages that contain attachments", + is_people: false, incompatible_patterns: [{operator: "has", operand: "attachment"}], }, { search_string: "has:reaction", description_html: "messages that contain reactions", + is_people: false, incompatible_patterns: [{operator: "has", operand: "reaction"}], }, ]; @@ -699,6 +753,7 @@ function get_sent_by_me_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Sugg { search_string: sender_query, description_html, + is_people: false, }, ]; } @@ -795,27 +850,32 @@ class Attacher { const search_strings = []; for (const suggestion of suggestion_line) { if (suggestion.description_html !== "") { - description_htmls.push(suggestion.description_html); + // To be able to render multiple user pills per suggestion, + // we generate the user pill html here and concatenate it + // together with other parts of the suggestion for the + // Suggestion html. + if (suggestion.is_people) { + const user_pills_html = suggestion.users + .map((user) => render_user_pill(user)) + .join(" "); + description_htmls.push(suggestion.description_html + user_pills_html); + } else { + description_htmls.push(suggestion.description_html); + } } if (suggestion.search_string !== "") { search_strings.push(suggestion.search_string); } } - const last_suggestion = suggestion_line.at(-1); - if (last_suggestion?.is_person) { - const user_pill_context = last_suggestion.user_pill_context; - assert(user_pill_context !== undefined); - return { - description_html: description_htmls.join(", "), - search_string: search_strings.join(" "), - is_person: true, - user_pill_context, - }; - } return { description_html: description_htmls.join(", "), search_string: search_strings.join(" "), + // This is a full suggestion line of multiple suggestions, + // some of which might be people, but people can be suggested + // alongside non-people, so we do the html conversion for user + // suggestions already by this point. + is_people: false, }; }); } @@ -873,6 +933,7 @@ export function get_search_result(query: string): Suggestion[] { description_html: `search for ${Handlebars.Utils.escapeExpression( last.operand, )}`, + is_people: false, }, ]; attacher.push([...attacher.base, ...suggestion_line]); diff --git a/web/templates/search_list_item.hbs b/web/templates/search_list_item.hbs index e324a999f9..e977d28bc7 100644 --- a/web/templates/search_list_item.hbs +++ b/web/templates/search_list_item.hbs @@ -1,8 +1,10 @@
{{{ description_html }}} - {{#if is_person}} - - {{> input_pill user_pill_context}} - + {{#if is_people}} + {{#each users}} + + {{> input_pill user_pill_context}} + + {{/each}} {{/if}}
diff --git a/web/templates/user_pill.hbs b/web/templates/user_pill.hbs new file mode 100644 index 0000000000..5077904136 --- /dev/null +++ b/web/templates/user_pill.hbs @@ -0,0 +1,3 @@ + + {{> input_pill user_pill_context }} + diff --git a/web/tests/search.test.js b/web/tests/search.test.js index bf76e9414a..9a1908da9a 100644 --- a/web/tests/search.test.js +++ b/web/tests/search.test.js @@ -26,11 +26,13 @@ run_test("initialize", ({override, override_rewire, mock_template}) => { mock_template("search_list_item.hbs", true, (data, html) => { assert.equal(typeof data.description_html, "string"); - if (data.is_person) { - assert.equal(typeof data.user_pill_context.id, "number"); - assert.equal(typeof data.user_pill_context.display_value, "string"); - assert.equal(typeof data.user_pill_context.has_image, "boolean"); - assert.equal(typeof data.user_pill_context.img_src, "string"); + if (data.is_people) { + for (const user of data.users) { + assert.equal(typeof user.user_pill_context.id, "number"); + assert.equal(typeof user.user_pill_context.display_value, "string"); + assert.equal(typeof user.user_pill_context.has_image, "boolean"); + assert.equal(typeof user.user_pill_context.img_src, "string"); + } } return html; }); @@ -90,45 +92,57 @@ run_test("initialize", ({override, override_rewire, mock_template}) => { "dm-including:zo", { description_html: "group direct messages including", - is_person: true, + is_people: true, search_string: "dm-including:user7@zulipdev.com", - user_pill_context: { - display_value: "Zoe", - has_image: true, - id: 7, - img_src: - "https://secure.gravatar.com/avatar/0f030c97ab51312c7bbffd3966198ced?d=identicon&version=1&s=50", - }, + users: [ + { + user_pill_context: { + display_value: "Zoe", + has_image: true, + id: 7, + img_src: + "https://secure.gravatar.com/avatar/0f030c97ab51312c7bbffd3966198ced?d=identicon&version=1&s=50", + }, + }, + ], }, ], [ "dm:zo", { description_html: "direct messages with", - is_person: true, + is_people: true, search_string: "dm:user7@zulipdev.com", - user_pill_context: { - display_value: "Zoe", - has_image: true, - id: 7, - img_src: - "https://secure.gravatar.com/avatar/0f030c97ab51312c7bbffd3966198ced?d=identicon&version=1&s=50", - }, + users: [ + { + user_pill_context: { + display_value: "Zoe", + has_image: true, + id: 7, + img_src: + "https://secure.gravatar.com/avatar/0f030c97ab51312c7bbffd3966198ced?d=identicon&version=1&s=50", + }, + }, + ], }, ], [ "sender:zo", { description_html: "sent by", - is_person: true, + is_people: true, search_string: "sender:user7@zulipdev.com", - user_pill_context: { - display_value: "Zoe", - has_image: true, - id: 7, - img_src: - "https://secure.gravatar.com/avatar/0f030c97ab51312c7bbffd3966198ced?d=identicon&version=1&s=50", - }, + users: [ + { + user_pill_context: { + display_value: "Zoe", + has_image: true, + id: 7, + img_src: + "https://secure.gravatar.com/avatar/0f030c97ab51312c7bbffd3966198ced?d=identicon&version=1&s=50", + }, + }, + ], }, ], [ @@ -152,13 +166,13 @@ run_test("initialize", ({override, override_rewire, mock_template}) => { let expected_value = `
\n Search for zo\n
\n`; assert.equal(opts.highlighter_html(source[0]), expected_value); - expected_value = `
\n sent by\n \n
\n \n \n <strong>Zo</strong>e\n
\n \n
\n
\n \n
\n`; + expected_value = `
\n sent by\n \n
\n \n \n <strong>Zo</strong>e\n
\n \n
\n
\n \n
\n`; assert.equal(opts.highlighter_html(source[1]), expected_value); - expected_value = `
\n direct messages with\n \n
\n \n \n <strong>Zo</strong>e\n
\n \n
\n
\n \n
\n`; + expected_value = `
\n direct messages with\n \n
\n \n \n <strong>Zo</strong>e\n
\n \n
\n
\n \n
\n`; assert.equal(opts.highlighter_html(source[2]), expected_value); - expected_value = `
\n group direct messages including\n \n
\n \n \n <strong>Zo</strong>e\n
\n \n
\n
\n \n
\n`; + expected_value = `
\n group direct messages including\n \n
\n \n \n <strong>Zo</strong>e\n
\n \n
\n
\n \n
\n`; assert.equal(opts.highlighter_html(source[3]), expected_value); /* Test sorter */ diff --git a/web/tests/search_suggestion.test.js b/web/tests/search_suggestion.test.js index b5e6b94f50..57f9820033 100644 --- a/web/tests/search_suggestion.test.js +++ b/web/tests/search_suggestion.test.js @@ -132,6 +132,7 @@ test("subset_suggestions", ({mock_template}) => { test("dm_suggestions", ({override, mock_template}) => { mock_template("search_description.hbs", true, (_data, html) => html); + mock_template("user_pill.hbs", true, (_data, html) => html); let query = "is:dm"; let suggestions = get_suggestions(query); @@ -269,6 +270,7 @@ test("dm_suggestions", ({override, mock_template}) => { test("group_suggestions", ({mock_template}) => { mock_template("search_description.hbs", true, (_data, html) => html); + mock_template("user_pill.hbs", true, (_data, html) => html); // Entering a comma in a "dm:" query should immediately // generate suggestions for the next person. @@ -359,6 +361,8 @@ test("group_suggestions", ({mock_template}) => { ]; assert.deepEqual(suggestions.strings, expected); + // Doesn't show ted@ because it's invalid in combination + // with a channel. query = "channel:Denmark has:link dm:bob@zulip.com,Smit"; suggestions = get_suggestions(query); expected = [ @@ -368,6 +372,12 @@ test("group_suggestions", ({mock_template}) => { ]; assert.deepEqual(suggestions.strings, expected); + // Invalid emails don't give suggestions + query = "has:link dm:invalid@zulip.com,Smit"; + suggestions = get_suggestions(query); + expected = ["has:link dm:invalid@zulip.com,Smit", "has:link"]; + assert.deepEqual(suggestions.strings, expected); + function message(user_ids, timestamp) { return { type: "private", @@ -533,6 +543,7 @@ test("has_suggestions", ({override, mock_template}) => { test("check_is_suggestions", ({override, mock_template}) => { mock_template("search_description.hbs", true, (_data, html) => html); + mock_template("user_pill.hbs", true, (_data, html) => html); stream_data.add_sub({stream_id: 44, name: "devel", subscribed: true}); stream_data.add_sub({stream_id: 77, name: "office", subscribed: true}); @@ -708,6 +719,7 @@ test("sent_by_me_suggestions", ({override, mock_template}) => { test("topic_suggestions", ({override, mock_template}) => { mock_template("search_description.hbs", true, (_data, html) => html); + mock_template("user_pill.hbs", true, (_data, html) => html); let suggestions; let expected; @@ -872,6 +884,7 @@ test("channel_completion", ({override, mock_template}) => { test("people_suggestions", ({override, mock_template}) => { mock_template("search_description.hbs", true, (_data, html) => html); + mock_template("user_pill.hbs", true, (_data, html) => html); let query = "te"; @@ -949,68 +962,76 @@ test("people_suggestions", ({override, mock_template}) => { assert.deepEqual(suggestions.strings, expected); function is_person(q) { - return suggestions.lookup_table.get(q).is_person; + return suggestions.lookup_table.get(q).description_html.includes(`class="user-pill`); } assert.equal(is_person("dm:ted@zulip.com"), true); assert.equal(is_person("sender:ted@zulip.com"), true); assert.equal(is_person("dm-including:ted@zulip.com"), true); function has_image(q) { - return suggestions.lookup_table.get(q).user_pill_context.has_image; + return suggestions.lookup_table.get(q).description_html.includes(`class="pill-image"`); } assert.equal(has_image("dm:bob@zulip.com"), true); assert.equal(has_image("sender:bob@zulip.com"), true); assert.equal(has_image("dm-including:bob@zulip.com"), true); - function describe(q) { - return suggestions.lookup_table.get(q).description_html; + function test_describe(q, description_html_start) { + assert.ok( + suggestions.lookup_table.get(q).description_html.startsWith(description_html_start), + ); } - assert.equal(describe("dm:ted@zulip.com"), "Direct messages with"); - assert.equal(describe("sender:ted@zulip.com"), "Sent by"); - assert.equal(describe("dm-including:ted@zulip.com"), "Direct messages including"); + test_describe("dm:ted@zulip.com", "Direct messages with"); + test_describe("sender:ted@zulip.com", "Sent by"); + test_describe("dm-including:ted@zulip.com", "Direct messages including"); let expectedString = "Ted Smith"; - function get_full_name(q) { - return suggestions.lookup_table.get(q).user_pill_context.display_value.string; + function test_full_name(q, full_name_html) { + return suggestions.lookup_table.get(q).description_html.includes(full_name_html); } - assert.equal(get_full_name("sender:ted@zulip.com"), expectedString); - assert.equal(get_full_name("dm:ted@zulip.com"), expectedString); - assert.equal(get_full_name("dm-including:ted@zulip.com"), expectedString); + test_full_name("sender:ted@zulip.com", expectedString); + test_full_name("dm:ted@zulip.com", expectedString); + test_full_name("dm-including:ted@zulip.com", expectedString); expectedString = example_avatar_url + "?s=50"; - function get_avatar_url(q) { - return suggestions.lookup_table.get(q).user_pill_context.img_src; + function test_avatar_url(q, avatar_url) { + return suggestions.lookup_table.get(q).description_html.includes(avatar_url); } - assert.equal(get_avatar_url("dm:bob@zulip.com"), expectedString); - assert.equal(get_avatar_url("sender:bob@zulip.com"), expectedString); - assert.equal(get_avatar_url("dm-including:bob@zulip.com"), expectedString); + test_avatar_url("dm:bob@zulip.com", expectedString); + test_avatar_url("sender:bob@zulip.com", expectedString); + test_avatar_url("dm-including:bob@zulip.com", expectedString); - function get_should_add_guest_user_indicator(q) { - return suggestions.lookup_table.get(q).user_pill_context.should_add_guest_user_indicator; + const guest_html = "(guest)"; + function test_guest_user_indicator(q, should_have_guest_indicator) { + const description_html = suggestions.lookup_table.get(q).description_html; + if (should_have_guest_indicator) { + assert.ok(description_html.includes(guest_html)); + } else { + assert.ok(!description_html.includes(guest_html)); + } } realm.realm_enable_guest_user_indicator = true; suggestions = get_suggestions(query); - assert.equal(get_should_add_guest_user_indicator("dm:bob@zulip.com"), false); - assert.equal(get_should_add_guest_user_indicator("sender:bob@zulip.com"), false); - assert.equal(get_should_add_guest_user_indicator("dm-including:bob@zulip.com"), false); + test_guest_user_indicator("dm:bob@zulip.com", false); + test_guest_user_indicator("sender:bob@zulip.com", false); + test_guest_user_indicator("dm-including:bob@zulip.com", false); bob.is_guest = true; suggestions = get_suggestions(query); - assert.equal(get_should_add_guest_user_indicator("dm:bob@zulip.com"), true); - assert.equal(get_should_add_guest_user_indicator("sender:bob@zulip.com"), true); - assert.equal(get_should_add_guest_user_indicator("dm-including:bob@zulip.com"), true); + test_guest_user_indicator("dm:bob@zulip.com", true); + test_guest_user_indicator("sender:bob@zulip.com", true); + test_guest_user_indicator("dm-including:bob@zulip.com", true); realm.realm_enable_guest_user_indicator = false; suggestions = get_suggestions(query); - assert.equal(get_should_add_guest_user_indicator("dm:bob@zulip.com"), false); - assert.equal(get_should_add_guest_user_indicator("sender:bob@zulip.com"), false); - assert.equal(get_should_add_guest_user_indicator("dm-including:bob@zulip.com"), false); + test_guest_user_indicator("dm:bob@zulip.com", false); + test_guest_user_indicator("sender:bob@zulip.com", false); + test_guest_user_indicator("dm-including:bob@zulip.com", false); suggestions = get_suggestions("Ted "); // note space