From 72abd4f12d6d8aecab7b7cb8ed9089c13ad51869 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Fri, 12 May 2017 13:47:34 -0700 Subject: [PATCH] mentions: Fix subject line and sender for missed-message mentions. This fixes 2 issues: * The term "@-mentioned" is simplified to "mentioned". * We would incorrectly list other people who sent context messages as among the people who mentioned you. --- zerver/lib/notifications.py | 26 ++++++++++++++++---------- zerver/tests/test_notifications.py | 28 +++++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/zerver/lib/notifications.py b/zerver/lib/notifications.py index 9157eb819c..b59676ba65 100644 --- a/zerver/lib/notifications.py +++ b/zerver/lib/notifications.py @@ -274,14 +274,7 @@ def do_send_missedmessage_events_reply_in_zulip(user_profile, missed_messages, m from zerver.lib.email_mirror import create_missed_message_address address = create_missed_message_address(user_profile, missed_messages[0]) - # TODO: For the mention case, senders should just be the people - # who mentioned you, not the other senders. - senders = set(m.sender.full_name for m in missed_messages) - sender_str = ", ".join(senders) - context.update({ - 'sender_str': sender_str, - 'realm_str': user_profile.realm.name, - }) + senders = list(set(m.sender for m in missed_messages)) if (missed_messages[0].recipient.type == Recipient.HUDDLE): display_recipient = get_display_recipient(missed_messages[0].recipient) # Make sure that this is a list of strings, not a string. @@ -301,7 +294,20 @@ def do_send_missedmessage_events_reply_in_zulip(user_profile, missed_messages, m elif (missed_messages[0].recipient.type == Recipient.PERSONAL): context.update({'private_message': True}) else: + # Keep only the senders who actually mentioned the user + # + # TODO: When we add wildcard mentions that send emails, add + # them to the filter here. + senders = list(set(m.sender for m in missed_messages if + UserMessage.objects.filter(message=m, user_profile=user_profile, + flags=UserMessage.flags.mentioned).exists())) context.update({'at_mention': True}) + + context.update({ + 'sender_str': ", ".join(sender.full_name for sender in senders), + 'realm_str': user_profile.realm.name, + }) + from_email = None if len(senders) == 1 and settings.SEND_MISSED_MESSAGE_EMAILS_AS_USER: @@ -310,8 +316,8 @@ def do_send_missedmessage_events_reply_in_zulip(user_profile, missed_messages, m # However, one must ensure the Zulip server is in the SPF # record for the domain, or there will be spam/deliverability # problems. - sender = missed_messages[0].sender - from_email = '"%s" <%s>' % (sender_str, sender.email) + sender = senders[0] + from_email = '"%s" <%s>' % (sender.full_name, sender.email) context.update({ 'reply_warning': False, 'reply_to_zulip': False, diff --git a/zerver/tests/test_notifications.py b/zerver/tests/test_notifications.py index cd09669934..afa49d3186 100644 --- a/zerver/tests/test_notifications.py +++ b/zerver/tests/test_notifications.py @@ -55,7 +55,7 @@ class TestMissedMessages(ZulipTestCase): self.assertIn(body, self.normalize_string(msg.body)) @patch('zerver.lib.email_mirror.generate_random_token') - def _extra_context_in_missed_stream_messages(self, send_as_user, mock_random_token): + def _extra_context_in_missed_stream_messages_mention(self, send_as_user, mock_random_token): # type: (bool, MagicMock) -> None tokens = self._get_tokens() mock_random_token.side_effect = tokens @@ -68,6 +68,19 @@ class TestMissedMessages(ZulipTestCase): subject = 'Othello, the Moor of Venice mentioned you in Zulip Dev' self._test_cases(tokens, msg_id, body, subject, send_as_user) + @patch('zerver.lib.email_mirror.generate_random_token') + def _extra_context_in_missed_stream_messages_mention_two_senders(self, send_as_user, mock_random_token): + # type: (bool, MagicMock) -> None + tokens = self._get_tokens() + mock_random_token.side_effect = tokens + + for i in range(0, 3): + self.send_message("cordelia@zulip.com", "Denmark", Recipient.STREAM, str(i)) + msg_id = self.send_message("othello@zulip.com", "Denmark", Recipient.STREAM, '@**hamlet**') + body = 'Denmark > test Cordelia Lear 0 1 2 Othello, the Moor of Venice @**hamlet**' + subject = 'Othello, the Moor of Venice mentioned you in Zulip Dev' + self._test_cases(tokens, msg_id, body, subject, send_as_user) + @patch('zerver.lib.email_mirror.generate_random_token') def _extra_context_in_personal_missed_stream_messages(self, send_as_user, mock_random_token): # type: (bool, MagicMock) -> None @@ -212,11 +225,20 @@ class TestMissedMessages(ZulipTestCase): @override_settings(SEND_MISSED_MESSAGE_EMAILS_AS_USER=True) def test_extra_context_in_missed_stream_messages_as_user(self): # type: () -> None - self._extra_context_in_missed_stream_messages(True) + self._extra_context_in_missed_stream_messages_mention(True) def test_extra_context_in_missed_stream_messages(self): # type: () -> None - self._extra_context_in_missed_stream_messages(False) + self._extra_context_in_missed_stream_messages_mention(False) + + @override_settings(SEND_MISSED_MESSAGE_EMAILS_AS_USER=True) + def test_extra_context_in_missed_stream_messages_as_user_two_senders(self): + # type: () -> None + self._extra_context_in_missed_stream_messages_mention_two_senders(True) + + def test_extra_context_in_missed_stream_messages_two_senders(self): + # type: () -> None + self._extra_context_in_missed_stream_messages_mention_two_senders(False) def test_reply_to_email_in_personal_missed_stream_messages(self): # type: () -> None