diff --git a/src/application/abstractions/i_chat_sender.py b/src/application/abstractions/i_chat_sender.py index 4abff6d..f5b42d4 100644 --- a/src/application/abstractions/i_chat_sender.py +++ b/src/application/abstractions/i_chat_sender.py @@ -10,6 +10,9 @@ class IChatSender(ABC): SELF_CHAT: str + @abstractmethod + def chat_members(self, note: IGraphClient, chat_id: str) -> list[dict]: ... + @abstractmethod def validate(self, msg: OutgoingMessage) -> OutgoingMessage: ... diff --git a/src/application/commands/schedule_chat_meeting_command.py b/src/application/commands/schedule_chat_meeting_command.py index 94f3435..dfc74a2 100644 --- a/src/application/commands/schedule_chat_meeting_command.py +++ b/src/application/commands/schedule_chat_meeting_command.py @@ -37,7 +37,7 @@ async def handle(self, command: ScheduleChatMeetingCommand) -> dict: raise ServiceError(400, "end must be after start.") def create(note, broker): - members = [] if command.chat_id == self._sender.SELF_CHAT else note.list(f"chats/{command.chat_id}/members") + members = [] if command.chat_id == self._sender.SELF_CHAT else self._sender.chat_members(note, command.chat_id) me = self._profile.get(note, broker) mine = {(me.get("mail") or "").lower(), (me.get("userPrincipalName") or "").lower()} attendees = [{"emailAddress": {"address": m["email"], "name": m.get("displayName") or m["email"]}, "type": "required"} for m in members if m.get("email") and m["email"].lower() not in mine] diff --git a/src/infrastructure/teams/chat_sender.py b/src/infrastructure/teams/chat_sender.py index 2babdef..2e8e473 100644 --- a/src/infrastructure/teams/chat_sender.py +++ b/src/infrastructure/teams/chat_sender.py @@ -8,9 +8,11 @@ """ import base64 +import logging import re from urllib.parse import quote from entrance import http_client +from outnote import GraphError from src.application.abstractions.i_chat_sender import IChatSender from src.application.abstractions.i_emoji_catalogue import IEmojiCatalogue @@ -22,6 +24,7 @@ from src.domain.chat.upload import Upload from src.domain.errors.emoji_error import EmojiError from src.domain.errors.send_error import SendError +from src.infrastructure.logging_config import LoggerNames class ChatSender(IChatSender): @@ -40,7 +43,9 @@ class ChatSender(IChatSender): IMPORTANCE = ("normal", "high", "urgent") GUID_IN_ETAG = re.compile(r"\{([0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12})\}") USER_ID = re.compile(r"[A-Za-z0-9@._-]{1,200}") + EXPANDED_MEMBERS_CEILING = 20 # at this many, Graph may have cut the expanded list short http_client = staticmethod(http_client) + log = logging.getLogger(LoggerNames.SERVICE) def __init__(self, html: ChatHtmlSanitizer, emojis: IEmojiCatalogue, profile: IProfileReader): self._html = html @@ -165,12 +170,25 @@ def upload_file(self, note, upload: Upload) -> dict: raise SendError(502, "OneDrive did not confirm the upload.") return item + def chat_members(self, note, chat_id: str) -> list[dict]: + """Graph throttles `chats/{id}/members` on its own while the same chat with its members expanded answers, so a long list is the only reason to ask the throttled route.""" + members = note.get(f"chats/{chat_id}", {"$expand": "members"}).get("members") or [] + if len(members) < self.EXPANDED_MEMBERS_CEILING: + return members + try: + return note.list(f"chats/{chat_id}/members") + except GraphError as ex: + if ex.status != 429: + raise + self.log.warning("Chat members route throttled; using the %d expanded members, which may be incomplete", len(members)) + return members + def chat_member_ids(self, note, broker, chat_id: str) -> list[str]: if chat_id == self.SELF_CHAT: return [] me = self._profile.get(note, broker).get("id") ids = [] - for m in note.list(f"chats/{chat_id}/members"): + for m in self.chat_members(note, chat_id): uid = m.get("userId") if isinstance(uid, str) and uid != me and uid not in ids: ids.append(uid) diff --git a/tests/test_teams_extras.py b/tests/test_teams_extras.py index 322f621..d14b078 100644 --- a/tests/test_teams_extras.py +++ b/tests/test_teams_extras.py @@ -29,6 +29,8 @@ def __call__(self, client_id, scope=None): class Note: def __init__(self): self.calls = [] + self.members_throttled = False + self.expanded = None self.messages = [] self.members = [{"userId": "me-id", "email": "me@school.ch"}, {"userId": "u2", "email": "bo@school.ch", "displayName": "Bo"}] @@ -53,6 +55,8 @@ def batch(self, requests): def list(self, path, params=None, limit=None): self.calls.append(("list", path)) if path.endswith("/members"): + if self.members_throttled: + raise GraphError(429, "throttled", retry_after=62) return self.members if path == "me/chats": return [{"id": "19:g@thread.v2", "chatType": "group"}, {"id": "19:o@unq.gbl.spaces", "chatType": "oneOnOne"}] @@ -60,6 +64,8 @@ def list(self, path, params=None, limit=None): def get(self, path, params=None, raw=False): self.calls.append(("get", path, raw)) + if params == {"$expand": "members"}: + return {"members": self.expanded if self.expanded is not None else self.members} if path == "me": return {"id": "me-id", "mail": "me@school.ch"} if "hostedContents" in path: @@ -343,6 +349,39 @@ def test_meeting_invites_the_chat_members_and_returns_the_join_url(env): assert [a["emailAddress"]["address"] for a in event["attendees"]] == ["bo@school.ch"] +def _send_file(note): + data = {"content": "hi", "contentType": "text"} + return TestClient(app).post("/api/chats/19:g@thread.v2/messages", headers=H, data=data, files=[("files", ("a.png", PNG, "image/png"))]) + + +def _invited(note): + return [r["objectId"] for r in next(c for c in note.calls if c[0] == "post" and c[1].endswith("/invite"))[2]["recipients"]] + + +def test_a_file_send_survives_a_throttled_members_route(env): + note, _ = env + note.members_throttled = True + assert _send_file(note).status_code < 300 + assert _invited(note) == ["u2"] + assert not any(c[:2] == ("list", "chats/19:g@thread.v2/members") for c in note.calls) + + +def test_a_long_expanded_list_asks_the_members_route_for_everyone(env): + note, _ = env + note.expanded = [{"userId": f"e{i}"} for i in range(20)] + note.members = [{"userId": f"e{i}"} for i in range(30)] + assert _send_file(note).status_code < 300 + assert len(_invited(note)) == 30 + + +def test_a_long_expanded_list_is_used_when_the_members_route_is_throttled(env): + note, _ = env + note.members_throttled = True + note.expanded = [{"userId": f"e{i}"} for i in range(20)] + assert _send_file(note).status_code < 300 + assert len(_invited(note)) == 20 + + def test_gif_search_needs_a_giphy_key(env, monkeypatch): monkeypatch.delenv("GIPHY_API_KEY", raising=False) r = TestClient(app).get("/api/gifs/search", headers=H, params={"q": "cat"})