paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-03 21:43:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

missed-message: Merge before calling handle_missedmessage_emails.

Browse Source 
The MissedMessage queue worker is the single callsite of
`handle_missedmessage_emails`, which immediately transforms the list
of events into a dict keyed by message-id.

Skip the intermediate list step, and use defaultdict and a dataclass
to simplify and make explicit the pieces.  This removes the unused
user_profile_id and message_id pieces of the data structure.
This commit is contained in:
Alex Vandiver
2023-07-12 17:05:57 +00:00
committed by Tim Abbott
parent c7d9a4784e
commit d87895a3ef
3 changed files with 124 additions and 165 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
zerver/lib/email_notifications.py
View File

@@ -6,9 +6,10 @@ import re
import subprocess import subprocess
import sys import sys
from collections import defaultdict from collections import defaultdict
from dataclasses import dataclass
from datetime import timedelta from datetime import timedelta
from email.headerregistry import Address from email.headerregistry import Address
from typing import Any, Dict, Iterable, List, Optional, Tuple, Union from typing import Any, Dict, List, Optional, Tuple, Union
import lxml.html import lxml.html
from bs4 import BeautifulSoup from bs4 import BeautifulSoup
@@ -604,17 +605,15 @@ def do_send_missedmessage_events_reply_in_zulip(
user_profile.save(update_fields=["last_reminder"]) user_profile.save(update_fields=["last_reminder"])
def handle_missedmessage_emails( @dataclass
user_profile_id: int, missed_email_events: Iterable[Dict[str, Any]] class MissedMessageData:
) -> None: trigger: str
message_ids = { mentioned_user_group_id: Optional[int] = None
event.get("message_id"): {
"trigger": event.get("trigger"),
"mentioned_user_group_id": event.get("mentioned_user_group_id"),
}
for event in missed_email_events
}
def handle_missedmessage_emails(
user_profile_id: int, message_ids: Dict[int, MissedMessageData]
) -> None:
user_profile = get_user_profile_by_id(user_profile_id) user_profile = get_user_profile_by_id(user_profile_id)
if user_profile.is_bot: # nocoverage if user_profile.is_bot: # nocoverage
# We don't expect to reach here for bot users. However, this code exists # We don't expect to reach here for bot users. However, this code exists
@@ -679,8 +678,8 @@ def handle_missedmessage_emails(
message_info = message_ids.get(m.id) message_info = message_ids.get(m.id)
unique_messages[m.id] = dict( unique_messages[m.id] = dict(
message=m, message=m,
trigger=message_info["trigger"] if message_info else None, trigger=message_info.trigger if message_info else None,
mentioned_user_group_id=message_info.get("mentioned_user_group_id") mentioned_user_group_id=message_info.mentioned_user_group_id
if message_info is not None if message_info is not None
else None, else None,
) )

213
zerver/tests/test_email_notifications.py
View File

@@ -24,6 +24,7 @@ from zerver.actions.user_settings import do_change_user_setting
from zerver.actions.user_topics import do_set_user_topic_visibility_policy from zerver.actions.user_topics import do_set_user_topic_visibility_policy
from zerver.actions.users import do_change_user_role from zerver.actions.users import do_change_user_role
from zerver.lib.email_notifications import ( from zerver.lib.email_notifications import (
MissedMessageData,
enqueue_welcome_emails, enqueue_welcome_emails,
fix_emojis, fix_emojis,
fix_spoilers_in_html, fix_spoilers_in_html,
@@ -546,7 +547,7 @@ class TestMissedMessages(ZulipTestCase):
"zerver.lib.email_notifications.do_send_missedmessage_events_reply_in_zulip" "zerver.lib.email_notifications.do_send_missedmessage_events_reply_in_zulip"
) as m: ) as m:
handle_missedmessage_emails( handle_missedmessage_emails(
cordelia.id, [{"message_id": message.id, "trigger": "private_message"}] cordelia.id, {message.id: MissedMessageData(trigger="private_message")}
) )
m.assert_not_called() m.assert_not_called()
@@ -555,7 +556,7 @@ class TestMissedMessages(ZulipTestCase):
"zerver.lib.email_notifications.do_send_missedmessage_events_reply_in_zulip" "zerver.lib.email_notifications.do_send_missedmessage_events_reply_in_zulip"
) as m: ) as m:
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, [{"message_id": message.id, "trigger": "private_message"}] hamlet.id, {message.id: MissedMessageData(trigger="private_message")}
) )
m.assert_called_once() m.assert_called_once()
@@ -571,7 +572,7 @@ class TestMissedMessages(ZulipTestCase):
"zerver.lib.email_notifications.do_send_missedmessage_events_reply_in_zulip" "zerver.lib.email_notifications.do_send_missedmessage_events_reply_in_zulip"
) as m: ) as m:
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, [{"message_id": message.id, "trigger": "private_message"}] hamlet.id, {message.id: MissedMessageData(trigger="private_message")}
) )
m.assert_not_called() m.assert_not_called()
@@ -600,13 +601,11 @@ class TestMissedMessages(ZulipTestCase):
with patch("zerver.lib.email_mirror.generate_missed_message_token", side_effect=tokens): with patch("zerver.lib.email_mirror.generate_missed_message_token", side_effect=tokens):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": msg_id, msg_id: MissedMessageData(
"trigger": trigger, trigger=trigger, mentioned_user_group_id=mentioned_user_group_id
"mentioned_user_group_id": mentioned_user_group_id, )
} },
],
) )
if settings.EMAIL_GATEWAY_PATTERN != "": if settings.EMAIL_GATEWAY_PATTERN != "":
reply_to_addresses = [settings.EMAIL_GATEWAY_PATTERN % (t,) for t in tokens] reply_to_addresses = [settings.EMAIL_GATEWAY_PATTERN % (t,) for t in tokens]
@@ -998,7 +997,7 @@ class TestMissedMessages(ZulipTestCase):
self.login("othello") self.login("othello")
result = self.client_patch("/json/messages/" + str(msg_id), {"content": " "}) result = self.client_patch("/json/messages/" + str(msg_id), {"content": " "})
self.assert_json_success(result) self.assert_json_success(result)
handle_missedmessage_emails(hamlet.id, [{"message_id": msg_id}]) handle_missedmessage_emails(hamlet.id, {msg_id: MissedMessageData(trigger="mentioned")})
self.assert_length(mail.outbox, 0) self.assert_length(mail.outbox, 0)
def _deleted_message_in_missed_personal_messages(self, send_as_user: bool) -> None: def _deleted_message_in_missed_personal_messages(self, send_as_user: bool) -> None:
@@ -1012,7 +1011,9 @@ class TestMissedMessages(ZulipTestCase):
self.login("othello") self.login("othello")
result = self.client_patch("/json/messages/" + str(msg_id), {"content": " "}) result = self.client_patch("/json/messages/" + str(msg_id), {"content": " "})
self.assert_json_success(result) self.assert_json_success(result)
handle_missedmessage_emails(hamlet.id, [{"message_id": msg_id}]) handle_missedmessage_emails(
hamlet.id, {msg_id: MissedMessageData(trigger="private_message")}
)
self.assert_length(mail.outbox, 0) self.assert_length(mail.outbox, 0)
def _deleted_message_in_missed_huddle_messages(self, send_as_user: bool) -> None: def _deleted_message_in_missed_huddle_messages(self, send_as_user: bool) -> None:
@@ -1030,9 +1031,11 @@ class TestMissedMessages(ZulipTestCase):
self.login("othello") self.login("othello")
result = self.client_patch("/json/messages/" + str(msg_id), {"content": " "}) result = self.client_patch("/json/messages/" + str(msg_id), {"content": " "})
self.assert_json_success(result) self.assert_json_success(result)
handle_missedmessage_emails(hamlet.id, [{"message_id": msg_id}]) handle_missedmessage_emails(
hamlet.id, {msg_id: MissedMessageData(trigger="private_message")}
)
self.assert_length(mail.outbox, 0) self.assert_length(mail.outbox, 0)
handle_missedmessage_emails(iago.id, [{"message_id": msg_id}]) handle_missedmessage_emails(iago.id, {msg_id: MissedMessageData(trigger="private_message")})
self.assert_length(mail.outbox, 0) self.assert_length(mail.outbox, 0)
def test_smaller_user_group_mention_priority(self) -> None: def test_smaller_user_group_mention_priority(self) -> None:
@@ -1054,18 +1057,14 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": hamlet_only_message_id, hamlet_only_message_id: MissedMessageData(
"trigger": "mentioned", trigger="mentioned", mentioned_user_group_id=hamlet_only.id
"mentioned_user_group_id": hamlet_only.id, ),
hamlet_and_cordelia_message_id: MissedMessageData(
trigger="mentioned", mentioned_user_group_id=hamlet_and_cordelia.id
),
}, },
{
"message_id": hamlet_and_cordelia_message_id,
"trigger": "mentioned",
"mentioned_user_group_id": hamlet_and_cordelia.id,
},
],
) )
expected_email_include = [ expected_email_include = [
@@ -1094,18 +1093,12 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": user_group_mentioned_message_id, user_group_mentioned_message_id: MissedMessageData(
"trigger": "mentioned", trigger="mentioned", mentioned_user_group_id=hamlet_and_cordelia.id
"mentioned_user_group_id": hamlet_and_cordelia.id, ),
personal_mentioned_message_id: MissedMessageData(trigger="mentioned"),
}, },
{
"message_id": personal_mentioned_message_id,
"trigger": "mentioned",
"mentioned_user_group_id": None,
},
],
) )
expected_email_include = [ expected_email_include = [
@@ -1134,18 +1127,14 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": stream_wildcard_mentioned_in_followed_topic_message_id, stream_wildcard_mentioned_in_followed_topic_message_id: MissedMessageData(
"trigger": "stream_wildcard_mentioned_in_followed_topic", trigger="stream_wildcard_mentioned_in_followed_topic"
"mentioned_user_group_id": None, ),
user_group_mentioned_message_id: MissedMessageData(
trigger="mentioned", mentioned_user_group_id=hamlet_and_cordelia.id
),
}, },
{
"message_id": user_group_mentioned_message_id,
"trigger": "mentioned",
"mentioned_user_group_id": hamlet_and_cordelia.id,
},
],
) )
expected_email_include = [ expected_email_include = [
@@ -1169,18 +1158,14 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": stream_wildcard_mentioned_message_id, stream_wildcard_mentioned_message_id: MissedMessageData(
"trigger": "stream_wildcard_mentioned", trigger="stream_wildcard_mentioned"
"mentioned_user_group_id": None, ),
stream_wildcard_mentioned_in_followed_topic_message_id: MissedMessageData(
trigger="stream_wildcard_mentioned_in_followed_topic"
),
}, },
{
"message_id": stream_wildcard_mentioned_in_followed_topic_message_id,
"trigger": "stream_wildcard_mentioned_in_followed_topic",
"mentioned_user_group_id": None,
},
],
) )
expected_email_include = [ expected_email_include = [
@@ -1202,18 +1187,14 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": followed_topic_mentioned_message_id, followed_topic_mentioned_message_id: MissedMessageData(
"trigger": "followed_topic_email_notify", trigger="followed_topic_email_notify"
"mentioned_user_group_id": None, ),
stream_wildcard_mentioned_message_id: MissedMessageData(
trigger="stream_wildcard_mentioned"
),
}, },
{
"message_id": stream_wildcard_mentioned_message_id,
"trigger": "stream_wildcard_mentioned",
"mentioned_user_group_id": None,
},
],
) )
expected_email_include = [ expected_email_include = [
@@ -1233,18 +1214,12 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": stream_mentioned_message_id, stream_mentioned_message_id: MissedMessageData(trigger="stream_email_notify"),
"trigger": "stream_email_notify", followed_topic_mentioned_message_id: MissedMessageData(
"mentioned_user_group_id": None, trigger="followed_topic_email_notify"
),
}, },
{
"message_id": followed_topic_mentioned_message_id,
"trigger": "followed_topic_email_notify",
"mentioned_user_group_id": None,
},
],
) )
expected_email_include = [ expected_email_include = [
@@ -1599,11 +1574,11 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {
{"message_id": msg_id_1, "trigger": "mentioned"}, msg_id_1: MissedMessageData(trigger="mentioned"),
{"message_id": msg_id_2, "trigger": "stream_email_notify"}, msg_id_2: MissedMessageData(trigger="stream_email_notify"),
{"message_id": msg_id_3}, msg_id_3: MissedMessageData(trigger="private_message"),
], },
) )
assert isinstance(mail.outbox[0], EmailMultiAlternatives) assert isinstance(mail.outbox[0], EmailMultiAlternatives)
@@ -1644,10 +1619,10 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {
{"message_id": msg_id_1}, msg_id_1: MissedMessageData(trigger="private_message"),
{"message_id": msg_id_2}, msg_id_2: MissedMessageData(trigger="private_message"),
], },
) )
self.assert_length(mail.outbox, 2) self.assert_length(mail.outbox, 2)
email_subject = "DMs with Othello, the Moor of Venice" email_subject = "DMs with Othello, the Moor of Venice"
@@ -1662,10 +1637,10 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {
{"message_id": msg_id_1, "trigger": "stream_email_notify"}, msg_id_1: MissedMessageData(trigger="stream_email_notify"),
{"message_id": msg_id_2, "trigger": "stream_email_notify"}, msg_id_2: MissedMessageData(trigger="stream_email_notify"),
], },
) )
self.assert_length(mail.outbox, 1) self.assert_length(mail.outbox, 1)
email_subject = "#Denmark > test" email_subject = "#Denmark > test"
@@ -1681,10 +1656,10 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {
{"message_id": msg_id_1, "trigger": "stream_email_notify"}, msg_id_1: MissedMessageData(trigger="stream_email_notify"),
{"message_id": msg_id_2, "trigger": "mentioned"}, msg_id_2: MissedMessageData(trigger="mentioned"),
], },
) )
self.assert_length(mail.outbox, 1) self.assert_length(mail.outbox, 1)
email_subject = "#Denmark > test" email_subject = "#Denmark > test"
@@ -1709,9 +1684,7 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
late_subscribed_user.id, late_subscribed_user.id,
[ {mention_msg_id: MissedMessageData(trigger="mentioned")},
{"message_id": mention_msg_id, "trigger": "mentioned"},
],
) )
self.assert_length(mail.outbox, 1) self.assert_length(mail.outbox, 1)
@@ -1738,11 +1711,11 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {
{"message_id": msg_id_1, "trigger": "mentioned"}, msg_id_1: MissedMessageData(trigger="mentioned"),
{"message_id": msg_id_2, "trigger": "mentioned"}, msg_id_2: MissedMessageData(trigger="mentioned"),
{"message_id": msg_id_3, "trigger": "stream_email_notify"}, msg_id_3: MissedMessageData(trigger="stream_email_notify"),
], },
) )
self.assert_length(mail.outbox, 1) self.assert_length(mail.outbox, 1)
email_subject = "#Denmark > test" email_subject = "#Denmark > test"
@@ -1758,10 +1731,10 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {
{"message_id": msg_id_1, "trigger": "stream_email_notify"}, msg_id_1: MissedMessageData(trigger="stream_email_notify"),
{"message_id": msg_id_2, "trigger": "stream_email_notify"}, msg_id_2: MissedMessageData(trigger="stream_email_notify"),
], },
) )
self.assert_length(mail.outbox, 2) self.assert_length(mail.outbox, 2)
email_subjects = {mail.outbox[0].subject, mail.outbox[1].subject} email_subjects = {mail.outbox[0].subject, mail.outbox[1].subject}
@@ -1957,7 +1930,7 @@ class TestMissedMessages(ZulipTestCase):
stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention)
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[{"message_id": stream_mentioned_message_id, "trigger": "mentioned"}], {stream_mentioned_message_id: MissedMessageData(trigger="mentioned")},
) )
# Direct message should soft reactivate the user # Direct message should soft reactivate the user
@@ -1966,7 +1939,7 @@ class TestMissedMessages(ZulipTestCase):
personal_message_id = self.send_personal_message(othello, hamlet, "Message") personal_message_id = self.send_personal_message(othello, hamlet, "Message")
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[{"message_id": personal_message_id, "trigger": "private_message"}], {personal_message_id: MissedMessageData(trigger="private_message")},
) )
# Hamlet FOLLOWS the topic. # Hamlet FOLLOWS the topic.
@@ -1986,12 +1959,11 @@ class TestMissedMessages(ZulipTestCase):
stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention)
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": stream_mentioned_message_id, stream_mentioned_message_id: MissedMessageData(
"trigger": "stream_wildcard_mentioned_in_followed_topic", trigger="stream_wildcard_mentioned_in_followed_topic"
} ),
], },
) )
# Reset # Reset
@@ -2009,12 +1981,11 @@ class TestMissedMessages(ZulipTestCase):
stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention)
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": stream_mentioned_message_id, stream_mentioned_message_id: MissedMessageData(
"trigger": "stream_wildcard_mentioned", trigger="stream_wildcard_mentioned"
} ),
], },
) )
# Group mention should NOT soft reactivate the user # Group mention should NOT soft reactivate the user
@@ -2023,13 +1994,11 @@ class TestMissedMessages(ZulipTestCase):
stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention) stream_mentioned_message_id = self.send_stream_message(othello, "Denmark", mention)
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[
{ {
"message_id": stream_mentioned_message_id, stream_mentioned_message_id: MissedMessageData(
"trigger": "mentioned", trigger="mentioned", mentioned_user_group_id=large_user_group.id
"mentioned_user_group_id": large_user_group.id, ),
} },
],
) )
def test_followed_topic_missed_message(self) -> None: def test_followed_topic_missed_message(self) -> None:
@@ -2039,9 +2008,7 @@ class TestMissedMessages(ZulipTestCase):
handle_missedmessage_emails( handle_missedmessage_emails(
hamlet.id, hamlet.id,
[ {msg_id: MissedMessageData(trigger="followed_topic_email_notify")},
{"message_id": msg_id, "trigger": "followed_topic_email_notify"},
],
) )
self.assert_length(mail.outbox, 1) self.assert_length(mail.outbox, 1)
email_subject = mail.outbox[0].subject email_subject = mail.outbox[0].subject

19
zerver/worker/queue_processors.py
View File

@@ -14,7 +14,7 @@ import threading
import time import time
import urllib import urllib
from abc import ABC, abstractmethod from abc import ABC, abstractmethod
from collections import deque from collections import defaultdict, deque
from email.message import EmailMessage from email.message import EmailMessage
from functools import wraps from functools import wraps
from types import FrameType from types import FrameType
@@ -64,7 +64,7 @@ from zerver.lib.email_mirror import (
rate_limit_mirror_by_realm, rate_limit_mirror_by_realm,
) )
from zerver.lib.email_mirror import process_message as mirror_email from zerver.lib.email_mirror import process_message as mirror_email
from zerver.lib.email_notifications import handle_missedmessage_emails from zerver.lib.email_notifications import MissedMessageData, handle_missedmessage_emails
from zerver.lib.error_notify import do_report_error from zerver.lib.error_notify import do_report_error
from zerver.lib.exceptions import RateLimitedError from zerver.lib.exceptions import RateLimitedError
from zerver.lib.export import export_realm_wrapper from zerver.lib.export import export_realm_wrapper
@@ -717,21 +717,14 @@ class MissedMessageWorker(QueueProcessingWorker):
) )
# Batch the entries by user # Batch the entries by user
events_by_recipient: Dict[int, List[Dict[str, Any]]] = {} events_by_recipient: Dict[int, Dict[int, MissedMessageData]] = defaultdict(dict)
for event in events_to_process: for event in events_to_process:
entry = dict( events_by_recipient[event.user_profile_id][event.message_id] = MissedMessageData(
user_profile_id=event.user_profile_id, trigger=event.trigger, mentioned_user_group_id=event.mentioned_user_group_id
message_id=event.message_id,
trigger=event.trigger,
mentioned_user_group_id=event.mentioned_user_group_id,
) )
if event.user_profile_id in events_by_recipient:
events_by_recipient[event.user_profile_id].append(entry)
else:
events_by_recipient[event.user_profile_id] = [entry]
for user_profile_id in events_by_recipient: for user_profile_id in events_by_recipient:
events: List[Dict[str, Any]] = events_by_recipient[user_profile_id] events = events_by_recipient[user_profile_id]
logging.info( logging.info(
"Batch-processing %s missedmessage_emails events for user %s", "Batch-processing %s missedmessage_emails events for user %s",