From c349d1137ce4bb4e4c82b0843a88b1842b3c999b Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Sat, 30 Sep 2023 16:10:39 +0530 Subject: [PATCH] sent_by_human: Add "test suite" to the set of Zulip UI-style clients. Earlier, when we used 'self.send_message()' in the backend tests, the sent message was not marked as read for the sender. Reason: To set the read flag, we have to check if 'message.sent_by_human()'. It returns False because the 'sending_client' for tests is "test suite" and the 'sent_by_human' function doesn't enlist the "test suite" client name as a human client. This commit adds "test suite" to that list. Also fixes a bug in when apply_unread_message_event was called that was revealed by this change. --- zerver/lib/events.py | 2 +- zerver/models.py | 32 ++++++++++-------- zerver/tests/test_events.py | 7 ++-- zerver/tests/test_home.py | 8 ++--- zerver/tests/test_message_edit.py | 46 ++++++++++++++++---------- zerver/tests/test_message_flags.py | 53 ++++++++++++++++-------------- zerver/tests/test_signup.py | 2 +- 7 files changed, 87 insertions(+), 63 deletions(-) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 1308143a6c..042ff316d1 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -718,7 +718,7 @@ def apply_event( ) -> None: if event["type"] == "message": state["max_message_id"] = max(state["max_message_id"], event["message"]["id"]) - if "raw_unread_msgs" in state: + if "raw_unread_msgs" in state and "read" not in event["flags"]: apply_unread_message_event( user_profile, state["raw_unread_msgs"], diff --git a/zerver/models.py b/zerver/models.py index e32fddd4d3..d4aa25ac1e 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -3111,20 +3111,26 @@ class Message(AbstractMessage): sending_client = self.sending_client.name.lower() return ( - sending_client - in ( - "zulipandroid", - "zulipios", - "zulipdesktop", - "zulipmobile", - "zulipelectron", - "zulipterminal", - "snipe", - "website", - "ios", - "android", + ( + sending_client + in ( + "zulipandroid", + "zulipios", + "zulipdesktop", + "zulipmobile", + "zulipelectron", + "zulipterminal", + "snipe", + "website", + "ios", + "android", + ) ) - ) or ("desktop app" in sending_client) + or ("desktop app" in sending_client) + # Since the vast majority of messages are sent by humans + # in Zulip, treat test suite messages as such. + or (sending_client == "test suite" and settings.TEST_SUITE) + ) @staticmethod def is_status_message(content: str, rendered_content: str) -> bool: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 7097dd3dd5..6ba310c25a 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -577,7 +577,8 @@ class NormalActionsTest(BaseAction): ) def test_stream_update_message_events(self) -> None: - self.send_stream_message(self.example_user("hamlet"), "Verona", "hello") + iago = self.example_user("iago") + self.send_stream_message(iago, "Verona", "hello") # Verify stream message editing - content only message = Message.objects.order_by("-id")[0] @@ -666,7 +667,7 @@ class NormalActionsTest(BaseAction): # Verify move topic to different stream. self.subscribe(self.user_profile, "Verona") self.subscribe(self.user_profile, "Denmark") - self.send_stream_message(self.user_profile, "Verona") + self.send_stream_message(iago, "Verona") message_id = self.send_stream_message(self.user_profile, "Verona") message = Message.objects.get(id=message_id) stream = get_stream("Denmark", self.user_profile.realm) @@ -788,7 +789,7 @@ class NormalActionsTest(BaseAction): check_update_message_flags_remove("events[0]", events[0]) personal_message = self.send_personal_message( - from_user=user_profile, to_user=self.example_user("cordelia"), content=content + from_user=self.example_user("cordelia"), to_user=user_profile, content=content ) self.verify_action( partial(do_update_message_flags, user_profile, "add", "read", [personal_message]), diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 04eed71866..4f90ba55f3 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -1124,7 +1124,7 @@ class HomeTest(ZulipTestCase): self.send_test_message(message) self.login_user(long_term_idle_user) with queries_captured() as queries: - self.assertEqual(self.soft_activate_and_get_unread_count(), 2) + self.assertEqual(self.soft_activate_and_get_unread_count(), 1) query_count = len(queries) long_term_idle_user.refresh_from_db() self.assertFalse(long_term_idle_user.long_term_idle) @@ -1134,7 +1134,7 @@ class HomeTest(ZulipTestCase): message = "Test message 2" self.send_test_message(message) with queries_captured() as queries: - self.assertEqual(self.soft_activate_and_get_unread_count(), 3) + self.assertEqual(self.soft_activate_and_get_unread_count(), 2) # Test here for query count to be at least 5 less than previous count. # This will assure add_missing_messages() isn't repeatedly called. self.assertGreaterEqual(query_count - len(queries), 5) @@ -1156,7 +1156,7 @@ class HomeTest(ZulipTestCase): self.send_test_message(message) self.login_user(long_term_idle_user) with queries_captured() as queries: - self.assertEqual(self.soft_activate_and_get_unread_count(), 4) + self.assertEqual(self.soft_activate_and_get_unread_count(), 3) query_count = len(queries) long_term_idle_user.refresh_from_db() self.assertFalse(long_term_idle_user.long_term_idle) @@ -1166,7 +1166,7 @@ class HomeTest(ZulipTestCase): message = "Test message 4" self.send_test_message(message) with queries_captured() as queries: - self.assertEqual(self.soft_activate_and_get_unread_count(), 5) + self.assertEqual(self.soft_activate_and_get_unread_count(), 4) self.assertGreaterEqual(query_count - len(queries), 5) idle_user_msg_list = get_user_messages(long_term_idle_user) self.assertEqual(idle_user_msg_list[-1].content, message) diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 15b00f8152..315f75350a 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -335,7 +335,7 @@ class EditMessageTest(EditMessageTestCase): response_dict = self.assert_json_success(result) self.assertEqual(response_dict["raw_content"], "Personal message") self.assertEqual(response_dict["message"]["id"], msg_id) - self.assertEqual(response_dict["message"]["flags"], []) + self.assertEqual(response_dict["message"]["flags"], ["read"]) # Send message to web-public stream where hamlet is not subscribed. # This will test case of user having no `UserMessage` but having access @@ -2127,7 +2127,7 @@ class EditMessageTest(EditMessageTestCase): [ { "id": hamlet.id, - "flags": ["wildcard_mentioned"], + "flags": ["read", "wildcard_mentioned"], }, { "id": cordelia.id, @@ -2181,13 +2181,19 @@ class EditMessageTest(EditMessageTestCase): ) message_id = self.send_stream_message(hamlet, stream_name, "Hello everyone") - def notify(user_id: int) -> Dict[str, Any]: - return { - "id": user_id, - "flags": ["wildcard_mentioned"], - } - - users_to_be_notified = sorted(map(notify, [cordelia.id, hamlet.id]), key=itemgetter("id")) + users_to_be_notified = sorted( + [ + { + "id": hamlet.id, + "flags": ["read", "wildcard_mentioned"], + }, + { + "id": cordelia.id, + "flags": ["wildcard_mentioned"], + }, + ], + key=itemgetter("id"), + ) result = self.client_patch( f"/json/messages/{message_id}", { @@ -2228,7 +2234,7 @@ class EditMessageTest(EditMessageTestCase): [ { "id": hamlet.id, - "flags": ["wildcard_mentioned"], + "flags": ["read", "wildcard_mentioned"], }, { "id": cordelia.id, @@ -2319,13 +2325,19 @@ class EditMessageTest(EditMessageTestCase): self.login_user(hamlet) message_id = self.send_stream_message(hamlet, stream_name, "Hello everyone") - def notify(user_id: int) -> Dict[str, Any]: - return { - "id": user_id, - "flags": ["wildcard_mentioned"], - } - - users_to_be_notified = sorted(map(notify, [cordelia.id, hamlet.id]), key=itemgetter("id")) + users_to_be_notified = sorted( + [ + { + "id": hamlet.id, + "flags": ["read", "wildcard_mentioned"], + }, + { + "id": cordelia.id, + "flags": ["wildcard_mentioned"], + }, + ], + key=itemgetter("id"), + ) result = self.client_patch( f"/json/messages/{message_id}", { diff --git a/zerver/tests/test_message_flags.py b/zerver/tests/test_message_flags.py index c915748eda..3662266f64 100644 --- a/zerver/tests/test_message_flags.py +++ b/zerver/tests/test_message_flags.py @@ -182,6 +182,7 @@ class UnreadCountTests(ZulipTestCase): mock_push_notifications_enabled.assert_called() # Sending a new message results in unread UserMessages being created + # for users other than sender. def test_new_message(self) -> None: self.login("hamlet") content = "Test message for unset read bit" @@ -190,8 +191,10 @@ class UnreadCountTests(ZulipTestCase): self.assertGreater(len(user_messages), 0) for um in user_messages: self.assertEqual(um.message.content, content) - if um.user_profile.email != self.example_email("hamlet"): + if um.user_profile.delivery_email != self.example_email("hamlet"): self.assertFalse(um.flags.read) + else: + self.assertTrue(um.flags.read) def test_update_flags(self) -> None: self.login("hamlet") @@ -315,14 +318,16 @@ class UnreadCountTests(ZulipTestCase): def test_mark_all_in_stream_read(self) -> None: self.login("hamlet") - user_profile = self.example_user("hamlet") - stream = self.subscribe(user_profile, "test_stream") - self.subscribe(self.example_user("cordelia"), "test_stream") + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + iago = self.example_user("iago") + for user in [hamlet, cordelia, iago]: + self.subscribe(user, "test_stream") + self.subscribe(user, "Denmark") + stream = get_stream("test_stream", hamlet.realm) - message_id = self.send_stream_message(self.example_user("hamlet"), "test_stream", "hello") - unrelated_message_id = self.send_stream_message( - self.example_user("hamlet"), "Denmark", "hello" - ) + message_id = self.send_stream_message(cordelia, "test_stream", "hello") + unrelated_message_id = self.send_stream_message(cordelia, "Denmark", "hello") with self.capture_send_event_calls(expected_num_events=1) as events: result = self.client_post( @@ -346,17 +351,18 @@ class UnreadCountTests(ZulipTestCase): differences = [key for key in expected if expected[key] != event[key]] self.assert_length(differences, 0) - hamlet = self.example_user("hamlet") + # cordelia sent the message and hamlet marked it as read. um = list(UserMessage.objects.filter(message=message_id)) for msg in um: - if msg.user_profile.email == hamlet.email: + if msg.user_profile.email in [hamlet.email, cordelia.email]: self.assertTrue(msg.flags.read) else: self.assertFalse(msg.flags.read) + # cordelia sent the message, so marked as read only for him. unrelated_messages = list(UserMessage.objects.filter(message=unrelated_message_id)) for msg in unrelated_messages: - if msg.user_profile.email == hamlet.email: + if msg.user_profile.email in [hamlet.email, iago.email]: self.assertFalse(msg.flags.read) def test_mark_all_in_invalid_stream_read(self) -> None: @@ -384,20 +390,19 @@ class UnreadCountTests(ZulipTestCase): def test_mark_all_in_stream_topic_read(self) -> None: self.login("hamlet") - user_profile = self.example_user("hamlet") - self.subscribe(user_profile, "test_stream") + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + for user in [hamlet, cordelia]: + self.subscribe(user, "test_stream") + self.subscribe(user, "Denmark") - message_id = self.send_stream_message( - self.example_user("hamlet"), "test_stream", "hello", "test_topic" - ) - unrelated_message_id = self.send_stream_message( - self.example_user("hamlet"), "Denmark", "hello", "Denmark2" - ) + message_id = self.send_stream_message(cordelia, "test_stream", "hello", "test_topic") + unrelated_message_id = self.send_stream_message(cordelia, "Denmark", "hello", "Denmark2") with self.capture_send_event_calls(expected_num_events=1) as events: result = self.client_post( "/json/mark_topic_as_read", { - "stream_id": get_stream("test_stream", user_profile.realm).id, + "stream_id": get_stream("test_stream", hamlet.realm).id, "topic_name": "test_topic", }, ) @@ -418,12 +423,12 @@ class UnreadCountTests(ZulipTestCase): um = list(UserMessage.objects.filter(message=message_id)) for msg in um: - if msg.user_profile_id == user_profile.id: + if msg.user_profile_id == hamlet.id: self.assertTrue(msg.flags.read) unrelated_messages = list(UserMessage.objects.filter(message=unrelated_message_id)) for msg in unrelated_messages: - if msg.user_profile_id == user_profile.id: + if msg.user_profile_id == hamlet.id: self.assertFalse(msg.flags.read) def test_mark_all_in_invalid_topic_read(self) -> None: @@ -1221,7 +1226,7 @@ class MessageAccessTests(ZulipTestCase): for msg in self.get_messages(): if msg["id"] in message_ids: - check_flags(msg["flags"], {"starred"}) + check_flags(msg["flags"], {"read", "starred"}) else: check_flags(msg["flags"], {"read"}) @@ -1231,7 +1236,7 @@ class MessageAccessTests(ZulipTestCase): for msg in self.get_messages(): if msg["id"] in message_ids: - check_flags(msg["flags"], set()) + check_flags(msg["flags"], {"read"}) def test_change_collapsed_public_stream_historical(self) -> None: hamlet = self.example_user("hamlet") diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 8545c5ac3e..02bb1b6480 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -336,7 +336,7 @@ class AddNewUserHistoryTest(ZulipTestCase): user_message = most_recent_usermessage(user_profile) self.assertEqual( repr(user_message), - f"", + f"", )