From 81ea09be1963856ed18bf42e7a2670d5f0f32b42 Mon Sep 17 00:00:00 2001 From: nimish Date: Wed, 25 Oct 2023 23:14:52 +0530 Subject: [PATCH] search: Add is:followed filter. Create the is:followed search operator. Fetch all messages that are from followed topics using exists. Update API documentation and changelog. Co-authored-by: Kenneth Rodrigues Fixes #27309. --- api_docs/changelog.md | 10 ++++++ api_docs/construct-narrow.md | 8 +++-- help/search-for-messages.md | 1 + version.py | 2 +- web/src/filter.ts | 25 +++++++++++++++ web/src/narrow_banner.ts | 4 +++ web/src/search_suggestion.ts | 10 ++++++ web/templates/search_description.hbs | 4 +++ web/templates/search_operators.hbs | 6 ++++ web/tests/filter.test.js | 27 ++++++++++++++++ web/tests/message_view.test.js | 7 +++++ web/tests/search_suggestion.test.js | 16 +++++++++- zerver/lib/narrow.py | 6 +++- zerver/lib/narrow_predicate.py | 5 +++ zerver/lib/topic_sqlalchemy.py | 22 ++++++++++++- zerver/tests/test_message_fetch.py | 47 ++++++++++++++++++++++++++++ 16 files changed, 194 insertions(+), 6 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index b4b22599a8..3556be3e84 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,16 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 9.0 +**Feature level 265** + +* [`GET /messages`](/api/get-messages), + [`GET /messages/matches_narrow`](/api/check-messages-match-narrow), + [`POST /messages/flags/narrow`](/api/update-message-flags-for-narrow), + [`POST /register`](/api/register-queue): + Added a new [search/narrow filter](/api/construct-narrow), + `is:followed`, matching messages in topics that the current user is + [following](/help/follow-a-topic). + **Feature level 264** * `PATCH /realm`, [`POST /register`](/api/register-queue), diff --git a/api_docs/construct-narrow.md b/api_docs/construct-narrow.md index 49b6effe15..0f69e7812f 100644 --- a/api_docs/construct-narrow.md +++ b/api_docs/construct-narrow.md @@ -51,8 +51,12 @@ important optimization when fetching messages in certain cases (e.g. when [adding the `read` flag to a user's personal messages](/api/update-message-flags-for-narrow)). -**Changes**: In Zulip 9.0 (feature level 250), support was added for -two filters related to channel messages: `channel` and `channels`. The +**Changes**: In Zulip 9.0 (feature level 265), support was added for a +new `is:followed` filter, matching messages in topics that the current +user is [following](/help/follow-a-topic). + +In Zulip 9.0 (feature level 250), support was added for +two filters related to stream messages: `channel` and `channels`. The `channel` operator is an alias for the `stream` operator. The `channels` operator is an alias for the `streams` operator. Both `channel` and `channels` return the same exact results as `stream` and `streams` diff --git a/help/search-for-messages.md b/help/search-for-messages.md index a6237b678b..9d55e47bef 100644 --- a/help/search-for-messages.md +++ b/help/search-for-messages.md @@ -116,6 +116,7 @@ Zulip offers the following filters based on the location of the message. ### Search by message status +* `is:followed`: Search messages in [followed topics](/help/follow-a-topic). * `is:resolved`: Search messages in [resolved topics](/help/resolve-a-topic). * `-is:resolved`: Search messages in [unresolved topics](/help/resolve-a-topic). * `is:unread`: Search your unread messages. diff --git a/version.py b/version.py index 9bd8a05d94..8e259142b8 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 264 +API_FEATURE_LEVEL = 265 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/web/src/filter.ts b/web/src/filter.ts index 2e8d04c2e3..4bb7eef6a5 100644 --- a/web/src/filter.ts +++ b/web/src/filter.ts @@ -165,6 +165,11 @@ function message_matches_search_term(message: Message, operator: string, operand return unread.message_unread(message); case "resolved": return message.type === "stream" && resolved_topic.is_resolved(message.topic); + case "followed": + return ( + message.type === "stream" && + user_topics.is_topic_followed(message.stream_id, message.topic) + ); default: return false; // is:whatever returns false } @@ -503,6 +508,7 @@ export class Filter { "is-starred", "is-unread", "is-resolved", + "is-followed", "has-link", "has-image", "has-attachment", @@ -753,6 +759,8 @@ export class Filter { "not-is-dm", "is-resolved", "not-is-resolved", + "is-followed", + "not-is-followed", "in-home", "in-all", "channels-public", @@ -853,6 +861,9 @@ export class Filter { if (_.isEqual(term_types, ["sender"])) { return true; } + if (_.isEqual(term_types, ["is-followed"])) { + return true; + } if ( _.isEqual(term_types, ["sender", "has-reaction"]) && this.operands("sender")[0] === people.my_current_email() @@ -924,6 +935,8 @@ export class Filter { return "/#narrow/dm/" + people.emails_to_slug(this.operands("dm").join(",")); case "is-resolved": return "/#narrow/topics/is/resolved"; + case "is-followed": + return "/#narrow/topics/is/followed"; // TODO: It is ambiguous how we want to handle the 'sender' case, // we may remove it in the future based on design decisions case "sender": @@ -988,6 +1001,9 @@ export class Filter { case "is-resolved": icon = "check"; break; + case "is-followed": + zulip_icon = "follow"; + break; default: icon = undefined; break; @@ -1069,6 +1085,8 @@ export class Filter { return $t({defaultMessage: "Direct message feed"}); case "is-resolved": return $t({defaultMessage: "Topics marked as resolved"}); + case "is-followed": + return $t({defaultMessage: "Followed topics"}); // These cases return false for is_common_narrow, and therefore are not // formatted in the message view header. They are used in narrow.js to // update the browser title. @@ -1103,6 +1121,13 @@ export class Filter { }), link: "/help/star-a-message#view-your-starred-messages", }; + case "is-followed": + return { + description: $t({ + defaultMessage: "Messages in topics you follow.", + }), + link: "/help/follow-a-topic", + }; } if ( _.isEqual(term_types, ["sender", "has-reaction"]) && diff --git a/web/src/narrow_banner.ts b/web/src/narrow_banner.ts index 66c1e197a3..6f9ff42979 100644 --- a/web/src/narrow_banner.ts +++ b/web/src/narrow_banner.ts @@ -261,6 +261,10 @@ function pick_empty_narrow_banner(): NarrowBannerData { return { title: $t({defaultMessage: "No topics are marked as resolved."}), }; + case "followed": + return { + title: $t({defaultMessage: "You aren't following any topics."}), + }; } // fallthrough to default case if no match is found break; diff --git a/web/src/search_suggestion.ts b/web/src/search_suggestion.ts index 5d8afb0638..dae8519e67 100644 --- a/web/src/search_suggestion.ts +++ b/web/src/search_suggestion.ts @@ -599,6 +599,16 @@ function get_is_filter_suggestions(last: NarrowTerm, terms: NarrowTerm[]): Sugge description_html: "@-mentions", incompatible_patterns: [{operator: "is", operand: "mentioned"}], }, + { + search_string: "is:followed", + description_html: "followed topics", + incompatible_patterns: [ + {operator: "is", operand: "followed"}, + {operator: "is", operand: "dm"}, + {operator: "dm"}, + {operator: "dm-including"}, + ], + }, { search_string: "is:alerted", description_html: "alerted messages", diff --git a/web/templates/search_description.hbs b/web/templates/search_description.hbs index f30d2daa05..dbeb7d465d 100644 --- a/web/templates/search_description.hbs +++ b/web/templates/search_description.hbs @@ -31,6 +31,10 @@ {{~!-- squash whitespace --~}} {{this.verb}}topics marked as resolved {{~!-- squash whitespace --~}} + {{else if (eq this.operand "followed")}} + {{~!-- squash whitespace --~}} + {{this.verb}}followed topics + {{~!-- squash whitespace --~}} {{else}} {{~!-- squash whitespace --~}} invalid {{this.operand}} operand for is operator diff --git a/web/templates/search_operators.hbs b/web/templates/search_operators.hbs index 770d9d8c5d..5ce4128118 100644 --- a/web/templates/search_operators.hbs +++ b/web/templates/search_operators.hbs @@ -129,6 +129,12 @@ {{t 'Narrow to messages in resolved topics.'}} + + is:followed + + {{t 'Narrow to messages in followed topics.'}} + + is:unread diff --git a/web/tests/filter.test.js b/web/tests/filter.test.js index ce80af6418..193f14a0e4 100644 --- a/web/tests/filter.test.js +++ b/web/tests/filter.test.js @@ -361,6 +361,7 @@ test("basics", () => { // filter.supports_collapsing_recipients loop. terms = [ {operator: "is", operand: "resolved", negated: true}, + {operator: "is", operand: "followed", negated: true}, {operator: "is", operand: "dm", negated: true}, {operator: "channel", operand: "channel_name", negated: true}, {operator: "channels", operand: "web-public", negated: true}, @@ -490,6 +491,10 @@ function assert_not_mark_read_with_is_operands(additional_terms_to_test) { is_operator = [{operator: "is", operand: "resolved", negated: true}]; filter = new Filter([...additional_terms_to_test, ...is_operator]); assert.ok(!filter.can_mark_messages_read()); + + is_operator = [{operator: "is", operand: "followed", negated: true}]; + filter = new Filter([...additional_terms_to_test, ...is_operator]); + assert.ok(!filter.can_mark_messages_read()); } function assert_not_mark_read_when_searching(additional_terms_to_test) { @@ -859,6 +864,14 @@ test("predicate_basics", ({override}) => { assert.ok(!predicate({topic: resolved_topic_name})); assert.ok(!predicate({type: stream_message, topic: "foo"})); + predicate = get_predicate([["is", "followed"]]); + + override(user_topics, "is_topic_followed", () => false); + assert.ok(!predicate({type: "stream", topic: "foo", stream_id: 5})); + + override(user_topics, "is_topic_followed", () => true); + assert.ok(predicate({type: "stream", topic: "foo", stream_id: 5})); + const unknown_stream_id = 999; override(user_topics, "is_topic_muted", () => false); override(user_topics, "is_topic_unmuted_or_followed", () => false); @@ -1406,6 +1419,10 @@ test("describe", ({mock_template}) => { string = "topics marked as resolved"; assert.equal(Filter.search_description_as_html(narrow), string); + narrow = [{operator: "is", operand: "followed"}]; + string = "followed topics"; + assert.equal(Filter.search_description_as_html(narrow), string); + narrow = [{operator: "is", operand: "something_we_do_not_support"}]; string = "invalid something_we_do_not_support operand for is operator"; assert.equal(Filter.search_description_as_html(narrow), string); @@ -1739,6 +1756,7 @@ test("navbar_helpers", () => { const is_dm = [{operator: "is", operand: "dm"}]; const is_mentioned = [{operator: "is", operand: "mentioned"}]; const is_resolved = [{operator: "is", operand: "resolved"}]; + const is_followed = [{operator: "is", operand: "followed"}]; const channels_public = [{operator: "channels", operand: "public"}]; const channel_topic_terms = [ {operator: "channel", operand: "foo"}, @@ -1842,6 +1860,15 @@ test("navbar_helpers", () => { title: "translated: Topics marked as resolved", redirect_url_with_search: "/#narrow/topics/is/resolved", }, + { + terms: is_followed, + is_common_narrow: true, + zulip_icon: "follow", + title: "translated: Followed topics", + redirect_url_with_search: "/#narrow/topics/is/followed", + description: "translated: Messages in topics you follow.", + link: "/help/follow-a-topic", + }, { terms: channel_topic_terms, is_common_narrow: true, diff --git a/web/tests/message_view.test.js b/web/tests/message_view.test.js index c8760658f0..640e0e386b 100644 --- a/web/tests/message_view.test.js +++ b/web/tests/message_view.test.js @@ -337,6 +337,13 @@ run_test("show_empty_narrow_message", ({mock_template}) => { empty_narrow_html("translated: No topics are marked as resolved."), ); + set_filter([["is", "followed"]]); + narrow_banner.show_empty_narrow_message(); + assert.equal( + $(".empty_feed_notice_main").html(), + empty_narrow_html("translated: You aren't following any topics."), + ); + // organization has disabled sending direct messages realm.realm_private_message_policy = settings_config.private_message_policy_values.disabled.code; diff --git a/web/tests/search_suggestion.test.js b/web/tests/search_suggestion.test.js index 0779516975..b5e6b94f50 100644 --- a/web/tests/search_suggestion.test.js +++ b/web/tests/search_suggestion.test.js @@ -438,6 +438,7 @@ test("empty_query_suggestions", () => { "is:dm", "is:starred", "is:mentioned", + "is:followed", "is:alerted", "is:unread", "is:resolved", @@ -461,6 +462,7 @@ test("empty_query_suggestions", () => { assert.equal(describe("is:alerted"), "Alerted messages"); assert.equal(describe("is:unread"), "Unread messages"); assert.equal(describe("is:resolved"), "Topics marked as resolved"); + assert.equal(describe("is:followed"), "Followed topics"); assert.equal(describe("sender:myself@zulip.com"), "Sent by me"); assert.equal(describe("has:link"), "Messages that contain links"); assert.equal(describe("has:image"), "Messages that contain images"); @@ -543,6 +545,7 @@ test("check_is_suggestions", ({override, mock_template}) => { "is:dm", "is:starred", "is:mentioned", + "is:followed", "is:alerted", "is:unread", "is:resolved", @@ -563,6 +566,7 @@ test("check_is_suggestions", ({override, mock_template}) => { assert.equal(describe("is:alerted"), "Alerted messages"); assert.equal(describe("is:unread"), "Unread messages"); assert.equal(describe("is:resolved"), "Topics marked as resolved"); + assert.equal(describe("is:followed"), "Followed topics"); query = "-i"; suggestions = get_suggestions(query); @@ -571,6 +575,7 @@ test("check_is_suggestions", ({override, mock_template}) => { "-is:dm", "-is:starred", "-is:mentioned", + "-is:followed", "-is:alerted", "-is:unread", "-is:resolved", @@ -583,12 +588,21 @@ test("check_is_suggestions", ({override, mock_template}) => { assert.equal(describe("-is:alerted"), "Exclude alerted messages"); assert.equal(describe("-is:unread"), "Exclude unread messages"); assert.equal(describe("-is:resolved"), "Exclude topics marked as resolved"); + assert.equal(describe("-is:followed"), "Exclude followed topics"); // operand suggestions follow. query = "is:"; suggestions = get_suggestions(query); - expected = ["is:dm", "is:starred", "is:mentioned", "is:alerted", "is:unread", "is:resolved"]; + expected = [ + "is:dm", + "is:starred", + "is:mentioned", + "is:followed", + "is:alerted", + "is:unread", + "is:resolved", + ]; assert.deepEqual(suggestions.strings, expected); query = "is:st"; diff --git a/zerver/lib/narrow.py b/zerver/lib/narrow.py index 1cc7b868fc..9097f5e36d 100644 --- a/zerver/lib/narrow.py +++ b/zerver/lib/narrow.py @@ -58,6 +58,7 @@ from zerver.lib.streams import ( get_web_public_streams_queryset, ) from zerver.lib.topic_sqlalchemy import ( + get_followed_topic_condition_sa, get_resolved_topic_condition_sa, topic_column_sa, topic_match_sa, @@ -405,6 +406,9 @@ class NarrowBuilder: elif operand == "resolved": cond = get_resolved_topic_condition_sa() return query.where(maybe_negate(cond)) + elif operand == "followed": + cond = get_followed_topic_condition_sa(self.user_profile.id) + return query.where(maybe_negate(cond)) raise BadNarrowOperatorError("unknown 'is' operand " + operand) _alphanum = frozenset("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") @@ -912,7 +916,7 @@ def ok_to_include_history( # that's a property on the UserMessage table. There cannot be # historical messages in these cases anyway. for term in narrow: - if term["operator"] == "is" and term["operand"] not in {"resolved"}: + if term["operator"] == "is" and term["operand"] not in {"resolved", "followed"}: include_history = False return include_history diff --git a/zerver/lib/narrow_predicate.py b/zerver/lib/narrow_predicate.py index 948e793eec..cd527403d2 100644 --- a/zerver/lib/narrow_predicate.py +++ b/zerver/lib/narrow_predicate.py @@ -14,10 +14,15 @@ channels_operators: List[str] = ["channels", "streams"] def check_narrow_for_events(narrow: Collection[NarrowTerm]) -> None: supported_operators = [*channel_operators, "topic", "sender", "is"] + unsupported_is_operands = ["followed"] for narrow_term in narrow: operator = narrow_term.operator if operator not in supported_operators: raise JsonableError(_("Operator {operator} not supported.").format(operator=operator)) + if operator == "is" and narrow_term.operand in unsupported_is_operands: + raise JsonableError( + _("Operand {operand} not supported.").format(operand=narrow_term.operand) + ) class NarrowPredicate(Protocol): diff --git a/zerver/lib/topic_sqlalchemy.py b/zerver/lib/topic_sqlalchemy.py index 6bbab54b89..9dfd9c7e8d 100644 --- a/zerver/lib/topic_sqlalchemy.py +++ b/zerver/lib/topic_sqlalchemy.py @@ -1,7 +1,8 @@ -from sqlalchemy.sql import ColumnElement, column, func, literal +from sqlalchemy.sql import ColumnElement, and_, column, func, literal, literal_column, select, table from sqlalchemy.types import Boolean, Text from zerver.lib.topic import RESOLVED_TOPIC_PREFIX +from zerver.models import UserTopic def topic_match_sa(topic_name: str) -> ColumnElement[Boolean]: @@ -18,3 +19,22 @@ def get_resolved_topic_condition_sa() -> ColumnElement[Boolean]: def topic_column_sa() -> ColumnElement[Text]: return column("subject", Text) + + +def get_followed_topic_condition_sa(user_id: int) -> ColumnElement[Boolean]: + follow_topic_cond = ( + select([1]) + .select_from(table("zerver_usertopic")) + .where( + and_( + literal_column("zerver_usertopic.user_profile_id") == literal(user_id), + literal_column("zerver_usertopic.visibility_policy") + == literal(UserTopic.VisibilityPolicy.FOLLOWED), + func.upper(literal_column("zerver_usertopic.topic_name")) + == func.upper(literal_column("zerver_message.subject")), + literal_column("zerver_usertopic.recipient_id") + == literal_column("zerver_message.recipient_id"), + ) + ) + ).exists() + return follow_topic_cond diff --git a/zerver/tests/test_message_fetch.py b/zerver/tests/test_message_fetch.py index 28ae1c8ae0..cb6c44dc85 100644 --- a/zerver/tests/test_message_fetch.py +++ b/zerver/tests/test_message_fetch.py @@ -270,6 +270,20 @@ class NarrowBuilderTest(ZulipTestCase): term = dict(operator="is", operand="resolved", negated=True) self._do_add_term_test(term, "WHERE (subject NOT LIKE %(subject_1)s || '%%'") + def test_add_term_using_is_operator_for_followed_topics(self) -> None: + term = dict(operator="is", operand="followed", negated=False) + self._do_add_term_test( + term, + "EXISTS (SELECT 1 \nFROM zerver_usertopic \nWHERE zerver_usertopic.user_profile_id = %(param_1)s AND zerver_usertopic.visibility_policy = %(param_2)s AND upper(zerver_usertopic.topic_name) = upper(zerver_message.subject) AND zerver_usertopic.recipient_id = zerver_message.recipient_id)", + ) + + def test_add_term_using_is_operator_for_negated_followed_topics(self) -> None: + term = dict(operator="is", operand="followed", negated=True) + self._do_add_term_test( + term, + "NOT (EXISTS (SELECT 1 \nFROM zerver_usertopic \nWHERE zerver_usertopic.user_profile_id = %(param_1)s AND zerver_usertopic.visibility_policy = %(param_2)s AND upper(zerver_usertopic.topic_name) = upper(zerver_message.subject) AND zerver_usertopic.recipient_id = zerver_message.recipient_id))", + ) + def test_add_term_using_non_supported_operator_should_raise_error(self) -> None: term = dict(operator="is", operand="non_supported") self.assertRaises(BadNarrowOperatorError, self._build_query, term) @@ -951,6 +965,8 @@ class NarrowLibraryTest(ZulipTestCase): def test_build_narrow_predicate_invalid(self) -> None: with self.assertRaises(JsonableError): build_narrow_predicate([NarrowTerm(operator="invalid_operator", operand="operand")]) + with self.assertRaises(JsonableError): + build_narrow_predicate([NarrowTerm(operator="is", operand="followed")]) def test_is_spectator_compatible(self) -> None: self.assertTrue(is_spectator_compatible([])) @@ -4584,6 +4600,37 @@ class MessageHasKeywordsTest(ZulipTestCase): self.assert_length(messages, 1) +class MessageIsTest(ZulipTestCase): + def test_message_is_followed(self) -> None: + self.login("iago") + is_followed_narrow = orjson.dumps([dict(operator="is", operand="followed")]).decode() + + # Sending a message in a topic that isn't followed by the user. + msg_id = self.send_stream_message(self.example_user("hamlet"), "Denmark", topic_name="hey") + result = self.client_get( + "/json/messages", + dict(narrow=is_followed_narrow, anchor=msg_id, num_before=0, num_after=0), + ) + messages = self.assert_json_success(result)["messages"] + self.assert_length(messages, 0) + + stream_id = self.get_stream_id("Denmark", self.example_user("hamlet").realm) + + # Following the topic. + payload = { + "stream_id": stream_id, + "topic": "hey", + "visibility_policy": int(UserTopic.VisibilityPolicy.FOLLOWED), + } + self.client_post("/json/user_topics", payload) + result = self.client_get( + "/json/messages", + dict(narrow=is_followed_narrow, anchor=msg_id, num_before=0, num_after=0), + ) + messages = self.assert_json_success(result)["messages"] + self.assert_length(messages, 1) + + class MessageVisibilityTest(ZulipTestCase): def test_update_first_visible_message_id(self) -> None: Message.objects.all().delete()