feat: add payment_links table and outstanding balance email

forgejo_admin commented

2026-05-08 05:32:18 +00:00

Author

Contributor

PR #519 Review

DOMAIN REVIEW

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

This PR adds a payment_links table, a backfill script that mints Stripe Payment Links, a query registry entry, an admin blast endpoint, and a branded email sender. Six files, 733 additions, zero deletions.

BLOCKERS

1. Zero test coverage (733 lines of new functionality)

No test file is included in this PR. This is a hard blocker per review policy. The following need tests:

Migration 048: at minimum a test that upgrade/downgrade runs without error against a test DB.
backfill_payment_links.py: dry-run path, commit path with mocked Stripe, idempotent skip when link exists, error handling when _mint_payment_link raises.
admin_send_outstanding_balance endpoint: happy path (grouped by parent), test_email filter, empty links, missing parent email.
send_outstanding_balance_email: HTML escaping of player/description, zero-item edge, Gmail send failure.
query_outstanding_balances: grouping logic, test_email filter, parent with no email skipped.

2. Massive DRY violation between query_outstanding_balances and admin_send_outstanding_balance

The admin route (admin.py lines 1345-1419) and the query function (email_queries.py lines 281-366) duplicate nearly identical logic:

Both query PaymentLink filtered by tenant + active status.
Both group by parent_id using dict.setdefault.
Both skip parents with no email.
Both apply a test_email filter.
Both render HTML table rows with hardcoded inline styles.

The query function (query_outstanding_balances) is registered in QUERY_REGISTRY but the admin endpoint does not use it. The endpoint re-implements the same grouping and filtering from scratch. This means:

A bug fix in one path will not propagate to the other.
The query function renders HTML with hardcoded hex colors (#f0f0f0, #333, #d42026) while the email function uses brand constants (_BRAND_LIGHT, _BRAND_BORDER, _BRAND_RED). These will diverge.

Either the endpoint should call the query function, or the query function should be removed from the registry if it is unused. Two parallel implementations of the same business logic is a defect factory.

3. Hardcoded inline styles diverge from brand constants

In email_queries.py, the query_outstanding_balances function hardcodes CSS colors directly:

f'border-bottom: 1px solid #333;'
f'background: #d42026; color: #ffffff;'

Meanwhile email.py uses the defined brand constants (_BRAND_RED, _BRAND_BORDER, _BRAND_WHITE, etc.). If brand colors change, query_outstanding_balances will render stale colors. This is a correctness risk for a function registered in the query registry and presumably callable from the generic blast path.

NITS

4. db.query(Product).get(order.product_id) is deprecated

In backfill_payment_links.py line ~220, Session.query(Model).get(pk) was deprecated in SQLAlchemy 2.0. Use db.get(Product, order.product_id) instead.

5. Backfill script swallows exceptions with broad except Exception

Lines ~180 and ~230 in the backfill script catch Exception broadly, log it, and continue. This is acceptable for a backfill (you want partial progress), but the error counter should be surfaced more prominently. Consider also logging the traceback with logger.exception() (which it does -- good) but returning a non-zero exit code even on partial success (which it does via the errors counter -- also good). No action needed, just noting the pattern.

6. payment_intent_data on Payment Links

In _mint_payment_link, payment_intent_data is set on PaymentLink.create() for monthly links. Per Stripe docs, payment_intent_data.setup_future_usage on Payment Links is valid, but verify this works correctly in test mode before blast. The PR checklist marks this as reviewed, which is good.

7. Unique constraint allows product_id = NULL duplicates

The unique constraint (player_id, product_id, tenant_id) will not prevent duplicate rows where product_id IS NULL, because SQL NULL != NULL in unique constraints. If backfill_from_orders ever processes orders without a product_id, duplicates can slip through. The backfill code does query Order.product_id without a null check. Consider whether a partial index or application-level guard is needed.

8. Migration downgrade does not remove the emailtype enum value

The downgrade drops the table and the paymentlinkstatus type, but does not remove 'outstanding_balance' from the emailtype enum. PostgreSQL does not support ALTER TYPE ... REMOVE VALUE, so this is technically correct (you cannot cleanly downgrade enum values in PG), but worth documenting in a comment.

9. _SUCCESS_URL loaded at module import time

The _SUCCESS_URL in the backfill script reads FRONTEND_URL at import time via os.environ.get. If the env var is set after import (unlikely for a script, but possible in test harnesses), the fallback URL will be used silently. Minor, but worth noting.

10. The send_outstanding_balance_email function commits inside itself

Line 2625 of email.py: db.commit() is called inside the email send function after inserting the EmailLog. But the caller (admin_send_outstanding_balance) does not expect this -- it loops over parents and each send commits independently. If a later send fails, earlier sends are already committed. This is inconsistent with the blast pattern where you might want atomicity. Verify this matches the existing email send pattern in the codebase (it likely does, given other send_* functions also commit internally, but it is worth confirming).

SOP COMPLIANCE

Branch named after issue: 518-add-payment-links-table-and-outstanding follows {issue-number}-{kebab-case-purpose} convention
PR body has Summary, Changes, Test Plan, Related sections
Related references plan slug: References westside-billing-status-2026-05 (a note, not a plan slug) and PR #498. No plan slug, which is acceptable since this is standalone work without a plan.
No secrets committed: Stripe key loaded from env/file, not hardcoded
No unnecessary file changes (scope is tight to the feature)
Tests exist: No tests included -- BLOCKER
Commit messages are descriptive

PROCESS OBSERVATIONS

Change failure risk: HIGH. This PR touches Stripe (real money), email blasts (parent-facing), and a new DB table. The DRY violation between the query function and the admin endpoint means a bug in grouping/filtering logic could cause over-sends or missed parents, and fixing one path would not fix the other. The blast-pool verification feedback (feedback_blast_pool_verification) is directly relevant here.
Deployment frequency: No impact on deployment frequency itself, but the lack of tests means CI cannot gate regressions on this feature.
Migration coordination: Migration 048 exists as both this PR and PR #515 (GRANT tournament tables). Verify these do not collide on the 048 slot. (Per feedback_migration_slot_coordination.)

VERDICT: NOT APPROVED

Two blockers must be resolved:

Add tests for the migration, backfill script, admin endpoint, email sender, and query function. At minimum: happy path + error path + idempotency for the backfill, and grouped-by-parent + test_email filter for the endpoint.
Eliminate the DRY violation between query_outstanding_balances in email_queries.py and the inline implementation in admin_send_outstanding_balance. Either the endpoint should consume the query function, or the query function should be removed. Two parallel implementations of the same grouping/filtering/rendering logic is unacceptable.

## PR #519 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic / Stripe API / Gmail SDK This PR adds a `payment_links` table, a backfill script that mints Stripe Payment Links, a query registry entry, an admin blast endpoint, and a branded email sender. Six files, 733 additions, zero deletions. --- ### BLOCKERS **1. Zero test coverage (733 lines of new functionality)** No test file is included in this PR. This is a hard blocker per review policy. The following need tests: - **Migration 048**: at minimum a test that upgrade/downgrade runs without error against a test DB. - **`backfill_payment_links.py`**: dry-run path, commit path with mocked Stripe, idempotent skip when link exists, error handling when `_mint_payment_link` raises. - **`admin_send_outstanding_balance` endpoint**: happy path (grouped by parent), `test_email` filter, empty links, missing parent email. - **`send_outstanding_balance_email`**: HTML escaping of player/description, zero-item edge, Gmail send failure. - **`query_outstanding_balances`**: grouping logic, `test_email` filter, parent with no email skipped. **2. Massive DRY violation between `query_outstanding_balances` and `admin_send_outstanding_balance`** The admin route (`admin.py` lines 1345-1419) and the query function (`email_queries.py` lines 281-366) duplicate nearly identical logic: - Both query `PaymentLink` filtered by tenant + active status. - Both group by `parent_id` using `dict.setdefault`. - Both skip parents with no email. - Both apply a `test_email` filter. - Both render HTML table rows with hardcoded inline styles. The query function (`query_outstanding_balances`) is registered in `QUERY_REGISTRY` but **the admin endpoint does not use it**. The endpoint re-implements the same grouping and filtering from scratch. This means: - A bug fix in one path will not propagate to the other. - The query function renders HTML with **hardcoded hex colors** (`#f0f0f0`, `#333`, `#d42026`) while the email function uses brand constants (`_BRAND_LIGHT`, `_BRAND_BORDER`, `_BRAND_RED`). These will diverge. Either the endpoint should call the query function, or the query function should be removed from the registry if it is unused. Two parallel implementations of the same business logic is a defect factory. **3. Hardcoded inline styles diverge from brand constants** In `email_queries.py`, the `query_outstanding_balances` function hardcodes CSS colors directly: ```python f'border-bottom: 1px solid #333;' f'background: #d42026; color: #ffffff;' ``` Meanwhile `email.py` uses the defined brand constants (`_BRAND_RED`, `_BRAND_BORDER`, `_BRAND_WHITE`, etc.). If brand colors change, `query_outstanding_balances` will render stale colors. This is a correctness risk for a function registered in the query registry and presumably callable from the generic blast path. --- ### NITS **4. `db.query(Product).get(order.product_id)` is deprecated** In `backfill_payment_links.py` line ~220, `Session.query(Model).get(pk)` was deprecated in SQLAlchemy 2.0. Use `db.get(Product, order.product_id)` instead. **5. Backfill script swallows exceptions with broad `except Exception`** Lines ~180 and ~230 in the backfill script catch `Exception` broadly, log it, and continue. This is acceptable for a backfill (you want partial progress), but the error counter should be surfaced more prominently. Consider also logging the traceback with `logger.exception()` (which it does -- good) but returning a non-zero exit code even on partial success (which it does via the `errors` counter -- also good). No action needed, just noting the pattern. **6. `payment_intent_data` on Payment Links** In `_mint_payment_link`, `payment_intent_data` is set on `PaymentLink.create()` for monthly links. Per Stripe docs, `payment_intent_data.setup_future_usage` on Payment Links is valid, but verify this works correctly in test mode before blast. The PR checklist marks this as reviewed, which is good. **7. Unique constraint allows `product_id = NULL` duplicates** The unique constraint `(player_id, product_id, tenant_id)` will not prevent duplicate rows where `product_id IS NULL`, because SQL NULL != NULL in unique constraints. If `backfill_from_orders` ever processes orders without a `product_id`, duplicates can slip through. The backfill code does query `Order.product_id` without a null check. Consider whether a partial index or application-level guard is needed. **8. Migration downgrade does not remove the `emailtype` enum value** The downgrade drops the table and the `paymentlinkstatus` type, but does not remove `'outstanding_balance'` from the `emailtype` enum. PostgreSQL does not support `ALTER TYPE ... REMOVE VALUE`, so this is technically correct (you cannot cleanly downgrade enum values in PG), but worth documenting in a comment. **9. `_SUCCESS_URL` loaded at module import time** The `_SUCCESS_URL` in the backfill script reads `FRONTEND_URL` at import time via `os.environ.get`. If the env var is set after import (unlikely for a script, but possible in test harnesses), the fallback URL will be used silently. Minor, but worth noting. **10. The `send_outstanding_balance_email` function commits inside itself** Line 2625 of `email.py`: `db.commit()` is called inside the email send function after inserting the `EmailLog`. But the caller (`admin_send_outstanding_balance`) does not expect this -- it loops over parents and each send commits independently. If a later send fails, earlier sends are already committed. This is inconsistent with the blast pattern where you might want atomicity. Verify this matches the existing email send pattern in the codebase (it likely does, given other `send_*` functions also commit internally, but it is worth confirming). --- ### SOP COMPLIANCE - [x] Branch named after issue: `518-add-payment-links-table-and-outstanding` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related references plan slug: References `westside-billing-status-2026-05` (a note, not a plan slug) and PR #498. No plan slug, which is acceptable since this is standalone work without a plan. - [x] No secrets committed: Stripe key loaded from env/file, not hardcoded - [x] No unnecessary file changes (scope is tight to the feature) - [ ] Tests exist: **No tests included** -- BLOCKER - [x] Commit messages are descriptive --- ### PROCESS OBSERVATIONS - **Change failure risk**: HIGH. This PR touches Stripe (real money), email blasts (parent-facing), and a new DB table. The DRY violation between the query function and the admin endpoint means a bug in grouping/filtering logic could cause over-sends or missed parents, and fixing one path would not fix the other. The blast-pool verification feedback (`feedback_blast_pool_verification`) is directly relevant here. - **Deployment frequency**: No impact on deployment frequency itself, but the lack of tests means CI cannot gate regressions on this feature. - **Migration coordination**: Migration 048 exists as both this PR and PR #515 (GRANT tournament tables). Verify these do not collide on the 048 slot. (Per `feedback_migration_slot_coordination`.) --- ### VERDICT: NOT APPROVED Two blockers must be resolved: 1. **Add tests** for the migration, backfill script, admin endpoint, email sender, and query function. At minimum: happy path + error path + idempotency for the backfill, and grouped-by-parent + test_email filter for the endpoint. 2. **Eliminate the DRY violation** between `query_outstanding_balances` in `email_queries.py` and the inline implementation in `admin_send_outstanding_balance`. Either the endpoint should consume the query function, or the query function should be removed. Two parallel implementations of the same grouping/filtering/rendering logic is unacceptable.

2026-05-08 05:30:20 +00:00

Summary

New payment_links table stores persistent Stripe Payment Links per player per product. Backfill script mints links for all signed players. Outstanding balance email blasts parents with per-item Pay Now buttons.

Changes

Migration 048: payment_links table with unique constraint (player_id, product_id, tenant_id)
PaymentLink model, PaymentLinkStatus enum, outstanding_balance EmailType
scripts/backfill_payment_links.py — mints Stripe links for signed players, creates missing price tiers, copies pending Orders. Dry-run default.
query_outstanding_balances in QUERY_REGISTRY — groups active links by parent
send_outstanding_balance_email — branded email with Player | Item | Amount | Pay Now table
POST /admin/email/outstanding-balance?test_email= endpoint

Test Plan

Migration 048 runs clean
Backfill dry-run lists all signed players
Backfill --commit mints Stripe links and inserts rows
Test email to draneylucas@gmail.com
Idempotent re-run (unique constraint)

Review Checklist

setup_future_usage: off_session on monthly links
Unique constraint prevents duplicates
_brand_wrapper (System 1) email styling
Backfill dry-run by default

westside-billing-status-2026-05
PR #498 (Payment Link migration)

Closes #518

## Summary New `payment_links` table stores persistent Stripe Payment Links per player per product. Backfill script mints links for all signed players. Outstanding balance email blasts parents with per-item Pay Now buttons. ## Changes - Migration 048: `payment_links` table with unique constraint (player_id, product_id, tenant_id) - `PaymentLink` model, `PaymentLinkStatus` enum, `outstanding_balance` EmailType - `scripts/backfill_payment_links.py` — mints Stripe links for signed players, creates missing price tiers, copies pending Orders. Dry-run default. - `query_outstanding_balances` in QUERY_REGISTRY — groups active links by parent - `send_outstanding_balance_email` — branded email with Player | Item | Amount | Pay Now table - `POST /admin/email/outstanding-balance?test_email=` endpoint ## Test Plan - [ ] Migration 048 runs clean - [ ] Backfill dry-run lists all signed players - [ ] Backfill `--commit` mints Stripe links and inserts rows - [ ] Test email to draneylucas@gmail.com - [ ] Idempotent re-run (unique constraint) ## Review Checklist - [x] `setup_future_usage: off_session` on monthly links - [x] Unique constraint prevents duplicates - [x] `_brand_wrapper` (System 1) email styling - [x] Backfill dry-run by default ## Related Notes - westside-billing-status-2026-05 - PR #498 (Payment Link migration) Closes #518

forgejo_admin added 1 commit

feat: add payment_links table and outstanding balance email (#518 )

ci/woodpecker/pr/woodpecker Pipeline failed

Details

2528f3891e

New payment_links table stores persistent Stripe Payment Links per
player per product. Backfill script mints links for all signed players
and copies existing pending Orders. Outstanding balance email groups
items by parent with per-item Pay Now buttons.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

2026-05-08 05:34:28 +00:00

fix: address QA review — remove DRY violation, fix deprecated .get(), add enum note

bd861d59da

- Remove query_outstanding_balances from email_queries.py — blast system
  can't handle dynamic HTML lists; admin endpoint is the single path
- Fix deprecated db.query(Product).get() → db.get(Product, id)
- Add comment about PG enum downgrade limitation in migration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>