feat: payment recovery for abandoned Stripe registrations #397

Merged

forgejo_admin merged 2 commits from 389-payment-recovery-abandoned-stripe into main 2026-04-08 21:45:49 +00:00

Conversation 2 Commits 2 Files changed 5 +554 -2

forgejo_admin commented 2026-04-08 21:31:44 +00:00

Owner

Copy link

Summary

Add an admin-triggered endpoint that sends payment recovery emails to parents who started Stripe registration but abandoned the checkout. Creates fresh Stripe checkout sessions so parents can complete payment without re-filling the form. Cash registrations are excluded. A recovery_email_sent column enforces the send-once constraint at the data layer.

Changes

• src/basketball_api/models.py — Added recovery_email_sent boolean column to Registration model; added payment_recovery to EmailType enum
• src/basketball_api/services/email.py — New send_payment_recovery_email() function using MJML template system with plaintext fallback
• src/basketball_api/routes/admin.py — New POST /admin/send-payment-recovery endpoint following existing outbox pattern (admin-triggered, not cron). Filters on signup_method="stripe", payment_status=pending, recovery_email_sent=False, and created_at older than configurable threshold (default 2h). Supports test_email param for safe testing.
• alembic/versions/039_add_recovery_email_sent.py — Migration adding the column and enum value
• tests/test_payment_recovery.py — 4 unit tests covering all acceptance criteria

Test Plan

• pytest tests/test_payment_recovery.py -v — all 4 tests pass
  • Recovery email sent for PENDING Stripe registration older than threshold
  • Recovery email NOT sent for already-paid registration
  • Recovery email NOT sent twice (column enforces exclusion)
  • Recovery email NOT sent for cash PENDING registration
• Deploy and run POST /admin/send-payment-recovery?test_email=draneylucas@gmail.com to verify end-to-end

Review Checklist

• ruff format and ruff check pass
• All 4 tests pass
• No unrelated changes
• Migration follows existing numbered convention (039)
• Endpoint follows existing admin pattern (require_admin, DEFAULT_TENANT_SLUG)

Related Notes

• Depends on #390 (merged — registration_token generation fix)

Related

Closes #389

## Summary Add an admin-triggered endpoint that sends payment recovery emails to parents who started Stripe registration but abandoned the checkout. Creates fresh Stripe checkout sessions so parents can complete payment without re-filling the form. Cash registrations are excluded. A `recovery_email_sent` column enforces the send-once constraint at the data layer. ## Changes - `src/basketball_api/models.py` — Added `recovery_email_sent` boolean column to Registration model; added `payment_recovery` to EmailType enum - `src/basketball_api/services/email.py` — New `send_payment_recovery_email()` function using MJML template system with plaintext fallback - `src/basketball_api/routes/admin.py` — New `POST /admin/send-payment-recovery` endpoint following existing outbox pattern (admin-triggered, not cron). Filters on `signup_method="stripe"`, `payment_status=pending`, `recovery_email_sent=False`, and `created_at` older than configurable threshold (default 2h). Supports `test_email` param for safe testing. - `alembic/versions/039_add_recovery_email_sent.py` — Migration adding the column and enum value - `tests/test_payment_recovery.py` — 4 unit tests covering all acceptance criteria ## Test Plan - `pytest tests/test_payment_recovery.py -v` — all 4 tests pass - Recovery email sent for PENDING Stripe registration older than threshold - Recovery email NOT sent for already-paid registration - Recovery email NOT sent twice (column enforces exclusion) - Recovery email NOT sent for cash PENDING registration - Deploy and run `POST /admin/send-payment-recovery?test_email=draneylucas@gmail.com` to verify end-to-end ## Review Checklist - [x] ruff format and ruff check pass - [x] All 4 tests pass - [x] No unrelated changes - [x] Migration follows existing numbered convention (039) - [x] Endpoint follows existing admin pattern (require_admin, DEFAULT_TENANT_SLUG) ## Related Notes - Depends on #390 (merged — registration_token generation fix) ## Related Closes #389

forgejo_admin added 1 commit 2026-04-08 21:31:44 +00:00

feat: add payment recovery emails for abandoned Stripe registrations ... 257d2ee2bd

Add admin endpoint POST /admin/send-payment-recovery that finds PENDING
Stripe registrations older than a configurable threshold, creates fresh
Stripe checkout sessions, and sends recovery emails so parents can
complete payment without re-filling the form. Cash registrations are
excluded. A recovery_email_sent column on registrations enforces the
send-once constraint at the data layer.

Closes #389

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

forgejo_admin commented 2026-04-08 21:32:31 +00:00

Author

Owner

Copy link

QA Review — PR #397

Files Reviewed

• alembic/versions/039_add_recovery_email_sent.py
• src/basketball_api/models.py
• src/basketball_api/services/email.py
• src/basketball_api/routes/admin.py
• tests/test_payment_recovery.py

Findings

1. Double join in test_email filter path (minor)
admin.py line ~1870: When test_email is provided, the query does .join(Registration.player).join(Player.parent) after already using .options(joinedload(Registration.player).joinedload(Player.parent)). SQLAlchemy's joinedload adds a LEFT OUTER JOIN implicitly, and the explicit .join() adds another. For single-parent test_email usage this is unlikely to cause issues in practice, but it is technically redundant and could produce duplicate rows on more complex data. Consider using contains_eager instead of joinedload when the explicit join path is also present, or restructure so joinedload is only applied when test_email is not set.

2. All acceptance criteria covered

• PENDING Stripe > 2h gets recovery email: YES (query filter + test)
• Paid registrations excluded: YES (payment_status filter + test)
• Recovery email only sent once: YES (recovery_email_sent column + test)
• Cash registrations excluded: YES (signup_method filter + test)

3. Pattern compliance

• Migration follows existing numbered convention (039, depends on 038)
• Endpoint follows existing admin pattern (require_admin, DEFAULT_TENANT_SLUG, test_email param)
• Email function follows established pattern (get_gmail_client, plaintext + HTML fallback, EmailLog, commit)
• Stripe checkout session creation mirrors original registration flow metadata exactly

4. No unrelated changes confirmed — diff is scoped to the 5 files listed.

VERDICT: APPROVE with nit

Finding #1 is a minor nit that does not block merge. The core logic, data model, migration, and test coverage are all correct.

## QA Review — PR #397 ### Files Reviewed - `alembic/versions/039_add_recovery_email_sent.py` - `src/basketball_api/models.py` - `src/basketball_api/services/email.py` - `src/basketball_api/routes/admin.py` - `tests/test_payment_recovery.py` ### Findings **1. Double join in test_email filter path (minor)** `admin.py` line ~1870: When `test_email` is provided, the query does `.join(Registration.player).join(Player.parent)` after already using `.options(joinedload(Registration.player).joinedload(Player.parent))`. SQLAlchemy's `joinedload` adds a LEFT OUTER JOIN implicitly, and the explicit `.join()` adds another. For single-parent `test_email` usage this is unlikely to cause issues in practice, but it is technically redundant and could produce duplicate rows on more complex data. Consider using `contains_eager` instead of `joinedload` when the explicit join path is also present, or restructure so `joinedload` is only applied when `test_email` is not set. **2. All acceptance criteria covered** - PENDING Stripe > 2h gets recovery email: YES (query filter + test) - Paid registrations excluded: YES (payment_status filter + test) - Recovery email only sent once: YES (recovery_email_sent column + test) - Cash registrations excluded: YES (signup_method filter + test) **3. Pattern compliance** - Migration follows existing numbered convention (039, depends on 038) - Endpoint follows existing admin pattern (require_admin, DEFAULT_TENANT_SLUG, test_email param) - Email function follows established pattern (get_gmail_client, plaintext + HTML fallback, EmailLog, commit) - Stripe checkout session creation mirrors original registration flow metadata exactly **4. No unrelated changes** confirmed — diff is scoped to the 5 files listed. ### VERDICT: APPROVE with nit Finding #1 is a minor nit that does not block merge. The core logic, data model, migration, and test coverage are all correct.

forgejo_admin added 1 commit 2026-04-08 21:33:07 +00:00

fix: use contains_eager to avoid double join in test_email path ... e1a7344492

Addresses QA review nit: when test_email is provided, the query now
uses explicit joins with contains_eager instead of layering joinedload
on top of explicit joins, preventing potential duplicate rows.

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

forgejo_admin commented 2026-04-08 21:35:17 +00:00

Author

Owner

Copy link

PR #397 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Stripe SDK / Gmail OAuth

Migration (039_add_recovery_email_sent.py)

• Correct: server_default=sa.text("false"), nullable=False, IF NOT EXISTS on enum ADD VALUE.
• Downgrade correctly notes PostgreSQL cannot remove enum values. Clean.

Model (models.py)

• payment_recovery added to EmailType enum -- consistent with existing pattern.
• recovery_email_sent column mirrors the existing confirmation_email_sent pattern exactly. Good.

Route (admin.py)

• Follows existing admin endpoint pattern: require_admin dependency, DEFAULT_TENANT_SLUG tenant lookup, response model.
• contains_eager vs joinedload split for the test_email filter path is correct SQLAlchemy -- avoids the double-join problem. Good awareness.
• Stripe checkout session creation mirrors the pattern in register.py (same metadata keys, same success_url/cancel_url structure).
• import stripe is inside the function body. Other files in this codebase (register.py, jersey.py, checkout.py) import stripe at module level. This is a minor inconsistency but not blocking -- it works and avoids loading stripe when the endpoint is not called.
• datetime.utcnow() is used for the cutoff. The rest of the codebase uses datetime.now(timezone.utc) in newer code (e.g., register.py:912, players.py:303, password_reset.py:87). utcnow() is deprecated since Python 3.12. Since created_at uses func.now() (server time, naive UTC), the comparison still works correctly, but this should be modernized.

Email service (email.py)

• Properly HTML-escapes user-supplied data (escape(parent.name), escape(player.name), etc.) before passing to template. Good.
• checkout_url is NOT escaped -- this is correct because it is a URL that needs to be used as-is in href attributes. The URL comes from Stripe SDK, not user input.
• FileNotFoundError catch with plaintext fallback is a nice defensive pattern.
• db.commit() at the end sets recovery_email_sent = True atomically with the email log insert. However, if the commit fails after the email was already sent via Gmail, the email goes out but the flag is not set, meaning a retry would send a duplicate. This is a known pattern in this codebase (same risk exists in other email functions) -- not a new regression.
• EmailLog entry follows existing pattern exactly.

Tests (test_payment_recovery.py)

• 4 tests covering: happy path send, paid exclusion, send-once enforcement, cash exclusion.
• Tests 2 and 4 verify the query filter logic (that paid/cash registrations would not be selected), which is the right layer to test since the endpoint owns the filtering.
• Test fixtures create realistic data with proper parent/player/registration chains.
• autouse=True on _seed_tenant is consistent with other test files in this repo.

BLOCKERS

None.

• Tests exist for all new functionality (4 tests, happy + edge cases). Not a blocker.
• No unvalidated user input. test_email and hours_threshold are query params used in DB filters, not interpolated into SQL. SQLAlchemy parameterizes these. Not a blocker.
• No secrets or credentials in code. Stripe API key comes from config/env. Not a blocker.
• No DRY violation in auth paths -- reuses existing require_admin dependency. Not a blocker.

NITS

1. datetime.utcnow() is deprecated (Python 3.12+). The newer codebase pattern is datetime.now(timezone.utc) (see register.py:912, players.py:303). Should be datetime.now(timezone.utc).replace(tzinfo=None) to stay compatible with the naive-UTC created_at column, matching the pattern in password_reset.py:87. Low risk since the comparison works today.

2. Inline import stripe inside the function body. Every other file in this codebase does import stripe at module level. Minor inconsistency. Similarly, from basketball_api.services.email import send_payment_recovery_email is imported inside the function rather than at module top with the other email imports (line 30-34 of admin.py).

3. Missing MJML template file. The code calls load_email_template("payment-recovery", ...) but no payment-recovery.html template exists in the repo. The FileNotFoundError fallback to plaintext will fire every time. If this is intentional (template to be added later), a comment in the code or a note in the PR body would clarify intent.

4. Hardcoded fallback amounts (3000 for tryout, 4000 otherwise) at line ~1890 of admin.py. These magic numbers duplicate pricing logic. If pricing changes, this is a second place to update. Consider extracting to a constant or reading from the tenant/product config.

5. No integration test for the endpoint itself. The 4 tests cover the email service function and query filter logic (unit level), but there is no test that hits POST /admin/send-payment-recovery via the test client to verify the full request path (auth, Stripe mock, response shape). This is consistent with how some other admin endpoints are tested in this codebase, so not a blocker, but worth noting.

SOP COMPLIANCE

• Branch named after issue: 389-payment-recovery-abandoned-stripe follows {issue-number}-{kebab-case-purpose}
• PR body has Summary, Changes, Test Plan, Related sections
• Related section references issue #389 with Closes #389
• No secrets committed
• No unrelated file changes (5 files, all scoped to the feature)
• Migration follows existing numbered convention (039)
• PR body missing plan slug reference in Related section -- however, this may be a standalone board item without a plan, which is acceptable per conventions

PROCESS OBSERVATIONS

• Clean, well-scoped PR. 554 additions across 5 files, all directly related to the feature.
• The dependency on #390 (registration_token fix) is noted in the PR body. Good traceability.
• The test_email query parameter for safe testing before real sends is a good operational pattern.
• Deployment risk is low: new endpoint (no existing behavior changed), additive migration (new column + enum value), admin-only access.

VERDICT: APPROVED

## PR #397 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic / Stripe SDK / Gmail OAuth **Migration (`039_add_recovery_email_sent.py`)** - Correct: `server_default=sa.text("false")`, `nullable=False`, `IF NOT EXISTS` on enum ADD VALUE. - Downgrade correctly notes PostgreSQL cannot remove enum values. Clean. **Model (`models.py`)** - `payment_recovery` added to `EmailType` enum -- consistent with existing pattern. - `recovery_email_sent` column mirrors the existing `confirmation_email_sent` pattern exactly. Good. **Route (`admin.py`)** - Follows existing admin endpoint pattern: `require_admin` dependency, `DEFAULT_TENANT_SLUG` tenant lookup, response model. - `contains_eager` vs `joinedload` split for the `test_email` filter path is correct SQLAlchemy -- avoids the double-join problem. Good awareness. - Stripe checkout session creation mirrors the pattern in `register.py` (same metadata keys, same `success_url`/`cancel_url` structure). - `import stripe` is inside the function body. Other files in this codebase (`register.py`, `jersey.py`, `checkout.py`) import stripe at module level. This is a minor inconsistency but not blocking -- it works and avoids loading stripe when the endpoint is not called. - `datetime.utcnow()` is used for the cutoff. The rest of the codebase uses `datetime.now(timezone.utc)` in newer code (e.g., `register.py:912`, `players.py:303`, `password_reset.py:87`). `utcnow()` is deprecated since Python 3.12. Since `created_at` uses `func.now()` (server time, naive UTC), the comparison still works correctly, but this should be modernized. **Email service (`email.py`)** - Properly HTML-escapes user-supplied data (`escape(parent.name)`, `escape(player.name)`, etc.) before passing to template. Good. - `checkout_url` is NOT escaped -- this is correct because it is a URL that needs to be used as-is in `href` attributes. The URL comes from Stripe SDK, not user input. - `FileNotFoundError` catch with plaintext fallback is a nice defensive pattern. - `db.commit()` at the end sets `recovery_email_sent = True` atomically with the email log insert. However, if the commit fails after the email was already sent via Gmail, the email goes out but the flag is not set, meaning a retry would send a duplicate. This is a known pattern in this codebase (same risk exists in other email functions) -- not a new regression. - EmailLog entry follows existing pattern exactly. **Tests (`test_payment_recovery.py`)** - 4 tests covering: happy path send, paid exclusion, send-once enforcement, cash exclusion. - Tests 2 and 4 verify the query filter logic (that paid/cash registrations would not be selected), which is the right layer to test since the endpoint owns the filtering. - Test fixtures create realistic data with proper parent/player/registration chains. - `autouse=True` on `_seed_tenant` is consistent with other test files in this repo. ### BLOCKERS None. - Tests exist for all new functionality (4 tests, happy + edge cases). Not a blocker. - No unvalidated user input. `test_email` and `hours_threshold` are query params used in DB filters, not interpolated into SQL. SQLAlchemy parameterizes these. Not a blocker. - No secrets or credentials in code. Stripe API key comes from config/env. Not a blocker. - No DRY violation in auth paths -- reuses existing `require_admin` dependency. Not a blocker. ### NITS 1. **`datetime.utcnow()` is deprecated** (Python 3.12+). The newer codebase pattern is `datetime.now(timezone.utc)` (see `register.py:912`, `players.py:303`). Should be `datetime.now(timezone.utc).replace(tzinfo=None)` to stay compatible with the naive-UTC `created_at` column, matching the pattern in `password_reset.py:87`. Low risk since the comparison works today. 2. **Inline `import stripe`** inside the function body. Every other file in this codebase does `import stripe` at module level. Minor inconsistency. Similarly, `from basketball_api.services.email import send_payment_recovery_email` is imported inside the function rather than at module top with the other email imports (line 30-34 of `admin.py`). 3. **Missing MJML template file**. The code calls `load_email_template("payment-recovery", ...)` but no `payment-recovery.html` template exists in the repo. The `FileNotFoundError` fallback to plaintext will fire every time. If this is intentional (template to be added later), a comment in the code or a note in the PR body would clarify intent. 4. **Hardcoded fallback amounts** (`3000` for tryout, `4000` otherwise) at line ~1890 of admin.py. These magic numbers duplicate pricing logic. If pricing changes, this is a second place to update. Consider extracting to a constant or reading from the tenant/product config. 5. **No integration test for the endpoint itself**. The 4 tests cover the email service function and query filter logic (unit level), but there is no test that hits `POST /admin/send-payment-recovery` via the test client to verify the full request path (auth, Stripe mock, response shape). This is consistent with how some other admin endpoints are tested in this codebase, so not a blocker, but worth noting. ### SOP COMPLIANCE - [x] Branch named after issue: `389-payment-recovery-abandoned-stripe` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references issue #389 with `Closes #389` - [x] No secrets committed - [x] No unrelated file changes (5 files, all scoped to the feature) - [x] Migration follows existing numbered convention (039) - [ ] PR body missing plan slug reference in Related section -- however, this may be a standalone board item without a plan, which is acceptable per conventions ### PROCESS OBSERVATIONS - Clean, well-scoped PR. 554 additions across 5 files, all directly related to the feature. - The dependency on #390 (registration_token fix) is noted in the PR body. Good traceability. - The `test_email` query parameter for safe testing before real sends is a good operational pattern. - Deployment risk is low: new endpoint (no existing behavior changed), additive migration (new column + enum value), admin-only access. ### VERDICT: APPROVED

forgejo_admin merged commit 16b96f547b into main 2026-04-08 21:45:49 +00:00

forgejo_admin deleted branch 389-payment-recovery-abandoned-stripe 2026-04-08 21:45:49 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-08 21:45:51 +00:00

feat: payment recovery for abandoned Stripe registrations (#397)

forgejo_admin referenced this pull request 2026-04-08 22:05:49 +00:00

Payment recovery for abandoned Stripe registrations #389

forgejo_admin referenced this pull request 2026-04-08 22:21:04 +00:00

Bug: update-kustomize-tag skipped when CI tests fail #405

forgejo_admin referenced this pull request 2026-04-10 22:23:48 +00:00

Fix 2 pre-existing failing tests on main (payment_recovery enum + first_payment subject) #434

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!397

No description provided.