From 0facaa07976de8c7619d49dad762c5b41fc94426 Mon Sep 17 00:00:00 2001 From: James Rowan Date: Wed, 3 May 2017 03:22:58 -0400 Subject: [PATCH] Changes sender and subject lines for missed message emails. Now, in the event of messages between two other members of a huddle, the missed message emails are threaded in "Group PMs with name1 and name2" and not in separate threads by sender. Also, now the order of recipients in get_display_recipient consistent with the order of names that appears in the list of personal messages on the left sidebar. Fixes most of #4553. --- .../zerver/emails/missed_message.subject | 5 +- zerver/lib/notifications.py | 25 +++++- zerver/models.py | 2 +- zerver/tests/test_notifications.py | 82 ++++++++++++++++--- 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/templates/zerver/emails/missed_message.subject b/templates/zerver/emails/missed_message.subject index fe64786598..b3e2e9042c 100644 --- a/templates/zerver/emails/missed_message.subject +++ b/templates/zerver/emails/missed_message.subject @@ -1 +1,4 @@ -Missed Zulips from {{ sender_str }} +{% if group_pm %} Group PMs with {{ huddle_display_name }} in {{ realm_str }} +{% elif at_mention %} {{ sender_str }} mentioned you in {{ realm_str }} +{% elif private_message %} {{ sender_str }} sent you a message in {{ realm_str }} +{% endif %} diff --git a/zerver/lib/notifications.py b/zerver/lib/notifications.py index 66f2f9dd56..9157eb819c 100644 --- a/zerver/lib/notifications.py +++ b/zerver/lib/notifications.py @@ -274,13 +274,36 @@ 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, }) - + 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. + assert not isinstance(display_recipient, Text) + other_recipients = [r['full_name'] for r in display_recipient + if r['id'] != user_profile.id] + context.update({'group_pm': True}) + if len(other_recipients) == 2: + huddle_display_name = u"%s" % (" and ".join(other_recipients)) + context.update({'huddle_display_name': huddle_display_name}) + elif len(other_recipients) == 3: + huddle_display_name = u"%s, %s, and %s" % (other_recipients[0], other_recipients[1], other_recipients[2]) + context.update({'huddle_display_name': huddle_display_name}) + else: + huddle_display_name = u"%s, and %s others" % (', '.join(other_recipients[:2]), len(other_recipients) - 2) + context.update({'huddle_display_name': huddle_display_name}) + elif (missed_messages[0].recipient.type == Recipient.PERSONAL): + context.update({'private_message': True}) + else: + context.update({'at_mention': True}) from_email = None + if len(senders) == 1 and settings.SEND_MISSED_MESSAGE_EMAILS_AS_USER: # If this setting is enabled, you can reply to the Zulip # missed message emails directly back to the original sender. diff --git a/zerver/models.py b/zerver/models.py index 83df122b97..4f579f527e 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -93,7 +93,7 @@ def get_display_recipient_remote_cache(recipient_id, recipient_type, recipient_t # We don't really care what the ordering is, just that it's deterministic. user_profile_list = (UserProfile.objects.filter(subscription__recipient_id=recipient_id) .select_related() - .order_by('email')) + .order_by('id')) return [{'email': user_profile.email, 'full_name': user_profile.full_name, 'short_name': user_profile.short_name, diff --git a/zerver/tests/test_notifications.py b/zerver/tests/test_notifications.py index 1a30ab859d..cd09669934 100644 --- a/zerver/tests/test_notifications.py +++ b/zerver/tests/test_notifications.py @@ -33,8 +33,8 @@ class TestMissedMessages(ZulipTestCase): # type: () -> List[str] return [str(random.getrandbits(32)) for _ in range(30)] - def _test_cases(self, tokens, msg_id, body, send_as_user): - # type: (List[str], int, str, bool) -> None + def _test_cases(self, tokens, msg_id, body, subject, send_as_user): + # type: (List[str], int, str, str, bool) -> None othello = self.example_user('othello') hamlet = self.example_user('hamlet') handle_missedmessage_emails(hamlet.id, [{'message_id': msg_id}]) @@ -49,6 +49,7 @@ class TestMissedMessages(ZulipTestCase): if send_as_user: from_email = '"%s" <%s>' % (othello.full_name, othello.email) self.assertEqual(msg.from_email, from_email) + self.assertEqual(msg.subject, subject) self.assertEqual(len(msg.reply_to), 1) self.assertIn(msg.reply_to[0], reply_to_addresses) self.assertIn(body, self.normalize_string(msg.body)) @@ -64,7 +65,8 @@ class TestMissedMessages(ZulipTestCase): self.send_message("othello@zulip.com", "Denmark", Recipient.STREAM, '11', subject='test2') msg_id = self.send_message("othello@zulip.com", "denmark", Recipient.STREAM, '@**hamlet**') body = 'Denmark > test Othello, the Moor of Venice 1 2 3 4 5 6 7 8 9 10 @**hamlet**' - self._test_cases(tokens, msg_id, body, send_as_user) + 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): @@ -76,7 +78,8 @@ class TestMissedMessages(ZulipTestCase): Recipient.PERSONAL, 'Extremely personal message!') body = 'You and Othello, the Moor of Venice Extremely personal message!' - self._test_cases(tokens, msg_id, body, send_as_user) + subject = 'Othello, the Moor of Venice sent you a message in Zulip Dev' + self._test_cases(tokens, msg_id, body, subject, send_as_user) @patch('zerver.lib.email_mirror.generate_random_token') def _reply_to_email_in_personal_missed_stream_messages(self, send_as_user, mock_random_token): @@ -88,7 +91,8 @@ class TestMissedMessages(ZulipTestCase): Recipient.PERSONAL, 'Extremely personal message!') body = 'Or just reply to this email.' - self._test_cases(tokens, msg_id, body, send_as_user) + subject = 'Othello, the Moor of Venice sent you a message in Zulip Dev' + self._test_cases(tokens, msg_id, body, subject, send_as_user) @patch('zerver.lib.email_mirror.generate_random_token') def _reply_warning_in_personal_missed_stream_messages(self, send_as_user, mock_random_token): @@ -100,22 +104,56 @@ class TestMissedMessages(ZulipTestCase): Recipient.PERSONAL, 'Extremely personal message!') body = 'Please do not reply to this automated message.' - self._test_cases(tokens, msg_id, body, send_as_user) + subject = 'Othello, the Moor of Venice sent you a message 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_huddle_missed_stream_messages(self, send_as_user, mock_random_token): + def _extra_context_in_huddle_missed_stream_messages_two_others(self, send_as_user, mock_random_token): # type: (bool, MagicMock) -> None tokens = self._get_tokens() mock_random_token.side_effect = tokens msg_id = self.send_message("othello@zulip.com", ["hamlet@zulip.com", "iago@zulip.com"], - Recipient.PERSONAL, + Recipient.HUDDLE, 'Group personal message!') body = ('You and Iago, Othello, the Moor of Venice Othello,' ' the Moor of Venice Group personal message') - self._test_cases(tokens, msg_id, body, send_as_user) + subject = 'Group PMs with Iago and Othello, the Moor of Venice 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_huddle_missed_stream_messages_three_others(self, send_as_user, mock_random_token): + # type: (bool, MagicMock) -> None + tokens = self._get_tokens() + mock_random_token.side_effect = tokens + + msg_id = self.send_message("othello@zulip.com", + ["hamlet@zulip.com", "iago@zulip.com", "cordelia@zulip.com"], + Recipient.HUDDLE, + 'Group personal message!') + + body = ('You and Cordelia Lear, Iago, Othello, the Moor of Venice Othello,' + ' the Moor of Venice Group personal message') + subject = 'Group PMs with Cordelia Lear, Iago, and Othello, the Moor of Venice 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_huddle_missed_stream_messages_many_others(self, send_as_user, mock_random_token): + # type: (bool, MagicMock) -> None + tokens = self._get_tokens() + mock_random_token.side_effect = tokens + + msg_id = self.send_message("othello@zulip.com", + ["hamlet@zulip.com", "iago@zulip.com", "cordelia@zulip.com", "prospero@zulip.com"], + Recipient.HUDDLE, + 'Group personal message!') + + body = ('You and Cordelia Lear, Iago, Othello, the Moor of Venice, Prospero from The Tempest' + ' Othello, the Moor of Venice Group personal message') + subject = 'Group PMs with Cordelia Lear, Iago, and 2 others in Zulip Dev' + self._test_cases(tokens, msg_id, body, subject, send_as_user) @patch('zerver.lib.email_mirror.generate_random_token') def _deleted_message_in_missed_stream_messages(self, send_as_user, mock_random_token): @@ -199,13 +237,31 @@ class TestMissedMessages(ZulipTestCase): self._extra_context_in_personal_missed_stream_messages(False) @override_settings(SEND_MISSED_MESSAGE_EMAILS_AS_USER=True) - def test_extra_context_in_huddle_missed_stream_messages_as_user(self): + def test_extra_context_in_huddle_missed_stream_messages_two_others_as_user(self): # type: () -> None - self._extra_context_in_huddle_missed_stream_messages(True) + self._extra_context_in_huddle_missed_stream_messages_two_others(True) - def test_extra_context_in_huddle_missed_stream_messages(self): + def test_extra_context_in_huddle_missed_stream_messages_two_others(self): # type: () -> None - self._extra_context_in_huddle_missed_stream_messages(False) + self._extra_context_in_huddle_missed_stream_messages_two_others(False) + + @override_settings(SEND_MISSED_MESSAGE_EMAILS_AS_USER=True) + def test_extra_context_in_huddle_missed_stream_messages_three_others_as_user(self): + # type: () -> None + self._extra_context_in_huddle_missed_stream_messages_three_others(True) + + def test_extra_context_in_huddle_missed_stream_messages_three_others(self): + # type: () -> None + self._extra_context_in_huddle_missed_stream_messages_three_others(False) + + @override_settings(SEND_MISSED_MESSAGE_EMAILS_AS_USER=True) + def test_extra_context_in_huddle_missed_stream_messages_many_others_as_user(self): + # type: () -> None + self._extra_context_in_huddle_missed_stream_messages_many_others(True) + + def test_extra_context_in_huddle_missed_stream_messages_many_others(self): + # type: () -> None + self._extra_context_in_huddle_missed_stream_messages_many_others(False) @override_settings(SEND_MISSED_MESSAGE_EMAILS_AS_USER=True) def test_deleted_message_in_missed_stream_messages_as_user(self):