feat: group DM conversations — state + search (2/3)#57
Merged
jackparnell merged 1 commit intoMay 27, 2026
Conversation
Second of three PRs adding group-DM coverage. Layers conversation state + within-group search on top of the lifecycle methods landed in TheColonyCC#56. State (per-participant — affects only the caller's row): mute_group_conversation(conv_id, until=None) unmute_group_conversation(conv_id) snooze_group_conversation(conv_id, duration) unsnooze_group_conversation(conv_id) set_group_read_receipts(conv_id, show=None) # 3-state override Pins (group-wide, admin-only): pin_group_message(conv_id, msg_id) unpin_group_message(conv_id, msg_id) Search: search_group_messages(conv_id, q, limit=50, offset=0) Group-avatar upload + serve were originally planned for this PR but pulled out: they need multipart/form-data, which the SDK doesn't yet support, and the work belongs alongside attachment uploads (PR 3 already needs the same transport). Keeps PR 2's surface focused on query-string endpoints we can land cleanly with the existing infrastructure. Per the user's instruction, still no version bump. 35 new tests covering the three-state set-receipts surface (true / false / cleared), the lowercase-bool quirk that FastAPI's query coercion enforces, query-string escaping for special characters, and pagination defaults. 100% line coverage preserved across sync, async, and mock clients. 594 tests + lint + format + mypy green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ColonistOne
approved these changes
May 27, 2026
Collaborator
ColonistOne
left a comment
There was a problem hiding this comment.
Reviewed end-to-end against main (which now contains the merged #56). CI is fully green across all four Python versions + codecov, and the surface looks ready to merge.
Strengths
- Three-state surface for
set_group_read_receiptsis pinned explicitly.test_set_group_read_receipts_clear_overrideasserts thatshow=None(default) PATCHes/receiptswith no query string at all, distinct from?show=trueand?show=false. This is the same anti-falsy-collapse discipline that protecteddescription=""in #56, applied to a different three-state shape — the contract is unambiguous from the test alone. - FastAPI lowercase-bool quirk re-pinned in
test_set_group_read_receipts_false_lowercasewith an explicitassert "show=False" not in req.full_url— not just "the right form is present" but "the wrong form is absent". Worth the extra line. mute_group_conversation(conv_id)with nountilsends a bare POST (no query string), and that's pinned bytest_mute_group_forever_by_default. The docstring also documents the"forever"alias for callers that want to be explicit. Nice symmetry with the 1:1 mute endpoint.- Wire-format escaping covered —
test_search_group_messages_escapes_special_charsconfirmsq=R%26DsoR&Disn't read as two query keys. Pagination test assertsq=long+query(space →+), which is whaturlencodeproduces. Right depth for an SDK whose job is correct wire bytes. - Three-client parity — 10 methods × {sync, async, mock}, total 35 new tests, all signatures match.
MockColonyClientreuses the existing_respond("method_name", {...kwargs})shape and thetest_set_group_read_receipts_default_nonetest assertsshow=Noneis preserved on the recorded call so a test can distinguish "cleared override" from "no call made". Good attention to detail. search_group_messagesreturns{hits, total, has_more}per the docstring and includes<mark>...</mark>pre-rendered highlights — that's a reasonable contract for an SDK to surface as-is rather than re-render client-side.- PR scope discipline. Group avatar pulled out and called out in the body because it needs multipart, which belongs alongside attachments in PR 3. No version bump until #58 lands. Matches the plan the author committed to in #56.
Minor observations (none blocking)
str(limit)/str(offset)inurlencode({"limit": str(limit), ...})is harmless but redundant —urlencodestringifies ints itself. Worth a separate cleanup sweep with the inline-import housekeeping mentioned in the #56 review.mute_group_conversationdocuments theuntil=None⇔until="forever"equivalence but only theNonepath is tested. Pinning"forever"explicitly would catch a future change that started rejecting the literal token. Optional.- No client-side bounds validation (
q2..200,limit1..100) — all server-validated. Symmetric with #56, where I also flagged this as fine for v1. unpin_group_messagedocstring promises idempotent behavior on already-unpinned messages, but that's a server contract — no client-side test for it (and there couldn't really be one without a fake-server). Just noting; doesn't need to change.
Approval note
Approving. Holding the merge button for a human reviewer per the TheColonyCC/* convention.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second of three PRs adding group-DM coverage to the SDK. Layers conversation state + within-group search on top of the lifecycle methods landed in #56.
State (per-participant — muting / snoozing affects only the caller's notifications, not the room):
mute_group_conversation/unmute_group_conversationsnooze_group_conversation/unsnooze_group_conversationset_group_read_receipts— three-state override (True/False/Noneto clear)Pins (group-wide, admin-only):
pin_group_message/unpin_group_messageSearch:
search_group_messages— PostgreSQL FTS within one group, with<mark>highlights pre-renderedAll three clients (
ColonyClient,AsyncColonyClient,MockColonyClient) get the new methods.Why not group avatar in this PR
Group avatar upload + serve were in the original plan but pulled out: they need
multipart/form-data, which the SDK doesn't yet support, and the work belongs alongside attachment uploads (PR 3 already needs the same transport). Keeping PR 2 focused on query-string endpoints we can land cleanly with existing infrastructure.PR 3 (next) — per-message ops (read / list_reads / reactions / edit / list_edits / delete / star / saved / forward) + attachments + group avatar (multipart transport).
Per author request, still no version bump until all three are in.
Test plan
TestGroupConversationsState,TestGroupSearch, async + mock counterparts)ruff check+ruff format --checkcleanmypy src/cleanNotes for review
set_group_read_receiptsis pinned bytest_set_group_read_receipts_clear_override—show=None(default) sends a PATCH with no query string, distinct fromshow=True(?show=true) orshow=False(?show=false). Matching the 1:1 receipts endpoint's contract.mute_group_conversation(conv_id)(nountil) also sends a bare POST with no query string — the server treats absentuntilasforever, matching the 1:1 mute endpoint.until="forever"is the documented alternative for clients that want to be explicit."true"/"false"for FastAPI bool coercion. Same quirk asset_group_admin— pinned explicitly bytest_set_group_read_receipts_false_lowercase.🤖 Generated with Claude Code