diff --git a/zerver/lib/digest.py b/zerver/lib/digest.py index ec46c76e9d..55f04cf922 100644 --- a/zerver/lib/digest.py +++ b/zerver/lib/digest.py @@ -5,7 +5,6 @@ import datetime import logging import pytz -from django.db.models import Q from django.conf import settings from django.utils.timezone import now as timezone_now @@ -25,11 +24,9 @@ log_to_file(logger, settings.DIGEST_LOG_PATH) DIGEST_CUTOFF = 5 -# Digests accumulate 4 types of interesting traffic for a user: -# 1. Missed PMs -# 2. New streams -# 3. New users -# 4. Interesting stream traffic, as determined by the longest and most +# Digests accumulate 2 types of interesting traffic for a user: +# 1. New streams +# 2. Interesting stream traffic, as determined by the longest and most # diversely comment upon topics. def inactive_since(user_profile: UserProfile, cutoff: datetime.datetime) -> bool: @@ -141,19 +138,6 @@ def gather_hot_conversations(user_profile: UserProfile, messages: List[Message]) hot_conversation_render_payloads.append(teaser_data) return hot_conversation_render_payloads -def gather_new_users(user_profile: UserProfile, threshold: datetime.datetime) -> Tuple[int, List[str]]: - # Gather information on users in the realm who have recently - # joined. - if not user_profile.can_access_all_realm_members(): - new_users = [] # type: List[UserProfile] - else: - new_users = list(UserProfile.objects.filter( - realm=user_profile.realm, date_joined__gt=threshold, - is_bot=False)) - user_names = [user.full_name for user in new_users] - - return len(user_names), user_names - def gather_new_streams(user_profile: UserProfile, threshold: datetime.datetime) -> Tuple[int, Dict[str, List[str]]]: if user_profile.can_access_public_streams(): @@ -175,15 +159,8 @@ def gather_new_streams(user_profile: UserProfile, return len(new_streams), {"html": streams_html, "plain": streams_plain} -def enough_traffic(unread_pms: str, hot_conversations: str, new_streams: int, new_users: int) -> bool: - if unread_pms or hot_conversations: - # If you have any unread traffic, good enough. - return True - if new_streams and new_users: - # If you somehow don't have any traffic but your realm did get - # new streams and users, good enough. - return True - return False +def enough_traffic(hot_conversations: str, new_streams: int) -> bool: + return bool(hot_conversations or new_streams) def handle_digest_email(user_profile_id: int, cutoff: float, render_to_web: bool = False) -> Union[None, Dict[str, Any]]: @@ -209,20 +186,6 @@ def handle_digest_email(user_profile_id: int, cutoff: float, 'unsubscribe_link': one_click_unsubscribe_link(user_profile, "digest") }) - # Gather recent missed PMs, re-using the missed PM email logic. - # You can't have an unread message that you sent, but when testing - # this causes confusion so filter your messages out. - pms = all_messages.filter( - ~Q(message__recipient__type=Recipient.STREAM) & - ~Q(message__sender=user_profile)) - - # Show up to 4 missed PMs. - pms_limit = 4 - - context['unread_pms'] = build_message_list( - user_profile, [pm.message for pm in pms[:pms_limit]]) - context['remaining_unread_pms_count'] = max(0, len(pms) - pms_limit) - home_view_recipients = Subscription.objects.filter( user_profile=user_profile, active=True, @@ -244,17 +207,11 @@ def handle_digest_email(user_profile_id: int, cutoff: float, context["new_streams"] = new_streams context["new_streams_count"] = new_streams_count - # Gather users who signed up recently. - new_users_count, new_users = gather_new_users( - user_profile, cutoff_date) - context["new_users"] = new_users - if render_to_web: return context # We don't want to send emails containing almost no information. - if enough_traffic(context["unread_pms"], context["hot_conversations"], - new_streams_count, new_users_count): + if enough_traffic(context["hot_conversations"], new_streams_count): logger.info("Sending digest email for %s" % (user_profile.email,)) # Send now, as a ScheduledEmail send_future_email('zerver/emails/digest', user_profile.realm, to_user_ids=[user_profile.id], diff --git a/zerver/tests/test_digest.py b/zerver/tests/test_digest.py index 234b9341f9..ab167898ef 100644 --- a/zerver/tests/test_digest.py +++ b/zerver/tests/test_digest.py @@ -8,8 +8,7 @@ from django.test import override_settings from django.utils.timezone import now as timezone_now from zerver.lib.actions import create_stream_if_needed, do_create_user -from zerver.lib.digest import gather_new_streams, handle_digest_email, enqueue_emails, \ - gather_new_users +from zerver.lib.digest import gather_new_streams, handle_digest_email, enqueue_emails from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import queries_captured from zerver.models import get_client, get_realm, flush_per_request_caches, \ @@ -17,74 +16,6 @@ from zerver.models import get_client, get_realm, flush_per_request_caches, \ class TestDigestEmailMessages(ZulipTestCase): - @mock.patch('zerver.lib.digest.enough_traffic') - @mock.patch('zerver.lib.digest.send_future_email') - def test_receive_digest_email_messages(self, mock_send_future_email: mock.MagicMock, - mock_enough_traffic: mock.MagicMock) -> None: - - # build dummy messages for missed messages email reply - # have Hamlet send Othello a PM. Othello will reply via email - # Hamlet will receive the message. - hamlet = self.example_user('hamlet') - self.login(hamlet.email) - result = self.client_post("/json/messages", {"type": "private", - "content": "test_receive_missed_message_email_messages", - "client": "test suite", - "to": self.example_email('othello')}) - self.assert_json_success(result) - - user_profile = self.example_user('othello') - cutoff = time.mktime(datetime.datetime(year=2016, month=1, day=1).timetuple()) - - handle_digest_email(user_profile.id, cutoff) - self.assertEqual(mock_send_future_email.call_count, 1) - - kwargs = mock_send_future_email.call_args[1] - self.assertEqual(kwargs['to_user_ids'], [user_profile.id]) - html = kwargs['context']['unread_pms'][0]['header']['html'] - expected_url = "'http://zulip.testserver/#narrow/pm-with/{id}-hamlet'".format(id=hamlet.id) - self.assertIn(expected_url, html) - - @mock.patch('zerver.lib.digest.enough_traffic') - @mock.patch('zerver.lib.digest.send_future_email') - def test_huddle_urls(self, mock_send_future_email: mock.MagicMock, - mock_enough_traffic: mock.MagicMock) -> None: - - email = self.example_email('hamlet') - self.login(email) - - huddle_emails = [ - self.example_email('cordelia'), - self.example_email('othello'), - ] - - payload = dict( - type='private', - content='huddle message', - client='test suite', - to=','.join(huddle_emails), - ) - result = self.client_post("/json/messages", payload) - self.assert_json_success(result) - - user_profile = self.example_user('othello') - cutoff = time.mktime(datetime.datetime(year=2016, month=1, day=1).timetuple()) - - handle_digest_email(user_profile.id, cutoff) - self.assertEqual(mock_send_future_email.call_count, 1) - - kwargs = mock_send_future_email.call_args[1] - self.assertEqual(kwargs['to_user_ids'], [user_profile.id]) - html = kwargs['context']['unread_pms'][0]['header']['html'] - - other_user_ids = sorted([ - self.example_user('cordelia').id, - self.example_user('hamlet').id, - ]) - slug = ','.join(str(user_id) for user_id in other_user_ids) + '-group' - expected_url = "'http://zulip.testserver/#narrow/pm-with/" + slug + "'" - self.assertIn(expected_url, html) - @mock.patch('zerver.lib.digest.enough_traffic') @mock.patch('zerver.lib.digest.send_future_email') def test_multiple_stream_senders(self, @@ -121,7 +52,7 @@ class TestDigestEmailMessages(ZulipTestCase): with queries_captured() as queries: handle_digest_email(othello.id, cutoff) - self.assertTrue(24 <= len(queries) <= 25) + self.assertTrue(21 <= len(queries) <= 22) self.assertEqual(mock_send_future_email.call_count, 1) kwargs = mock_send_future_email.call_args[1] @@ -245,32 +176,6 @@ class TestDigestEmailMessages(ZulipTestCase): expected_html = "New stream".format(stream_id=stream_id) self.assertIn(expected_html, new_stream['html']) - @mock.patch('zerver.lib.digest.timezone_now') - def test_gather_new_users(self, mock_django_timezone: mock.MagicMock) -> None: - cutoff = timezone_now() - do_create_user('abc@example.com', password='abc', realm=get_realm('zulip'), full_name='abc', short_name='abc') - - # Normal users get info about new users - user = self.example_user('aaron') - gathered_no_of_user, _ = gather_new_users(user, cutoff) - self.assertEqual(gathered_no_of_user, 1) - - # Definitely, admin users get info about new users - user = self.example_user('iago') - gathered_no_of_user, _ = gather_new_users(user, cutoff) - self.assertEqual(gathered_no_of_user, 1) - - # Guest users don't get info about new users - user = self.example_user('polonius') - gathered_no_of_user, _ = gather_new_users(user, cutoff) - self.assertEqual(gathered_no_of_user, 0) - - # Zephyr users also don't get info about new users in their realm - user = self.mit_user('starnine') - do_create_user('abc@mit.edu', password='abc', realm=user.realm, full_name='abc', short_name='abc') - gathered_no_of_user, _ = gather_new_users(user, cutoff) - self.assertEqual(gathered_no_of_user, 0) - class TestDigestContentInBrowser(ZulipTestCase): def test_get_digest_content_in_browser(self) -> None: self.login(self.example_email('hamlet'))