From ece752014c5c9cc1d41bacecc1ddd692cea9e709 Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Mon, 3 Apr 2023 13:58:08 +0200 Subject: [PATCH] narrow: Add backend support for `is:dm` narrow. Adds backend support for `is` operator with the `dm` operand. This will deprecate the `is` operator with the `private` operand, but we keep support for backwards-compatibility. Note that there is some clean up of references to private messages in the updated backend test. In commit 43ec7ed, the documentation for `build_narrow_filter` wasn't updated for the rename of `BuildNarrowFilterTest` to `NarrowLibraryTest`, so that's also corrected in these changes. The general API changelog and documentation updates will be done in a final commit in the series of commits that adds support for the various new direct message narrows. --- zerver/lib/narrow.py | 8 +++-- zerver/tests/fixtures/narrow.json | 24 +++++++++++++ zerver/tests/test_message_fetch.py | 57 +++++++++++++++++++++--------- 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/zerver/lib/narrow.py b/zerver/lib/narrow.py index 2edf9ca854..5209b68249 100644 --- a/zerver/lib/narrow.py +++ b/zerver/lib/narrow.py @@ -133,7 +133,7 @@ def is_web_public_narrow(narrow: Optional[Iterable[Dict[str, Any]]]) -> bool: def build_narrow_filter(narrow: Collection[Sequence[str]]) -> Callable[[Mapping[str, Any]], bool]: """Changes to this function should come with corresponding changes to - BuildNarrowFilterTest.""" + NarrowLibraryTest.""" check_supported_events_narrow_filter(narrow) def narrow_filter(event: Mapping[str, Any]) -> bool: @@ -156,7 +156,8 @@ def build_narrow_filter(narrow: Collection[Sequence[str]]) -> Callable[[Mapping[ elif operator == "sender": if operand.lower() != message["sender_email"].lower(): return False - elif operator == "is" and operand == "private": + elif operator == "is" and operand in ["dm", "private"]: + # "is:private" is a legacy alias for "is:dm" if message["type"] != "private": return False elif operator == "is" and operand in ["starred"]: @@ -336,7 +337,8 @@ class NarrowBuilder: assert not self.is_web_public_query assert self.user_profile is not None - if operand == "private": + if operand in ["dm", "private"]: + # "is:private" is a legacy alias for "is:dm" cond = column("flags", Integer).op("&")(UserMessage.flags.is_private.mask) != 0 return query.where(maybe_negate(cond)) elif operand == "starred": diff --git a/zerver/tests/fixtures/narrow.json b/zerver/tests/fixtures/narrow.json index 926476b6c1..e2b1e21ed0 100644 --- a/zerver/tests/fixtures/narrow.json +++ b/zerver/tests/fixtures/narrow.json @@ -146,6 +146,30 @@ ] ] }, + { + "accept_events":[ + { + "message":{ + "type":"private" + }, + "flags":null + } + ], + "reject_events":[ + { + "message":{ + "type":"stream" + }, + "flags":null + } + ], + "narrow":[ + [ + "is", + "dm" + ] + ] + }, { "accept_events":[ { diff --git a/zerver/tests/test_message_fetch.py b/zerver/tests/test_message_fetch.py index 7e177b22f3..e9000eb22a 100644 --- a/zerver/tests/test_message_fetch.py +++ b/zerver/tests/test_message_fetch.py @@ -127,10 +127,6 @@ class NarrowBuilderTest(ZulipTestCase): term = dict(operator="stream", operand="NonExistingStream") self.assertRaises(BadNarrowOperatorError, self._build_query, term) - def test_add_term_using_is_operator_and_private_operand(self) -> None: - term = dict(operator="is", operand="private") - self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) != %(param_1)s") - def test_add_term_using_streams_operator_and_invalid_operand_should_raise_error( self, ) -> None: # NEGATED @@ -209,11 +205,15 @@ class NarrowBuilderTest(ZulipTestCase): "WHERE (recipient_id NOT IN (__[POSTCOMPILE_recipient_id_1]))", ) - def test_add_term_using_is_operator_private_operand_and_negated(self) -> None: # NEGATED - term = dict(operator="is", operand="private", negated=True) + def test_add_term_using_is_operator_and_dm_operand(self) -> None: + term = dict(operator="is", operand="dm") + self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) != %(param_1)s") + + def test_add_term_using_is_operator_dm_operand_and_negated(self) -> None: # NEGATED + term = dict(operator="is", operand="dm", negated=True) self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) = %(param_1)s") - def test_add_term_using_is_operator_and_non_private_operand(self) -> None: + def test_add_term_using_is_operator_and_non_dm_operand(self) -> None: for operand in ["starred", "mentioned", "alerted"]: term = dict(operator="is", operand=operand) self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) != %(param_1)s") @@ -226,7 +226,7 @@ class NarrowBuilderTest(ZulipTestCase): term = dict(operator="is", operand="unread", negated=True) self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) != %(param_1)s") - def test_add_term_using_is_operator_non_private_operand_and_negated(self) -> None: # NEGATED + def test_add_term_using_is_operator_non_dm_operand_and_negated(self) -> None: # NEGATED term = dict(operator="is", operand="starred", negated=True) where_clause = "WHERE (flags & %(flags_1)s) = %(param_1)s" params = dict( @@ -522,6 +522,15 @@ class NarrowBuilderTest(ZulipTestCase): self.assertRaises(BadNarrowOperatorError, _build_query, term) + # Test "is:private" (legacy alias for "is:dm") + def test_add_term_using_is_operator_and_private_operand(self) -> None: + term = dict(operator="is", operand="private") + self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) != %(param_1)s") + + def test_add_term_using_is_operator_private_operand_and_negated(self) -> None: # NEGATED + term = dict(operator="is", operand="private", negated=True) + self._do_add_term_test(term, "WHERE (flags & %(flags_1)s) = %(param_1)s") + # Test that the underscore version of "pm-with" works. def test_add_term_using_underscore_version_of_pm_with_operator(self) -> None: term = dict(operator="pm_with", operand=self.hamlet_email) @@ -552,7 +561,7 @@ class NarrowLibraryTest(ZulipTestCase): fixtures_path = os.path.join(os.path.dirname(__file__), "fixtures/narrow.json") with open(fixtures_path, "rb") as f: scenarios = orjson.loads(f.read()) - self.assert_length(scenarios, 10) + self.assert_length(scenarios, 11) for scenario in scenarios: narrow = scenario["narrow"] accept_events = scenario["accept_events"] @@ -597,11 +606,15 @@ class NarrowLibraryTest(ZulipTestCase): ) ) self.assertFalse(is_spectator_compatible([{"operator": "is", "operand": "starred"}])) - self.assertFalse(is_spectator_compatible([{"operator": "is", "operand": "private"}])) + self.assertFalse(is_spectator_compatible([{"operator": "is", "operand": "dm"}])) self.assertTrue(is_spectator_compatible([{"operator": "streams", "operand": "public"}])) + # Malformed input not allowed self.assertFalse(is_spectator_compatible([{"operator": "has"}])) + # "is:private" is a legacy alias for "is:dm". + self.assertFalse(is_spectator_compatible([{"operator": "is", "operand": "private"}])) + class IncludeHistoryTest(ZulipTestCase): def test_ok_to_include_history(self) -> None: @@ -651,7 +664,12 @@ class IncludeHistoryTest(ZulipTestCase): self.assertFalse(ok_to_include_history(narrow, user_profile, False)) self.assertTrue(ok_to_include_history(narrow, subscribed_user_profile, False)) - # History doesn't apply to PMs. + # History doesn't apply to direct messages. + narrow = [ + dict(operator="is", operand="dm"), + ] + self.assertFalse(ok_to_include_history(narrow, user_profile, False)) + # "is:private" is a legacy alias for "is:dm". narrow = [ dict(operator="is", operand="private"), ] @@ -1539,7 +1557,14 @@ class GetOldMessagesTest(ZulipTestCase): result = self.client_get("/json/messages", dict(web_public_stream_get_params)) self.assert_json_error(result, "Invalid subdomain", status_code=404) - # Cannot access private messages without login. + # Cannot access direct messages without login. + direct_messages_get_params: Dict[str, Union[int, str, bool]] = { + **get_params, + "narrow": orjson.dumps([dict(operator="is", operand="dm")]).decode(), + } + result = self.client_get("/json/messages", dict(direct_messages_get_params)) + self.check_unauthenticated_response(result) + # "is:private" is a legacy alias for "is:dm". private_message_get_params: Dict[str, Union[int, str, bool]] = { **get_params, "narrow": orjson.dumps([dict(operator="is", operand="private")]).decode(), @@ -1550,11 +1575,11 @@ class GetOldMessagesTest(ZulipTestCase): # narrow should pass conditions in `is_spectator_compatible`. non_spectator_compatible_narrow_get_params: Dict[str, Union[int, str, bool]] = { **get_params, - # "is:private" is not a is_spectator_compatible narrow. + # "is:dm" is not a is_spectator_compatible narrow. "narrow": orjson.dumps( [ dict(operator="streams", operand="web-public"), - dict(operator="is", operand="private"), + dict(operator="is", operand="dm"), ] ).decode(), } @@ -1613,7 +1638,7 @@ class GetOldMessagesTest(ZulipTestCase): def setup_web_public_test(self, num_web_public_message: int = 1) -> None: """ Send N+2 messages, N in a web-public stream, then one in a non-web-public stream - and then a private message. + and then a direct message. """ user_profile = self.example_user("iago") do_set_realm_property( @@ -1633,7 +1658,7 @@ class GetOldMessagesTest(ZulipTestCase): user_profile, non_web_public_stream.name, content="non-web-public message" ) self.send_personal_message( - user_profile, self.example_user("hamlet"), content="private message" + user_profile, self.example_user("hamlet"), content="direct message" ) self.logout()