From ca74cd6e37001f7fab286da04b7cbfece56ef372 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Tue, 17 Mar 2020 22:17:12 +0000 Subject: [PATCH] bug fix: Fix unread counts for certain API messages. If I send a message from a normal Zulip client, it is considered to be "read" by me. But if I send it via an API program (using my human account), the message is not immediately "read" by me. Now we handle this correctly in `get_raw_unread_data`. The symptom of this was that these messages would get "stuck" in "Private Messages" narrows until the next time you reloaded your app. --- zerver/lib/message.py | 24 ++++++-- zerver/tests/test_events.py | 107 ++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 4 deletions(-) diff --git a/zerver/lib/message.py b/zerver/lib/message.py index 2e4ec344ed..ad138c23a7 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -853,8 +853,19 @@ def get_raw_unread_data(user_profile: UserProfile) -> RawUnreadMessagesResult: unmuted_stream_msgs.add(message_id) elif msg_type == Recipient.PERSONAL: + if sender_id == user_profile.id: + other_user_id = row['message__recipient__type_id'] + else: + other_user_id = sender_id + + # The `sender_id` field here is misnamed. It's really + # just the other participant in a PM conversation. For + # most unread PM messages, the other user is also the sender, + # but that's not true for certain messages sent from the + # API. Unfortunately, it's difficult now to rename the + # field without breaking mobile. pm_dict[message_id] = dict( - sender_id=sender_id, + sender_id=other_user_id, ) elif msg_type == Recipient.HUDDLE: @@ -940,7 +951,7 @@ def apply_unread_message_event(user_profile: UserProfile, elif message['type'] == 'private': others = [ recip for recip in message['display_recipient'] - if recip['id'] != message['sender_id'] + if recip['id'] != user_profile.id ] if len(others) <= 1: message_type = 'private' @@ -967,9 +978,14 @@ def apply_unread_message_event(user_profile: UserProfile, state['unmuted_stream_msgs'].add(message_id) elif message_type == 'private': - sender_id = message['sender_id'] + if len(others) == 1: + other_id = others[0]['id'] + else: + other_id = user_profile.id + + # The `sender_id` field here is misnamed. new_row = dict( - sender_id=sender_id, + sender_id=other_id, ) state['pm_dict'][message_id] = new_row diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 245bfc5957..59584036fc 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -112,8 +112,10 @@ from zerver.lib.events import ( ) from zerver.lib.message import ( aggregate_unread_data, + apply_unread_message_event, get_raw_unread_data, render_markdown, + MessageDict, UnreadMessagesResult, ) from zerver.lib.test_helpers import POSTRequestMock, get_subscription, \ @@ -3065,6 +3067,111 @@ class GetUnreadMsgsTest(ZulipTestCase): dict(sender_id=cordelia.id), ) + def test_raw_unread_personal_from_self(self) -> None: + hamlet = self.example_user('hamlet') + + def send_unread_pm(other_user: UserProfile) -> Message: + # It is rare to send a message from Hamlet to Othello + # (or any other user) and have it be unread for + # Hamlet himself, but that is actually normal + # behavior for most API clients. + message_id = self.send_personal_message( + from_user=hamlet, + to_user=other_user, + sending_client_name='some_api_program', + ) + + # Check our test setup is correct--the message should + # not have looked like it was sent by a human. + message = Message.objects.get(id=message_id) + self.assertFalse(message.sent_by_human()) + + # And since it was not sent by a human, it should not + # be read, not even by the sender (Hamlet). + um = UserMessage.objects.get( + user_profile_id=hamlet.id, + message_id=message_id, + ) + self.assertFalse(um.flags.read) + + return message + + othello = self.example_user('othello') + othello_msg = send_unread_pm(other_user=othello) + + # And now check the unread data structure... + raw_unread_data = get_raw_unread_data( + user_profile=hamlet, + ) + + pm_dict = raw_unread_data['pm_dict'] + + self.assertEqual(set(pm_dict.keys()), {othello_msg.id}) + + # For legacy reason we call the field `sender_id` here, + # but it really refers to the other user id in the conversation, + # which is Othello. + self.assertEqual( + pm_dict[othello_msg.id], + dict(sender_id=othello.id), + ) + + cordelia = self.example_user('cordelia') + cordelia_msg = send_unread_pm(other_user=cordelia) + + apply_unread_message_event( + user_profile=hamlet, + state=raw_unread_data, + message=MessageDict.wide_dict(cordelia_msg), + flags=[], + ) + self.assertEqual( + set(pm_dict.keys()), + {othello_msg.id, cordelia_msg.id} + ) + + # Again, `sender_id` is misnamed here. + self.assertEqual( + pm_dict[cordelia_msg.id], + dict(sender_id=cordelia.id), + ) + + # Send a message to ourself. + hamlet_msg = send_unread_pm(other_user=hamlet) + apply_unread_message_event( + user_profile=hamlet, + state=raw_unread_data, + message=MessageDict.wide_dict(hamlet_msg), + flags=[], + ) + self.assertEqual( + set(pm_dict.keys()), + {othello_msg.id, cordelia_msg.id, hamlet_msg.id} + ) + + # Again, `sender_id` is misnamed here. + self.assertEqual( + pm_dict[hamlet_msg.id], + dict(sender_id=hamlet.id), + ) + + # Call get_raw_unread_data again. + raw_unread_data = get_raw_unread_data( + user_profile=hamlet, + ) + pm_dict = raw_unread_data['pm_dict'] + + self.assertEqual( + set(pm_dict.keys()), + {othello_msg.id, cordelia_msg.id, hamlet_msg.id} + ) + + # Again, `sender_id` is misnamed here. + self.assertEqual( + pm_dict[hamlet_msg.id], + dict(sender_id=hamlet.id), + ) + def test_unread_msgs(self) -> None: sender = self.example_user('cordelia') sender_id = sender.id