feat: GroupMe data model, outbox pattern, contract welcome email #158

Merged

forgejo_admin merged 3 commits from 156-groupme-data-model-outbox into main 2026-03-24 08:06:46 +00:00

Conversation 2 Commits 3 Files changed 10 +1095 -1

forgejo_admin commented 2026-03-24 07:52:43 +00:00

Owner

Copy link

Summary

• Adds GroupMe integration to basketball-api using the outbox pattern for resilient contract signing
• Contract signing writes an event to a new outbox table in the same DB transaction, ensuring the contract always succeeds regardless of external service availability
• Async worker processes pending events to send branded welcome emails with GroupMe share links

Changes

• src/basketball_api/models.py: Add groupme_group_id and groupme_share_url columns to Team model, add contract_signed to EmailType enum, add new Outbox model
• alembic/versions/018_add_groupme_and_outbox.py: Migration for Team columns, emailtype enum value, and outbox table
• alembic/env.py: Import Outbox model for autogenerate awareness
• src/basketball_api/routes/players.py: Contract signing endpoint writes outbox event in same transaction as contract status update
• src/basketball_api/services/email.py: Add send_contract_signed_email() with GroupMe share link button (Westside branded HTML)
• src/basketball_api/services/outbox.py: Outbox worker -- polls pending events, resolves player/parent/team, calls email function, marks processed/failed
• scripts/create_groupme_groups.py: Reconciliation script -- queries GroupMe API, matches to teams by normalized name, creates missing groups, adds Lucas + Marcus
• pyproject.toml: Add groupme-sdk dependency
• tests/test_groupme.py: 14 tests covering migration, outbox event creation, worker processing, edge cases

Test Plan

• Tests pass locally -- 515/515 pass, 14 new groupme tests
• Manual verification: contract signing creates outbox event, worker processes it with mocked email
• No regressions in contract signing or email functionality
• Edge cases tested: no team (generic welcome), no GroupMe URL, email failure (event marked failed), already-processed events skipped

Review Checklist

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

Related

• Closes #156
• plan-wkq

## Summary - Adds GroupMe integration to basketball-api using the outbox pattern for resilient contract signing - Contract signing writes an event to a new outbox table in the same DB transaction, ensuring the contract always succeeds regardless of external service availability - Async worker processes pending events to send branded welcome emails with GroupMe share links ## Changes - `src/basketball_api/models.py`: Add `groupme_group_id` and `groupme_share_url` columns to Team model, add `contract_signed` to EmailType enum, add new `Outbox` model - `alembic/versions/018_add_groupme_and_outbox.py`: Migration for Team columns, emailtype enum value, and outbox table - `alembic/env.py`: Import Outbox model for autogenerate awareness - `src/basketball_api/routes/players.py`: Contract signing endpoint writes outbox event in same transaction as contract status update - `src/basketball_api/services/email.py`: Add `send_contract_signed_email()` with GroupMe share link button (Westside branded HTML) - `src/basketball_api/services/outbox.py`: Outbox worker -- polls pending events, resolves player/parent/team, calls email function, marks processed/failed - `scripts/create_groupme_groups.py`: Reconciliation script -- queries GroupMe API, matches to teams by normalized name, creates missing groups, adds Lucas + Marcus - `pyproject.toml`: Add groupme-sdk dependency - `tests/test_groupme.py`: 14 tests covering migration, outbox event creation, worker processing, edge cases ## Test Plan - [x] Tests pass locally -- 515/515 pass, 14 new groupme tests - [x] Manual verification: contract signing creates outbox event, worker processes it with mocked email - [x] No regressions in contract signing or email functionality - Edge cases tested: no team (generic welcome), no GroupMe URL, email failure (event marked failed), already-processed events skipped ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #156 - `plan-wkq`

forgejo_admin added 1 commit 2026-03-24 07:52:43 +00:00

feat: GroupMe data model, outbox pattern, and contract welcome email ... c5bd199765

Add GroupMe integration to basketball-api: teams get groupme_group_id
and groupme_share_url columns, contract signing writes an outbox event
(same transaction), and an async worker sends welcome emails with
GroupMe share links. Includes reconciliation script for matching/creating
GroupMe groups via the SDK.

Closes #156

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

forgejo_admin added 1 commit 2026-03-24 07:53:51 +00:00

fix: correct outbox worker docstring about failed event behavior ... 327f00b4c3

The docstring said failed sends stay as 'pending' for retry, but the
code actually marks them as 'failed' to prevent infinite retry loops.
Updated the docstring to match the implementation.

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

forgejo_admin commented 2026-03-24 07:54:09 +00:00

Author

Owner

Copy link

Review: PR #158

Files Reviewed (9 files, +990/-1)

File	Status
alembic/env.py	OK -- Outbox import added
alembic/versions/018_add_groupme_and_outbox.py	OK -- Migration correct, IF NOT EXISTS for enum, downgrade note
pyproject.toml	OK -- groupme-sdk dependency added
scripts/create_groupme_groups.py	OK -- Reconciliation script, normalized name matching, idempotent
src/basketball_api/models.py	OK -- Team GroupMe columns, EmailType enum, Outbox model
src/basketball_api/routes/players.py	OK -- Outbox event in same transaction as contract signing
src/basketball_api/services/email.py	OK -- send_contract_signed_email() follows existing pattern
src/basketball_api/services/outbox.py	FIXED -- Docstring corrected (said "stay pending" but code marks "failed")
tests/test_groupme.py	OK -- 14 tests, good edge case coverage

Findings

1. Fixed: Docstring inaccuracy in outbox.py -- Module docstring said "Failed sends stay as 'pending' and are retried on the next cycle" but the code marks failed events as "failed". Fixed in commit 327f00b to say "Failed sends are marked as 'failed' to prevent infinite retry loops."

2. Observation (not blocking): No worker entrypoint -- The outbox worker function process_pending_events() exists but there is no cron/scheduler/CLI entrypoint to call it. This is expected per the issue scope -- the worker invocation mechanism (cron job, FastAPI background task, or CLI command) is a follow-up concern.

Tests

• 14 new tests pass (pytest tests/ -k groupme -v)
• Full suite: 515/515 pass, no regressions
• ruff format + check: clean

VERDICT: APPROVE

## Review: PR #158 ### Files Reviewed (9 files, +990/-1) | File | Status | |------|--------| | `alembic/env.py` | OK -- Outbox import added | | `alembic/versions/018_add_groupme_and_outbox.py` | OK -- Migration correct, `IF NOT EXISTS` for enum, downgrade note | | `pyproject.toml` | OK -- groupme-sdk dependency added | | `scripts/create_groupme_groups.py` | OK -- Reconciliation script, normalized name matching, idempotent | | `src/basketball_api/models.py` | OK -- Team GroupMe columns, EmailType enum, Outbox model | | `src/basketball_api/routes/players.py` | OK -- Outbox event in same transaction as contract signing | | `src/basketball_api/services/email.py` | OK -- `send_contract_signed_email()` follows existing pattern | | `src/basketball_api/services/outbox.py` | FIXED -- Docstring corrected (said "stay pending" but code marks "failed") | | `tests/test_groupme.py` | OK -- 14 tests, good edge case coverage | ### Findings 1. **Fixed: Docstring inaccuracy in outbox.py** -- Module docstring said "Failed sends stay as 'pending' and are retried on the next cycle" but the code marks failed events as `"failed"`. Fixed in commit `327f00b` to say "Failed sends are marked as 'failed' to prevent infinite retry loops." 2. **Observation (not blocking): No worker entrypoint** -- The outbox worker function `process_pending_events()` exists but there is no cron/scheduler/CLI entrypoint to call it. This is expected per the issue scope -- the worker invocation mechanism (cron job, FastAPI background task, or CLI command) is a follow-up concern. ### Tests - 14 new tests pass (`pytest tests/ -k groupme -v`) - Full suite: 515/515 pass, no regressions - ruff format + check: clean ### VERDICT: APPROVE

forgejo_admin commented 2026-03-24 07:56:24 +00:00

Author

Owner

Copy link

PR #158 Review

DOMAIN REVIEW

Stack: Python 3.12 / FastAPI / SQLAlchemy 2.0 / Alembic / PostgreSQL

This PR adds GroupMe integration to basketball-api using a well-implemented transactional outbox pattern. The core design is sound: contract signing writes an outbox event in the same DB transaction as the contract status update, and a separate async worker processes events later. This means the contract signing endpoint is resilient to GroupMe/email service failures.

Critical design verification (per review brief):

1. Outbox event in SAME transaction as contract signing -- CONFIRMED. In /src/basketball_api/routes/players.py, the Outbox row is added via db.add(outbox_event) before the single db.commit() at line 318. No separate commit. The test test_outbox_event_in_same_transaction explicitly verifies this atomicity.

2. Email function follows existing pattern -- CONFIRMED. send_contract_signed_email() follows the same structure as send_password_reset_email() and other email functions: uses get_gmail_client(tenant), builds branded HTML with _brand_wrapper(), includes plain-text fallback, logs to EmailLog, catches exceptions and returns None on failure. All user-supplied values are escaped via html.escape().

3. Migration is reversible -- PARTIALLY. The downgrade() drops the outbox table and removes the GroupMe columns, which is correct. The comment correctly notes that PostgreSQL cannot remove enum values, so contract_signed persists after downgrade. This is a known PostgreSQL limitation and the comment documents it properly.

4. Contract signing works if GroupMe/email is down -- CONFIRMED. The outbox pattern decouples signing from notification. The endpoint only writes a DB row; the worker (invoked separately) handles email delivery. If email fails, the event is marked failed and the contract remains signed.

5. Reconciliation script idempotent -- CONFIRMED. The script skips teams that already have groupme_group_id set (line 105: if team.groupme_group_id: ... skipped += 1; continue). Re-running won't duplicate groups.

6. No secrets committed -- CONFIRMED. GroupMe user IDs (public identifiers) are hardcoded in the reconciliation script, which is acceptable. The GROUPME_ACCESS_TOKEN is read from env vars. No API keys, passwords, or tokens in code.

7. groupme-sdk dependency -- CONFIRMED. Added to pyproject.toml as groupme-sdk>=0.1.0. The reconciliation script does a graceful ImportError check with installation instructions pointing to the Forgejo PyPI registry.

PEP compliance: Type hints present on all function signatures. Docstrings on all public functions. Code follows the existing project patterns.

SQLAlchemy patterns: The Outbox model uses proper Mapped[] annotations consistent with the rest of the codebase. The tenant relationship is defined. Session management follows existing patterns (single commit per request).

OWASP/Input validation: All HTML template interpolation uses html.escape() for user-controlled values (parent_name, player_name, team_name, groupme_share_url). The outbox payload uses json.dumps()/json.loads() for safe serialization. The contract signing endpoint already validates auth and input via Pydantic ContractSignRequest.

BLOCKERS

None.

NITS

1. Outbox worker invocation not shown: The process_pending_events() function exists but there is no CLI entry point, cron job registration, or route that calls it. The PR description says "async worker" but the mechanism to trigger it is not in this diff. This is likely intentional scope deferral, but worth confirming the invocation mechanism is tracked.

2. Outbox status as string, not enum: The status field uses String(20) with literal string comparisons ("pending", "processed", "failed"). The rest of the codebase uses Python enums for status fields (e.g., ContractStatus, PaymentStatus, OrderStatus). Consider an OutboxStatus enum for type safety and consistency. Non-blocking since the values are well-defined and consistently used.

3. No retry mechanism for failed events: Events that fail are marked "failed" permanently. There is no retry logic, dead-letter queue, or mechanism to re-process failed events. For an initial implementation this is fine (manual DB update can re-set to "pending"), but worth noting for future iteration.

4. _mock_admin_user defined but unused in tests: The _mock_admin_user() factory in test_groupme.py (line 33) is never called in any test. Minor dead code.

5. Reconciliation script sys.path.insert: The script uses sys.path.insert(0, ...) to find the basketball_api package. This works but is fragile. Consider documenting that the script should be run from the project root with the package installed (pip install -e .).

SOP COMPLIANCE

• Branch named after issue: 156-groupme-data-model-outbox matches issue #156
• PR body has: Summary, Changes, Test Plan, Related sections
• Related references plan slug: plan-wkq referenced
• Related references issue: Closes #156
• No secrets committed: GroupMe user IDs are public identifiers, not secrets. Token read from env var
• No unnecessary file changes: All 9 changed files are directly related to the feature
• Commit messages are descriptive (PR title follows convention)
• Tests exist: 14 new tests covering migration, outbox event creation, worker processing, edge cases (no team, no GroupMe URL, email failure, already-processed events)

PROCESS OBSERVATIONS

• Deployment frequency: Clean feature PR, well-scoped. Ready for merge.
• Change failure risk: Low. The outbox pattern is additive -- it adds a new table and columns but does not modify existing contract signing behavior beyond appending an event write to the existing transaction. Existing tests (501 prior) would catch regressions.
• Test coverage: 14 new tests is strong for this feature. All key edge cases are covered: with/without team, with/without GroupMe URL, email failure, idempotency of processed events, same-transaction atomicity.
• Documentation gap: The outbox worker trigger mechanism (cron? admin endpoint? CLI?) is not in this PR. Confirm this is tracked as a follow-up.

VERDICT: APPROVED

## PR #158 Review ### DOMAIN REVIEW **Stack**: Python 3.12 / FastAPI / SQLAlchemy 2.0 / Alembic / PostgreSQL This PR adds GroupMe integration to basketball-api using a well-implemented transactional outbox pattern. The core design is sound: contract signing writes an outbox event in the same DB transaction as the contract status update, and a separate async worker processes events later. This means the contract signing endpoint is resilient to GroupMe/email service failures. **Critical design verification (per review brief):** 1. **Outbox event in SAME transaction as contract signing** -- CONFIRMED. In `/src/basketball_api/routes/players.py`, the `Outbox` row is added via `db.add(outbox_event)` before the single `db.commit()` at line 318. No separate commit. The test `test_outbox_event_in_same_transaction` explicitly verifies this atomicity. 2. **Email function follows existing pattern** -- CONFIRMED. `send_contract_signed_email()` follows the same structure as `send_password_reset_email()` and other email functions: uses `get_gmail_client(tenant)`, builds branded HTML with `_brand_wrapper()`, includes plain-text fallback, logs to `EmailLog`, catches exceptions and returns `None` on failure. All user-supplied values are escaped via `html.escape()`. 3. **Migration is reversible** -- PARTIALLY. The `downgrade()` drops the outbox table and removes the GroupMe columns, which is correct. The comment correctly notes that PostgreSQL cannot remove enum values, so `contract_signed` persists after downgrade. This is a known PostgreSQL limitation and the comment documents it properly. 4. **Contract signing works if GroupMe/email is down** -- CONFIRMED. The outbox pattern decouples signing from notification. The endpoint only writes a DB row; the worker (invoked separately) handles email delivery. If email fails, the event is marked `failed` and the contract remains signed. 5. **Reconciliation script idempotent** -- CONFIRMED. The script skips teams that already have `groupme_group_id` set (line 105: `if team.groupme_group_id: ... skipped += 1; continue`). Re-running won't duplicate groups. 6. **No secrets committed** -- CONFIRMED. GroupMe user IDs (public identifiers) are hardcoded in the reconciliation script, which is acceptable. The `GROUPME_ACCESS_TOKEN` is read from env vars. No API keys, passwords, or tokens in code. 7. **groupme-sdk dependency** -- CONFIRMED. Added to `pyproject.toml` as `groupme-sdk>=0.1.0`. The reconciliation script does a graceful `ImportError` check with installation instructions pointing to the Forgejo PyPI registry. **PEP compliance**: Type hints present on all function signatures. Docstrings on all public functions. Code follows the existing project patterns. **SQLAlchemy patterns**: The `Outbox` model uses proper `Mapped[]` annotations consistent with the rest of the codebase. The `tenant` relationship is defined. Session management follows existing patterns (single commit per request). **OWASP/Input validation**: All HTML template interpolation uses `html.escape()` for user-controlled values (`parent_name`, `player_name`, `team_name`, `groupme_share_url`). The outbox payload uses `json.dumps()`/`json.loads()` for safe serialization. The contract signing endpoint already validates auth and input via Pydantic `ContractSignRequest`. ### BLOCKERS None. ### NITS 1. **Outbox worker invocation not shown**: The `process_pending_events()` function exists but there is no CLI entry point, cron job registration, or route that calls it. The PR description says "async worker" but the mechanism to trigger it is not in this diff. This is likely intentional scope deferral, but worth confirming the invocation mechanism is tracked. 2. **Outbox status as string, not enum**: The `status` field uses `String(20)` with literal string comparisons (`"pending"`, `"processed"`, `"failed"`). The rest of the codebase uses Python enums for status fields (e.g., `ContractStatus`, `PaymentStatus`, `OrderStatus`). Consider an `OutboxStatus` enum for type safety and consistency. Non-blocking since the values are well-defined and consistently used. 3. **No retry mechanism for failed events**: Events that fail are marked `"failed"` permanently. There is no retry logic, dead-letter queue, or mechanism to re-process failed events. For an initial implementation this is fine (manual DB update can re-set to `"pending"`), but worth noting for future iteration. 4. **`_mock_admin_user` defined but unused in tests**: The `_mock_admin_user()` factory in `test_groupme.py` (line 33) is never called in any test. Minor dead code. 5. **Reconciliation script `sys.path.insert`**: The script uses `sys.path.insert(0, ...)` to find the `basketball_api` package. This works but is fragile. Consider documenting that the script should be run from the project root with the package installed (`pip install -e .`). ### SOP COMPLIANCE - [x] Branch named after issue: `156-groupme-data-model-outbox` matches issue #156 - [x] PR body has: Summary, Changes, Test Plan, Related sections - [x] Related references plan slug: `plan-wkq` referenced - [x] Related references issue: `Closes #156` - [x] No secrets committed: GroupMe user IDs are public identifiers, not secrets. Token read from env var - [x] No unnecessary file changes: All 9 changed files are directly related to the feature - [x] Commit messages are descriptive (PR title follows convention) - [x] Tests exist: 14 new tests covering migration, outbox event creation, worker processing, edge cases (no team, no GroupMe URL, email failure, already-processed events) ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean feature PR, well-scoped. Ready for merge. - **Change failure risk**: Low. The outbox pattern is additive -- it adds a new table and columns but does not modify existing contract signing behavior beyond appending an event write to the existing transaction. Existing tests (501 prior) would catch regressions. - **Test coverage**: 14 new tests is strong for this feature. All key edge cases are covered: with/without team, with/without GroupMe URL, email failure, idempotency of processed events, same-transaction atomicity. - **Documentation gap**: The outbox worker trigger mechanism (cron? admin endpoint? CLI?) is not in this PR. Confirm this is tracked as a follow-up. ### VERDICT: APPROVED

forgejo_admin added 1 commit 2026-03-24 08:00:47 +00:00

feat: add POST /admin/process-outbox endpoint for cron and test triggers ... c9f084df08

Exposes outbox processing as an admin-only HTTP endpoint so cron jobs
and Playwright tests can trigger event processing without a background
worker. Returns the count of processed events.

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

forgejo_admin merged commit 0a0737a925 into main 2026-03-24 08:06:46 +00:00

forgejo_admin deleted branch 156-groupme-data-model-outbox 2026-03-24 08:06:46 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-24 08:06:47 +00:00

feat: GroupMe data model, outbox pattern, contract welcome email (#158)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/basketball-api!158

No description provided.