From 278f8bb9958c16741eb760c2e4f4a407d350914a Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Fri, 28 Aug 2026 19:51:02 -0600 Subject: [PATCH] Also email room subscribers on the first unread message, not just mentions (#67) The previous pass only emailed on a mention. Corrected scope: the room's first unread message triggers one debounced email (same shape as #66), and every mention additionally emails regardless of that debounce, since a mention shouldn't get silently absorbed by an earlier plain message's already-sent notification. Co-Authored-By: Claude Sonnet 5 --- backend/app/models/membership.py | 9 +- backend/app/schemas/room.py | 8 +- backend/app/services/message_events.py | 91 +++++++++++-------- .../test_room_mention_email_notifications.py | 64 +++++++++---- frontend/src/api/rooms.ts | 7 +- frontend/src/components/RoomInfoPanel.tsx | 4 +- frontend/src/types.ts | 5 +- 7 files changed, 120 insertions(+), 68 deletions(-) diff --git a/backend/app/models/membership.py b/backend/app/models/membership.py index 67cbb99..7429bbf 100644 --- a/backend/app/models/membership.py +++ b/backend/app/models/membership.py @@ -42,10 +42,11 @@ class RoomMembership(Base): # arrives in the room, or when find_or_create_dm resolves back to it -- # both count as the conversation being active again. hidden_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) - # #67: opt-in, per-member -- email when a message in this room mentions - # them while they're offline. Deliberately separate from #66's DM - # emails (always-on, no toggle) rather than a shared flag, since DMs - # are explicitly out of scope for this setting (see message_events.py). + # #67: opt-in, per-member -- email while offline on the room's first + # unread message, plus every mention regardless of that debounce (see + # message_events.py's _maybe_email_room_notifications). Deliberately + # separate from #66's DM emails (always-on, no toggle) rather than a + # shared flag, since DMs are explicitly out of scope for this setting. email_notifications: Mapped[bool] = mapped_column( Boolean, default=False, server_default="false", nullable=False ) diff --git a/backend/app/schemas/room.py b/backend/app/schemas/room.py index c526db5..93eaca5 100644 --- a/backend/app/schemas/room.py +++ b/backend/app/schemas/room.py @@ -63,9 +63,11 @@ class MyRoomItem(RoomRead): # avatar, not this room's internal `name`) without a second fetch per # row. None for a regular room. dm_partner: DmPartnerInfo | None = None - # #67: this viewer's own opt-in for "email me when mentioned here while - # offline" -- always false for a DM (see message_events.py, deliberately - # out of scope; DMs already get #66's automatic offline email). + # #67: this viewer's own opt-in for email while offline -- the room's + # first unread message plus every mention (see message_events.py's + # _maybe_email_room_notifications for the exact debounce shape). Always + # false for a DM, deliberately out of scope -- DMs already get #66's + # automatic offline email. email_notifications: bool = False diff --git a/backend/app/services/message_events.py b/backend/app/services/message_events.py index 0d882a2..f3a5817 100644 --- a/backend/app/services/message_events.py +++ b/backend/app/services/message_events.py @@ -249,7 +249,7 @@ async def _maybe_email_dm_notification( ) -async def _maybe_email_room_mention_notifications( +async def _maybe_email_room_notifications( db: AsyncSession, global_presence: GlobalPresence, base_url: str, @@ -257,11 +257,16 @@ async def _maybe_email_room_mention_notifications( sender: User, message: Message, ) -> None: - """#67: opt-in, per-room email on mention -- deliberately scoped to - regular rooms only (see set_room_email_notifications: DMs already get - #66's always-on offline email, no separate toggle). Debounced the same - shape as #66 -- only the first unread mention in this room emails, not - one per mention while the recipient is away. + """#67: opt-in, per-room email -- deliberately scoped to regular rooms + only (see set_room_email_notifications: DMs already get #66's + always-on offline email, no separate toggle). + + Two triggers, not one: the room's first unread message debounces the + same way #66 does (one email per unread burst, not one per message), + but a mention always emails regardless of that debounce -- a mention + is a stronger, individually-addressed signal that shouldn't get + silently swallowed just because an earlier plain message in the same + burst already used up the "first unread" email. """ room = await db.get(Room, room_id) if room is None or room.is_dm: @@ -270,20 +275,21 @@ async def _maybe_email_room_mention_notifications( result = await db.execute( select(MessageMention.user_id).where(MessageMention.message_id == message.id) ) - mentioned_ids = {row[0] for row in result.all()} - {sender.id} - if not mentioned_ids: + mentioned_ids = {row[0] for row in result.all()} + + result = await db.execute( + select(RoomMembership).where( + RoomMembership.room_id == room_id, + RoomMembership.user_id != sender.id, + RoomMembership.email_notifications.is_(True), + ) + ) + subscribed_memberships = result.scalars().all() + if not subscribed_memberships: return - for user_id in mentioned_ids: - membership_result = await db.execute( - select(RoomMembership).where( - RoomMembership.room_id == room_id, RoomMembership.user_id == user_id - ) - ) - membership = membership_result.scalar_one_or_none() - if membership is None or not membership.email_notifications: - continue - + for membership in subscribed_memberships: + user_id = membership.user_id recipient = await db.get(User, user_id) if recipient is None: continue @@ -291,30 +297,41 @@ async def _maybe_email_room_mention_notifications( if not recipient.appear_offline and await global_presence.is_online(recipient.id): continue - already_unread_mention = await db.execute( - select(Message.id) - .join(MessageMention, MessageMention.message_id == Message.id) - .where( - MessageMention.user_id == user_id, - Message.room_id == room_id, - Message.id != message.id, - Message.created_at > membership.last_read_at, + mentioned = user_id in mentioned_ids + if not mentioned: + already_unread = await db.execute( + select(Message.id) + .where( + Message.room_id == room_id, + Message.id != message.id, + Message.created_at > membership.last_read_at, + ) + .limit(1) ) - .limit(1) - ) - if already_unread_mention.scalar_one_or_none() is not None: - continue + if already_unread.scalar_one_or_none() is not None: + continue + + if mentioned: + subject = f"New mention in #{room.name}" + body_line = ( + f"{sender.username} mentioned you: {message.content[:200]}" + if message.content + else f"{sender.username} mentioned you" + ) + else: + subject = f"New message in #{room.name}" + if message.content: + body_line = f"{sender.username}: {message.content[:200]}" + elif message.file_id: + body_line = f"{sender.username} sent a file" + else: + body_line = f"{sender.username} sent an image" - body_line = ( - f"{sender.username} mentioned you: {message.content[:200]}" - if message.content - else f"{sender.username} mentioned you" - ) link = f"{base_url.rstrip('/')}/rooms/{room_id}" await send_email( db, recipient.email, - f"New mention in #{room.name}", + subject, [body_line], cta_label="Open room", cta_url=link, @@ -361,7 +378,7 @@ async def broadcast_new_message( ) await _notify_offline_members(db, broadcaster, presence, focus_presence, room_id, sender, message) await _maybe_email_dm_notification(db, global_presence, base_url, room_id, sender, message) - await _maybe_email_room_mention_notifications(db, global_presence, base_url, room_id, sender, message) + await _maybe_email_room_notifications(db, global_presence, base_url, room_id, sender, message) await dispatch_event(db, "message.created", room_id, payload) _maybe_fetch_link_preview(broadcaster, room_id, message) diff --git a/backend/tests/test_room_mention_email_notifications.py b/backend/tests/test_room_mention_email_notifications.py index 8fe5275..c155a8a 100644 --- a/backend/tests/test_room_mention_email_notifications.py +++ b/backend/tests/test_room_mention_email_notifications.py @@ -73,7 +73,7 @@ def _subscribe(ws_client, room_id: str, username: str, password: str = "password assert resp.status_code == 204, resp.text -def test_room_mention_emails_offline_subscribed_member(ws_client_factory, monkeypatch): +def test_room_first_message_emails_offline_subscribed_member(ws_client_factory, monkeypatch): calls = _fake_smtp(monkeypatch) instance1 = ws_client_factory() instance2 = ws_client_factory() @@ -88,18 +88,18 @@ def test_room_mention_emails_offline_subscribed_member(ws_client_factory, monkey with instance1.websocket_connect("/ws/chat") as alice_ws: alice_ws.send_json({"type": "join", "room_id": room["id"]}) assert alice_ws.receive_json()["type"] == "joined" - _send_and_sync(alice_ws, room["id"], f"hey @{bob['username']}, look at this") + _send_and_sync(alice_ws, room["id"], "no mention here, just a plain message") assert len(calls) == 1 email = calls[0]["message"] assert email["To"] == bob["email"] - assert f"New mention in #{room['name']}" in email["Subject"] + assert f"New message in #{room['name']}" in email["Subject"] body = email.get_body(preferencelist=("plain",)).get_content() assert alice["username"] in body assert f"/rooms/{room['id']}" in body -def test_room_message_without_mention_does_not_email_subscribed_member(ws_client_factory, monkeypatch): +def test_room_second_plain_message_does_not_reemail_before_read(ws_client_factory, monkeypatch): calls = _fake_smtp(monkeypatch) instance1 = ws_client_factory() instance2 = ws_client_factory() @@ -113,9 +113,45 @@ def test_room_message_without_mention_does_not_email_subscribed_member(ws_client with instance1.websocket_connect("/ws/chat") as alice_ws: alice_ws.send_json({"type": "join", "room_id": room["id"]}) assert alice_ws.receive_json()["type"] == "joined" - _send_and_sync(alice_ws, room["id"], "hello room, no mention here") - assert calls == [] + _send_and_sync(alice_ws, room["id"], "message one") + assert len(calls) == 1 + + # A second plain message while bob still hasn't read the first -- + # no second email for the same unread burst. + _send_and_sync(alice_ws, room["id"], "message two") + assert len(calls) == 1 + + +def test_room_mention_always_emails_even_mid_unread_burst(ws_client_factory, monkeypatch): + calls = _fake_smtp(monkeypatch) + instance1 = ws_client_factory() + instance2 = ws_client_factory() + + alice = _register_ws(instance1, _unique("alice")) + bob = _register_ws(instance2, _unique("bob")) + room = instance1.post("/api/rooms", json={"name": _unique("general")}).json() + instance2.post(f"/api/rooms/{room['id']}/join") + _subscribe(instance2, room["id"], bob["username"]) + + with instance1.websocket_connect("/ws/chat") as alice_ws: + alice_ws.send_json({"type": "join", "room_id": room["id"]}) + assert alice_ws.receive_json()["type"] == "joined" + + # First unread message (plain) -- emails once, uses up the debounce. + _send_and_sync(alice_ws, room["id"], "hey everyone") + assert len(calls) == 1 + + # A mention arriving while that first message is still unread -- + # must email anyway, unlike a second plain message. + _send_and_sync(alice_ws, room["id"], f"@{bob['username']} specifically you") + assert len(calls) == 2 + + mention_email = calls[1]["message"] + assert mention_email["To"] == bob["email"] + assert f"New mention in #{room['name']}" in mention_email["Subject"] + body = mention_email.get_body(preferencelist=("plain",)).get_content() + assert f"{alice['username']} mentioned you" in body def test_room_mention_does_not_email_unsubscribed_member(ws_client_factory, monkeypatch): @@ -123,7 +159,7 @@ def test_room_mention_does_not_email_unsubscribed_member(ws_client_factory, monk instance1 = ws_client_factory() instance2 = ws_client_factory() - alice = _register_ws(instance1, _unique("alice")) + _register_ws(instance1, _unique("alice")) bob = _register_ws(instance2, _unique("bob")) room = instance1.post("/api/rooms", json={"name": _unique("general")}).json() instance2.post(f"/api/rooms/{room['id']}/join") @@ -137,7 +173,7 @@ def test_room_mention_does_not_email_unsubscribed_member(ws_client_factory, monk assert calls == [] -def test_room_mention_does_not_email_online_subscribed_member(ws_client_factory, monkeypatch): +def test_room_message_does_not_email_online_subscribed_member(ws_client_factory, monkeypatch): calls = _fake_smtp(monkeypatch) instance1 = ws_client_factory() instance2 = ws_client_factory() @@ -159,7 +195,7 @@ def test_room_mention_does_not_email_online_subscribed_member(ws_client_factory, assert calls == [] -def test_room_mention_email_debounced_to_first_unread_then_resets_after_read(ws_client_factory, monkeypatch): +def test_room_notifications_reset_after_read(ws_client_factory, monkeypatch): calls = _fake_smtp(monkeypatch) instance1 = ws_client_factory() instance2 = ws_client_factory() @@ -173,13 +209,7 @@ def test_room_mention_email_debounced_to_first_unread_then_resets_after_read(ws_ with instance1.websocket_connect("/ws/chat") as alice_ws: alice_ws.send_json({"type": "join", "room_id": room["id"]}) assert alice_ws.receive_json()["type"] == "joined" - - _send_and_sync(alice_ws, room["id"], f"@{bob['username']} first mention") - assert len(calls) == 1 - - # A second mention while bob still hasn't read the first -- no - # second email for the same burst. - _send_and_sync(alice_ws, room["id"], f"@{bob['username']} second mention") + _send_and_sync(alice_ws, room["id"], "message one") assert len(calls) == 1 instance2.post( @@ -191,7 +221,7 @@ def test_room_mention_email_debounced_to_first_unread_then_resets_after_read(ws_ with instance1.websocket_connect("/ws/chat") as alice_ws: alice_ws.send_json({"type": "join", "room_id": room["id"]}) assert alice_ws.receive_json()["type"] == "joined" - _send_and_sync(alice_ws, room["id"], f"@{bob['username']} third mention") + _send_and_sync(alice_ws, room["id"], "message two") assert len(calls) == 2 diff --git a/frontend/src/api/rooms.ts b/frontend/src/api/rooms.ts index cff38a5..fcab92a 100644 --- a/frontend/src/api/rooms.ts +++ b/frontend/src/api/rooms.ts @@ -68,9 +68,10 @@ export function hideDm(roomId: string): Promise { return apiFetch(`/api/rooms/${roomId}/hide`, { method: 'POST' }) } -// #67: per-viewer opt-in for email-on-mention in this room. 400s for a DM -// (backend rejects it -- see set_room_email_notifications), so callers -// should only expose the toggle for a non-DM room. +// #67: per-viewer opt-in for email on this room's first unread message +// and every mention. 400s for a DM (backend rejects it -- see +// set_room_email_notifications), so callers should only expose the +// toggle for a non-DM room. export function updateRoomNotifications(roomId: string, emailNotifications: boolean): Promise { return apiFetch(`/api/rooms/${roomId}/notifications`, { method: 'PATCH', diff --git a/frontend/src/components/RoomInfoPanel.tsx b/frontend/src/components/RoomInfoPanel.tsx index 3e757b9..4c32826 100644 --- a/frontend/src/components/RoomInfoPanel.tsx +++ b/frontend/src/components/RoomInfoPanel.tsx @@ -398,8 +398,8 @@ export function RoomInfoPanel({
- Email me on mentions - Sent only while you're offline + Email notifications + New messages and mentions, while you're offline