From 02ea52fc186a96a420598cf3c05f53ef69b5838c Mon Sep 17 00:00:00 2001 From: Mohit Gupta Date: Sat, 13 Jun 2020 22:35:31 +0530 Subject: [PATCH] filter: Implement has: filters on client side. Prior to this commit has:link, has:attachment, has:image filter couldn't be applied locally and deferred filtering to web server. This commits make sure client filters all messages it can instead of completely deferring to the server and hence improve speed. A tradeoff is also made to turn off local echo for has: narrows as messages with link sent to has:link narrow were locally echoing to another narrow and not appearing in the active has:link narrow. Fixes: #6186. --- frontend_tests/node_tests/filter.js | 62 ++++++++++++++++++++++++++++- static/js/echo.js | 2 +- static/js/filter.js | 23 ++++++++--- static/js/message_util.js | 12 ++++++ 4 files changed, 92 insertions(+), 7 deletions(-) diff --git a/frontend_tests/node_tests/filter.js b/frontend_tests/node_tests/filter.js index 140207b2fc..729a2ef84e 100644 --- a/frontend_tests/node_tests/filter.js +++ b/frontend_tests/node_tests/filter.js @@ -3,6 +3,9 @@ zrequire('unread'); zrequire('stream_data'); zrequire('people'); set_global('Handlebars', global.make_handlebars()); +global.stub_out_jquery(); +set_global('$', global.make_zjquery()); +zrequire('message_util', 'js/message_util'); zrequire('Filter', 'js/filter'); set_global('message_store', {}); @@ -121,7 +124,8 @@ run_test('basics', () => { ]; filter = new Filter(operators); assert(filter.has_operator('has')); - assert(!filter.can_apply_locally()); + assert(filter.can_apply_locally()); + assert(!filter.can_apply_locally(true)); assert(!filter.includes_full_stream_history()); assert(!filter.can_mark_messages_read()); assert(!filter.is_personal_filter()); @@ -715,6 +719,62 @@ run_test('predicate_basics', () => { display_recipient: [{id: steve.user_id}, {id: me.user_id}], })); assert(!predicate({type: 'stream'})); + + const img_msg = { + content: `

test.jpeg

`, + }; + + const link_msg = { + content: `

chat.zulip.org

`, + }; + + const non_img_attachment_msg = { + content: `

attachment.ext

`, + }; + + const no_has_filter_matching_msg = { + content: "

Testing

", + }; + + predicate = get_predicate([['has', 'non_valid_operand']]); + assert(!predicate(img_msg)); + assert(!predicate(non_img_attachment_msg)); + assert(!predicate(link_msg)); + assert(!predicate(no_has_filter_matching_msg)); + + // HTML content of message is used to determine if image have link, image or attachment. + // We are using jquery to parse the html and find existence of relevant tags/elements. + // In tests we need to stub the calls to jquery so using zjquery's .set_find_results method. + const has_link = get_predicate([['has', 'link']]); + $(img_msg.content).set_find_results("a", [$("")]); + assert(has_link(img_msg)); + $(non_img_attachment_msg.content).set_find_results("a", [$("")]); + assert(has_link(non_img_attachment_msg)); + $(link_msg.content).set_find_results("a", [$("")]); + assert(has_link(link_msg)); + $(no_has_filter_matching_msg.content).set_find_results("a", false); + assert(!has_link(no_has_filter_matching_msg)); + + const has_attachment = get_predicate([['has', 'attachment']]); + $(img_msg.content).set_find_results("a[href^='/user_uploads']", [$("")]); + assert(has_attachment(img_msg)); + $(non_img_attachment_msg.content).set_find_results("a[href^='/user_uploads']", [$("")]); + assert(has_attachment(non_img_attachment_msg)); + $(link_msg.content).set_find_results("a[href^='/user_uploads']", false); + assert(!has_attachment(link_msg)); + $(no_has_filter_matching_msg.content).set_find_results("a[href^='/user_uploads']", false); + assert(!has_attachment(no_has_filter_matching_msg)); + + const has_image = get_predicate([['has', 'image']]); + $(img_msg.content).set_find_results(".message_inline_image", [$("")]); + assert(has_image(img_msg)); + $(non_img_attachment_msg.content).set_find_results(".message_inline_image", false); + assert(!has_image(non_img_attachment_msg)); + $(link_msg.content).set_find_results(".message_inline_image", false); + assert(!has_image(link_msg)); + $(no_has_filter_matching_msg.content).set_find_results(".message_inline_image", false); + assert(!has_image(no_has_filter_matching_msg)); + }); run_test('negated_predicates', () => { diff --git a/static/js/echo.js b/static/js/echo.js index b3714abdeb..aa48e6f801 100644 --- a/static/js/echo.js +++ b/static/js/echo.js @@ -146,7 +146,7 @@ exports.try_deliver_locally = function (message_request) { return; } - if (narrow_state.active() && !narrow_state.filter().can_apply_locally()) { + if (narrow_state.active() && !narrow_state.filter().can_apply_locally(true)) { return; } diff --git a/static/js/filter.js b/static/js/filter.js index 231e1ef0f3..436a742f27 100644 --- a/static/js/filter.js +++ b/static/js/filter.js @@ -46,6 +46,15 @@ function message_in_home(message) { function message_matches_search_term(message, operator, operand) { switch (operator) { + case 'has': + if (operand === 'image') { + return message_util.message_has_image(message); + } else if (operand === 'link') { + return message_util.message_has_link(message); + } else if (operand === 'attachment') { + return message_util.message_has_attachment(message); + } + return false; // has:something_else returns false case 'is': if (operand === 'private') { return message.type === 'private'; @@ -620,7 +629,10 @@ Filter.prototype = { return this.has_operand("is", "mentioned") || this.has_operand("is", "starred"); }, - can_apply_locally: function () { + can_apply_locally: function (is_local_echo) { + // Since there can be multiple operators, each block should + // just return false here. + if (this.is_search()) { // The semantics for matching keywords are implemented // by database plugins, and we don't have JS code for @@ -629,10 +641,11 @@ Filter.prototype = { return false; } - if (this.has_operator('has')) { - // See #6186 to see why we currently punt on 'has:foo' - // queries. This can be fixed, there are just some random - // complications that make it non-trivial. + if (this.has_operator('has') && is_local_echo) { + // The has: operators can be applied locally for messages + // rendered by the backend; links, attachments, and images + // are not handled properly by the local echo markdown + // processor. return false; } diff --git a/static/js/message_util.js b/static/js/message_util.js index 8467542bef..dca374b1e3 100644 --- a/static/js/message_util.js +++ b/static/js/message_util.js @@ -16,6 +16,18 @@ function add_messages(messages, msg_list, opts) { return render_info; } +exports.message_has_link = function (message) { + return $(message.content).find(`a`).length > 0; +}; + +exports.message_has_image = function (message) { + return $(message.content).find(`.message_inline_image`).length > 0; +}; + +exports.message_has_attachment = function (message) { + return $(message.content).find(`a[href^='/user_uploads']`).length > 0; +}; + exports.add_old_messages = function (messages, msg_list) { return add_messages(messages, msg_list, {messages_are_new: false}); };