feat: add list_messages method for reading group message history #4

Merged

forgejo_admin merged 1 commit from 3-add-list-messages into main

2026-03-26 07:00:45 +00:00

forgejo_admin commented

2026-03-26 06:56:54 +00:00

Owner

Summary

Adds list_messages() to MessagesMixin, enabling reading group message history via the GroupMe messages index endpoint. Supports pagination through before_id, since_id, and after_id parameters, with a default limit of 20 clamped to the API maximum of 100. No automatic pagination -- API parameters are exposed directly.

Changes

src/groupme_sdk/messages.py: Added list_messages(group_id, before_id=None, since_id=None, after_id=None, limit=20) method that calls GET /groups/{group_id}/messages and extracts the messages list from the response envelope
tests/test_messages.py: Added 9 unit tests in TestListMessages class covering: list return with full message data (text, sender name, created_at, attachments), all three pagination parameters, empty group response, default limit, limit clamping to 100, custom limit, and 404 error handling

Test Plan

Tests pass locally -- pytest tests/ -v -m "not integration" -- 36 passed, 0 failed
ruff check src/ tests/ -- all checks passed
ruff format --check src/ tests/ -- all files formatted
No regressions in existing send_message tests

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #3

## Summary Adds `list_messages()` to `MessagesMixin`, enabling reading group message history via the GroupMe messages index endpoint. Supports pagination through `before_id`, `since_id`, and `after_id` parameters, with a default limit of 20 clamped to the API maximum of 100. No automatic pagination -- API parameters are exposed directly. ## Changes - `src/groupme_sdk/messages.py`: Added `list_messages(group_id, before_id=None, since_id=None, after_id=None, limit=20)` method that calls `GET /groups/{group_id}/messages` and extracts the `messages` list from the response envelope - `tests/test_messages.py`: Added 9 unit tests in `TestListMessages` class covering: list return with full message data (text, sender name, created_at, attachments), all three pagination parameters, empty group response, default limit, limit clamping to 100, custom limit, and 404 error handling ## Test Plan - [x] Tests pass locally -- `pytest tests/ -v -m "not integration"` -- 36 passed, 0 failed - [x] `ruff check src/ tests/` -- all checks passed - [x] `ruff format --check src/ tests/` -- all files formatted - [x] No regressions in existing send_message tests ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #3

forgejo_admin added 1 commit

2026-03-26 06:56:54 +00:00

feat: add list_messages method for reading group message history 3eb7f50cd1

Implements GroupMe message index endpoint with pagination support via
before_id, since_id, and after_id parameters. Limit defaults to 20 and
is clamped to the API maximum of 100. Includes 9 unit tests covering
list return, all pagination params, empty group, limit clamping, and 404.

Closes #3

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-26 06:57:20 +00:00

Author

Owner

Review -- LGTM

Files reviewed: src/groupme_sdk/messages.py, tests/test_messages.py (2 files, +168/-1)

Correctness

Method signature matches issue spec: list_messages(group_id, before_id=None, since_id=None, after_id=None, limit=20)
Extracts result.get("messages", []) per the implementation note about the API envelope
Limit clamped to API max of 100 via min(limit, 100)
No automatic pagination -- exposes API parameters directly as required

Pattern Consistency

Mixin pattern with self: GroupMeClient type annotation matches groups.py and members.py
Uses _request("GET", ...) consistent with existing read methods
Docstring follows existing Args/Returns style
Tests use shared make_client, mock_response, mock_http_error fixtures from conftest

Test Coverage

9 tests covering all acceptance criteria: list return with full message data, all 3 pagination params (before_id, since_id, after_id), empty group, default limit, limit clamping to 100, custom limit, 404 error
Tests verify both return values and URL query parameter construction

Verification

ruff check -- passed
ruff format --check -- passed
pytest tests/ -v -m "not integration" -- 36 passed, 0 failed

No findings. Ready to merge.

## Review -- LGTM **Files reviewed:** `src/groupme_sdk/messages.py`, `tests/test_messages.py` (2 files, +168/-1) ### Correctness - Method signature matches issue spec: `list_messages(group_id, before_id=None, since_id=None, after_id=None, limit=20)` - Extracts `result.get("messages", [])` per the implementation note about the API envelope - Limit clamped to API max of 100 via `min(limit, 100)` - No automatic pagination -- exposes API parameters directly as required ### Pattern Consistency - Mixin pattern with `self: GroupMeClient` type annotation matches `groups.py` and `members.py` - Uses `_request("GET", ...)` consistent with existing read methods - Docstring follows existing Args/Returns style - Tests use shared `make_client`, `mock_response`, `mock_http_error` fixtures from conftest ### Test Coverage - 9 tests covering all acceptance criteria: list return with full message data, all 3 pagination params (before_id, since_id, after_id), empty group, default limit, limit clamping to 100, custom limit, 404 error - Tests verify both return values and URL query parameter construction ### Verification - `ruff check` -- passed - `ruff format --check` -- passed - `pytest tests/ -v -m "not integration"` -- 36 passed, 0 failed No findings. Ready to merge.

forgejo_admin commented

2026-03-26 06:59:07 +00:00

Author

Owner

PR #4 Review

DOMAIN REVIEW

Tech stack: Pure Python SDK, urllib-based HTTP client, pytest unit tests, ruff linting. Reviewed against Python/PEP standards and SDK design patterns.

Architecture consistency: The new list_messages() method follows the established mixin pattern (MessagesMixin with self: GroupMeClient typing), matches the docstring style of existing methods, and uses the same _request() plumbing. The response envelope extraction (result.get("messages", [])) correctly handles the GroupMe messages endpoint which nests data under a messages key (unlike most endpoints which use response -- the client's _request already unwraps that outer response envelope, so .get("messages", []) extracts the inner list). This is correct.

Query parameter construction: Parameters are built via manual f-string interpolation and "&".join(params). This is consistent with the existing list_groups in groups.py (line 38), so it follows the established pattern. The pagination IDs (before_id, since_id, after_id) are not URL-encoded via urllib.parse.quote, but GroupMe message IDs are numeric strings, so injection risk is negligible here.

Limit clamping: min(limit, 100) correctly caps at the API maximum. However, there is no floor -- limit=0 or limit=-1 would pass through unclamped. The API likely rejects these gracefully, but max(1, min(limit, 100)) would be more defensive.

Test coverage: 9 tests in TestListMessages covering: full message data assertions, all three pagination parameters (with URL inspection), empty group, default limit, limit clamping to 100, custom limit, and 404 error. This is thorough and well-structured.

BLOCKERS

None.

New functionality has test coverage (9 tests).
No unvalidated user input leading to injection risk (pagination IDs are query params to an external API, not to a database).
No secrets or credentials in code.
No duplicated auth/security logic.

NITS

Negative limit not clamped (messages.py line 56): min(limit, 100) does not protect against zero or negative values. Consider max(1, min(limit, 100)) for defensive completeness. Low risk since the GroupMe API would reject bad values, but SDK callers would get a confusing API error instead of a clear local one.
Imprecise exception assertion (test_messages.py, test_list_messages_not_found): Uses pytest.raises(Exception) rather than pytest.raises(GroupMeNotFoundError). The existing test_send_message_to_nonexistent_group has the same pattern, so this is consistent but imprecise. A future test improvement could tighten both to assert the specific exception type and status code, similar to how test_send_message_server_error asserts GroupMeHTTPError with status_code == 500.
No test for negative/zero limit behavior: Since the clamping logic only caps at 100, a test for limit=0 or limit=-1 would document the expected behavior (or catch the gap from nit #1).

SOP COMPLIANCE

Branch named after issue (3-add-list-messages references issue #3)
PR body has Summary, Changes, Test Plan, Related sections
Closes #3 links to parent issue
No secrets committed
No unnecessary file changes (2 files, both directly related)
Commit messages are descriptive
Related section does not reference a plan slug -- acceptable for a standalone SDK feature issue with no parent plan

PROCESS OBSERVATIONS

Deployment frequency: Clean incremental feature addition. SDK publishes to Forgejo PyPI on merge to main via Woodpecker CI, so this will be immediately available to downstream consumers (groupme-mcp).
Change failure risk: Low. The change is additive (new method, no modifications to existing behavior beyond a docstring tweak). The 9 new tests plus 0 regressions in existing tests provide good confidence.
Documentation: The method docstring is complete with Args/Returns documentation. README may benefit from a list_messages usage example, but that is out of scope for this PR.

VERDICT: APPROVED

## PR #4 Review ### DOMAIN REVIEW **Tech stack:** Pure Python SDK, urllib-based HTTP client, pytest unit tests, ruff linting. Reviewed against Python/PEP standards and SDK design patterns. **Architecture consistency:** The new `list_messages()` method follows the established mixin pattern (`MessagesMixin` with `self: GroupMeClient` typing), matches the docstring style of existing methods, and uses the same `_request()` plumbing. The response envelope extraction (`result.get("messages", [])`) correctly handles the GroupMe messages endpoint which nests data under a `messages` key (unlike most endpoints which use `response` -- the client's `_request` already unwraps that outer `response` envelope, so `.get("messages", [])` extracts the inner list). This is correct. **Query parameter construction:** Parameters are built via manual f-string interpolation and `"&".join(params)`. This is consistent with the existing `list_groups` in `groups.py` (line 38), so it follows the established pattern. The pagination IDs (`before_id`, `since_id`, `after_id`) are not URL-encoded via `urllib.parse.quote`, but GroupMe message IDs are numeric strings, so injection risk is negligible here. **Limit clamping:** `min(limit, 100)` correctly caps at the API maximum. However, there is no floor -- `limit=0` or `limit=-1` would pass through unclamped. The API likely rejects these gracefully, but `max(1, min(limit, 100))` would be more defensive. **Test coverage:** 9 tests in `TestListMessages` covering: full message data assertions, all three pagination parameters (with URL inspection), empty group, default limit, limit clamping to 100, custom limit, and 404 error. This is thorough and well-structured. ### BLOCKERS None. - New functionality has test coverage (9 tests). - No unvalidated user input leading to injection risk (pagination IDs are query params to an external API, not to a database). - No secrets or credentials in code. - No duplicated auth/security logic. ### NITS 1. **Negative limit not clamped** (`messages.py` line 56): `min(limit, 100)` does not protect against zero or negative values. Consider `max(1, min(limit, 100))` for defensive completeness. Low risk since the GroupMe API would reject bad values, but SDK callers would get a confusing API error instead of a clear local one. 2. **Imprecise exception assertion** (`test_messages.py`, `test_list_messages_not_found`): Uses `pytest.raises(Exception)` rather than `pytest.raises(GroupMeNotFoundError)`. The existing `test_send_message_to_nonexistent_group` has the same pattern, so this is consistent but imprecise. A future test improvement could tighten both to assert the specific exception type and status code, similar to how `test_send_message_server_error` asserts `GroupMeHTTPError` with `status_code == 500`. 3. **No test for negative/zero limit behavior**: Since the clamping logic only caps at 100, a test for `limit=0` or `limit=-1` would document the expected behavior (or catch the gap from nit #1). ### SOP COMPLIANCE - [x] Branch named after issue (`3-add-list-messages` references issue #3) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] `Closes #3` links to parent issue - [x] No secrets committed - [x] No unnecessary file changes (2 files, both directly related) - [x] Commit messages are descriptive - [ ] Related section does not reference a plan slug -- acceptable for a standalone SDK feature issue with no parent plan ### PROCESS OBSERVATIONS - **Deployment frequency:** Clean incremental feature addition. SDK publishes to Forgejo PyPI on merge to main via Woodpecker CI, so this will be immediately available to downstream consumers (groupme-mcp). - **Change failure risk:** Low. The change is additive (new method, no modifications to existing behavior beyond a docstring tweak). The 9 new tests plus 0 regressions in existing tests provide good confidence. - **Documentation:** The method docstring is complete with Args/Returns documentation. README may benefit from a `list_messages` usage example, but that is out of scope for this PR. ### VERDICT: APPROVED