From e33ddcb658f29a65ebc2c440f7b4bcabf553750a Mon Sep 17 00:00:00 2001 From: olen Date: Thu, 14 May 2026 16:17:09 +0200 Subject: [PATCH] fix: correct send_message and update_event return contracts (#238, #239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #238 and #239 in one PR — both bugs are in the same area (message/event chat-write paths) and the fixes are independent but small enough that one PR is reasonable. ## send_message() — #238 Three issues addressed: - Missing `await` on `_continue_chat()` (spond.py:285 in pre-fix code). Callers using the `chat_id` path were getting an unawaited coroutine object back instead of the API response, plus a RuntimeWarning at GC time. - Sentinel-dict response (`{"error": "wrong usage, ..."}`) when args were inconsistent. Replaced with `raise ValueError(...)` so the error is loud and unambiguous, and the return type annotation is finally honest. - Unreachable `return False` branch when `user_obj` is falsy. `get_person()` raises `KeyError` rather than returning falsy, so this branch was dead code. Removed; the `user_obj["profile"]["id"]` indexing is now unconditional. Docstring updated: removed the "Notes / known bugs" block (since the bugs are gone), added a `Raises` section documenting ValueError + KeyError. ## update_event() — #239 The method was assigning the POST response to `self.events_update` and then returning `self.events` (the unrelated cached events list). Two fixes: - Return `await r.json()` directly — the actual API response. - Drop the now-unused `self.events_update` attribute entirely. It was only ever set inside this method and never read elsewhere (grep across spond/, tests/, examples/ confirms zero external consumers). Return annotation tightened from `list[JSONDict] | None` to `JSONDict` (the singular updated event). ## Tests - `test_update_event__returns_api_response`: locks in that the return value is the API response, not the cached list. - `test_send_message__continues_chat_when_chat_id_given`: locks in that `_continue_chat` is properly awaited (would have caught the missing-await bug). - `test_send_message__missing_args_raises_valueerror`: locks in that the three inconsistent-arg cases raise ValueError rather than silently returning a sentinel. ## Compatibility note Both changes are technically breaking for callers depending on undocumented behaviour (the sentinel dict, the cache-list return). But the return-type annotations always claimed `JSONDict` / the correct shape, so any consumer respecting the type contract was already broken in practice. The new behaviour matches the contract. --- spond/spond.py | 62 ++++++++++++++----------------------- tests/test_spond.py | 74 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 40 deletions(-) diff --git a/spond/spond.py b/spond/spond.py index 5665888..6348291 100644 --- a/spond/spond.py +++ b/spond/spond.py @@ -395,35 +395,31 @@ async def send_message( Returns ------- JSONDict - On success: the Spond API response for the send operation. - - Notes - ----- - Has two known bugs tracked in #238: - - - When `chat_id` is provided, the call to `_continue_chat()` is - missing `await`, so the returned value is a coroutine rather than - the API response. - - When `user` and `group_uid` are inconsistent, the method returns a - sentinel dict `{"error": "..."}` rather than raising, and an - unreachable branch returns `False` despite the `JSONDict` - annotation. + The Spond API response for the send operation. + + Raises + ------ + ValueError + Neither `chat_id` nor both of `user`/`group_uid` were supplied — + the call has no way to identify the target chat. + KeyError + `user` was given but doesn't match any member or guardian in any + of the authenticated user's groups (propagated from + `get_person`). """ if self._auth is None: await self._login_chat() if chat_id is not None: - return self._continue_chat(chat_id, text) + return await self._continue_chat(chat_id, text) if group_uid is None or user is None: - return { - "error": "wrong usage, group_id and user_id needed or continue chat with chat_id" - } + raise ValueError( + "send_message requires either chat_id (to continue an existing " + "chat) or both user and group_uid (to start a new one)." + ) user_obj = await self.get_person(user) - if user_obj: - user_uid = user_obj["profile"]["id"] - else: - return False + user_uid = user_obj["profile"]["id"] url = f"{self._chat_url}/messages" data = { "text": text, @@ -571,7 +567,7 @@ async def get_event(self, uid: str) -> JSONDict: return await self._get_entity(self._EVENT, uid) @_SpondBase.require_authentication - async def update_event(self, uid: str, updates: JSONDict) -> list[JSONDict] | None: + async def update_event(self, uid: str, updates: JSONDict) -> JSONDict: """Update an existing event by merging changes into the current state. The implementation fetches the event via `_get_entity()`, copies the @@ -593,22 +589,9 @@ async def update_event(self, uid: str, updates: JSONDict) -> list[JSONDict] | No Returns ------- - list[JSONDict] or None - Currently returns `self.events` (the cached events list, or - `None` if no `get_events()` call has populated it). This is a - bug — see Notes. - - Notes - ----- - Known bug #239: the return value should be the Spond API response - from the POST, not the cached events list. The API response *is* - captured on `self.events_update`, so the data is still accessible - as a workaround until #239 is fixed: - - ```python - await s.update_event(uid, {"description": "..."}) - result = s.events_update # the actual API response - ``` + JSONDict + The Spond API response from the POST — the updated event as + persisted server-side. """ event = await self._get_entity(self._EVENT, uid) url = f"{self.api_url}sponds/{uid}" @@ -623,8 +606,7 @@ async def update_event(self, uid: str, updates: JSONDict) -> list[JSONDict] | No async with self.clientsession.post( url, json=base_event, headers=self.auth_headers ) as r: - self.events_update = await r.json() - return self.events + return await r.json() @_SpondBase.require_authentication async def get_event_attendance_xlsx(self, uid: str) -> bytes: diff --git a/tests/test_spond.py b/tests/test_spond.py index 7592c30..7ab7c7b 100644 --- a/tests/test_spond.py +++ b/tests/test_spond.py @@ -113,6 +113,32 @@ async def test_get_event__no_events_available_raises_keyerror( with pytest.raises(KeyError): await s.get_event("ID1") + @pytest.mark.asyncio + @patch("aiohttp.ClientSession.post") + async def test_update_event__returns_api_response( + self, mock_post, mock_token + ) -> None: + """`update_event()` should return the POST response, not the cached + events list (regression test for #239).""" + s = Spond(MOCK_USERNAME, MOCK_PASSWORD) + s.token = mock_token + s.events = [{"id": "ID1", "heading": "Old"}] # cached event for _get_entity + + api_response = { + "id": "ID1", + "heading": "New", + "updated": "2026-05-14T13:00:00Z", + } + mock_post.return_value.__aenter__.return_value.json = AsyncMock( + return_value=api_response + ) + + result = await s.update_event(uid="ID1", updates={"heading": "New"}) + + assert result == api_response + # The cached events list should NOT be what we returned. + assert result is not s.events + @pytest.mark.asyncio @patch("aiohttp.ClientSession.put") async def test_change_response(self, mock_put, mock_payload, mock_token) -> None: @@ -219,6 +245,54 @@ async def test_get_group__no_groups_available_raises_keyerror( await s.get_group("ID1") +class TestSendMessage: + """Tests for `Spond.send_message()` — covers the fixes in #238.""" + + @pytest.mark.asyncio + @patch("aiohttp.ClientSession.post", new_callable=AsyncMock) + async def test_send_message__continues_chat_when_chat_id_given( + self, mock_post, mock_token + ) -> None: + """With `chat_id`, the call should route through `_continue_chat()` + and properly await it (regression: the await was missing).""" + s = Spond(MOCK_USERNAME, MOCK_PASSWORD) + s.token = mock_token + s._auth = "MOCK_CHAT_AUTH" + s._chat_url = "https://chat.example.invalid" + + api_response = {"ok": True, "messageId": "MID1"} + # _continue_chat does `r = await session.post(...)` (no `async with`), + # so the post mock must be AsyncMock and r.json must be AsyncMock too. + mock_post.return_value.json = AsyncMock(return_value=api_response) + + result = await s.send_message(text="hello", chat_id="CHAT1") + + assert result == api_response # was a coroutine before the fix + mock_post.assert_called_once() + _, kwargs = mock_post.call_args + assert kwargs["json"] == {"chatId": "CHAT1", "text": "hello", "type": "TEXT"} + + @pytest.mark.asyncio + async def test_send_message__missing_args_raises_valueerror( + self, mock_token + ) -> None: + """Without `chat_id` and without both `user` and `group_uid`, the + call should raise rather than silently return a sentinel dict.""" + s = Spond(MOCK_USERNAME, MOCK_PASSWORD) + s.token = mock_token + s._auth = "MOCK_CHAT_AUTH" + s._chat_url = "https://chat.example.invalid" + + with pytest.raises(ValueError, match="chat_id"): + await s.send_message(text="hello") + + with pytest.raises(ValueError, match="user and group_uid"): + await s.send_message(text="hello", user="USER1") + + with pytest.raises(ValueError, match="user and group_uid"): + await s.send_message(text="hello", group_uid="GROUP1") + + class TestExportMethod: @pytest.mark.asyncio @patch("aiohttp.ClientSession.get")