ldraney/basketball-api

Fork 0

fix: May monthly payment email + dedup + full fee #521

Open

forgejo_admin wants to merge 3 commits from monthly-payment-email-may into main

forgejo_admin commented

2026-05-08 06:08:52 +00:00

Contributor

Summary

Rewrites first-payment email copy for May: removes April proration references and bug apology, replaces with clean "monthly club dues" messaging
Adds subscription dedup filter so active/past_due players are excluded from blast
Changes checkout to charge full monthly fee instead of prorated amount
Includes PaymentLink model + migration + outstanding balance endpoint (prior session work bundled)

Changes

services/email.py: Rewrite send_first_payment_email — subject, HTML body, and plain text updated for current monthly billing
routes/admin.py: Add SubscriptionStatus import, filter notin_(active, past_due) on first-payment blast query; add outstanding balance endpoint + imports
routes/checkout.py: Replace _calculate_prorated_cents(monthly_fee) with monthly_fee * 100
models.py: Add PaymentLink model, PaymentLinkStatus enum, outstanding_balance email type
services/email_queries.py: Add outstanding balance email query
alembic/versions/048_add_payment_links_table.py: Migration for payment_links table
scripts/backfill_payment_links.py: Backfill existing Stripe payment links into new table

Test Plan

Send test email to draneylucas@gmail.com via POST /admin/email/first-payment?test_email=draneylucas@gmail.com
Verify email renders "Monthly fee: $X" — no April, no proration, no bug apology
Verify payment link charges full monthly (not prorated)
Verify active-subscription players excluded from blast
Run migration 048 successfully
No regressions in tournament checkout or existing payment flows

Review Checklist

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

Closes #520
westside-basketball — project
westside-billing-status-2026-05 — billing status snapshot

## Summary - Rewrites first-payment email copy for May: removes April proration references and bug apology, replaces with clean "monthly club dues" messaging - Adds subscription dedup filter so active/past_due players are excluded from blast - Changes checkout to charge full monthly fee instead of prorated amount - Includes PaymentLink model + migration + outstanding balance endpoint (prior session work bundled) ## Changes - `services/email.py`: Rewrite `send_first_payment_email` — subject, HTML body, and plain text updated for current monthly billing - `routes/admin.py`: Add `SubscriptionStatus` import, filter `notin_(active, past_due)` on first-payment blast query; add outstanding balance endpoint + imports - `routes/checkout.py`: Replace `_calculate_prorated_cents(monthly_fee)` with `monthly_fee * 100` - `models.py`: Add `PaymentLink` model, `PaymentLinkStatus` enum, `outstanding_balance` email type - `services/email_queries.py`: Add outstanding balance email query - `alembic/versions/048_add_payment_links_table.py`: Migration for payment_links table - `scripts/backfill_payment_links.py`: Backfill existing Stripe payment links into new table ## Test Plan - [ ] Send test email to draneylucas@gmail.com via `POST /admin/email/first-payment?test_email=draneylucas@gmail.com` - [ ] Verify email renders "Monthly fee: $X" — no April, no proration, no bug apology - [ ] Verify payment link charges full monthly (not prorated) - [ ] Verify active-subscription players excluded from blast - [ ] Run migration 048 successfully - [ ] No regressions in tournament checkout or existing payment flows ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - Closes #520 - `westside-basketball` — project - `westside-billing-status-2026-05` — billing status snapshot

forgejo_admin added 2 commits

2026-05-08 06:08:52 +00:00

feat: add PaymentLink model, outstanding balance email, and migration d5371c3adc

Adds payment_links table for tracking per-player Stripe payment links
with status. Includes outstanding balance email endpoint and query,
plus backfill script for existing payment links.

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

fix: update monthly payment email for May, add dedup, use full fee

ci/woodpecker/pr/woodpecker Pipeline failed

Details

a18fb79c67

- Rewrite email copy: remove stale April proration and bug apology,
  clean May monthly with card-save messaging
- Add dedup: filter out players with active/past_due subscriptions
  from the first-payment blast endpoint
- Charge full monthly fee instead of prorated amount

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

forgejo_admin commented

2026-05-08 06:11:23 +00:00

Author

Contributor

PR #521 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Alembic / Stripe SDK

This PR bundles three concerns into one branch: (1) rewriting the first-payment email copy to remove April proration references, (2) adding subscription dedup filtering on the blast endpoint, (3) adding an entirely new PaymentLink model + migration + outstanding balance email endpoint + backfill script. The first two map directly to issue #520; the third is prior-session work from closed issue #518 / PR #519.

Migration 048 (alembic/versions/048_add_payment_links_table.py):

Table schema is clean: proper FKs, indexes, unique constraint.
ALTER TYPE emailtype ADD VALUE IF NOT EXISTS 'outstanding_balance' -- correct PostgreSQL syntax for extending existing enums.
downgrade() drops the table and the paymentlinkstatus enum but does NOT remove the outstanding_balance value from the emailtype enum. PostgreSQL cannot remove individual enum values without recreating the type, so this is a known limitation, not a bug. Worth a comment in the migration though.

Model (models.py):

PaymentLink model aligns with the migration. Relationships declared without back_populates, which is consistent with the rest of the codebase (other models also use bare relationship()).
PaymentLinkStatus enum is clean. Matches OrderStatus shape.

Checkout (routes/checkout.py):

_calculate_prorated_cents(monthly_fee) replaced with monthly_fee * 100. The function _calculate_prorated_cents is now dead code at line 296 of checkout.py -- still defined but never called. Same for _calculate_prorated_fee at line 1012 of email.py.
The docstring on first_payment_checkout (line 313) still says "prorated first monthly payment" -- stale after this change.

Admin (routes/admin.py):

Subscription dedup filter using Player.subscription_status.notin_([active, past_due]) is correct and directly addresses issue #520.
New /email/outstanding-balance endpoint: groups active PaymentLink rows by parent, builds item dicts, delegates to send_outstanding_balance_email. Auth-gated via require_admin. Good.

DRY concern -- outstanding balance grouping logic is duplicated:

routes/admin.py lines 1376-1407: queries PaymentLink, groups by parent_id, builds item dicts, iterates.
services/email_queries.py lines 299-356: queries PaymentLink, groups by parent_id, builds HTML rows, iterates.
These do similar but not identical work (one builds dicts for send_outstanding_balance_email, the other builds pre-rendered HTML for the blast registry). This is not a security/auth path so it does not meet the BLOCKER threshold, but the divergence risk is real -- if the filter criteria change (e.g., add tenant filter, exclude canceled), both must be updated.

Email (services/email.py):

send_outstanding_balance_email is well-structured: HTML table with per-item "Pay Now" links, total summary, plain-text fallback, email logging.
Uses escape() on user-supplied data (player_name, description, tenant.name) -- XSS-safe for HTML email context.
The stripe_payment_link_url is rendered as an href without escaping. Stripe URLs are system-generated, not user-supplied, so this is acceptable.

Backfill script (scripts/backfill_payment_links.py):

Dry-run by default with --commit flag. Good safety pattern.
Stripe key loading: checks env var, then file at ~/secrets/basketball-api/stripe-api-key. No hardcoded keys.
Handles errors per-player without aborting the batch. Commits or rolls back atomically at the end.
Uses db.query(Product).get(order.product_id) -- .get() is deprecated in SQLAlchemy 2.0 (should be db.get(Product, order.product_id)). Non-blocking but worth noting.

BLOCKERS

1. No test coverage for new functionality.

Three pieces of new functionality have zero tests:

The PaymentLink model
The /email/outstanding-balance admin endpoint
The send_outstanding_balance_email service function
The query_outstanding_balances query function
The subscription dedup filter on admin_send_first_payment

The existing test files (test_admin.py, test_checkout.py) do not cover any of these new paths. Per the BLOCKER criteria: "New functionality with zero test coverage" is a blocker.

At minimum, the dedup filter on admin_send_first_payment needs a test proving that players with subscription_status=active are excluded from the blast query -- this is the core fix for issue #520 and a financial safety guard.

NITS

Dead code: _calculate_prorated_cents in checkout.py (line 296) and _calculate_prorated_fee in email.py (line 1012) are now unreferenced. Remove them or add a deprecation comment.
Stale docstring: first_payment_checkout docstring (checkout.py line 313) still says "prorated first monthly payment" -- should say "full monthly payment" now.
DRY opportunity: The outstanding balance grouping/filtering logic in admin.py and email_queries.py could share a common helper. Not blocking, but reduces divergence risk on future filter changes.
Deprecated SQLAlchemy API: db.query(Product).get(order.product_id) in backfill_payment_links.py line 220 -- use db.get(Product, order.product_id) for SQLAlchemy 2.0 compatibility.
Scope bundling: The PR bundles the #520 fix (email copy + dedup + full fee) with the #518 PaymentLink feature (model + migration + outstanding balance endpoint + backfill). The PR body acknowledges this ("prior session work bundled"). This makes the PR harder to review and revert independently.
Missing player_id on outstanding balance email log: send_outstanding_balance_email creates an EmailLog without player_id (email.py line 2594-2600). This is technically correct since the email covers multiple players, but differs from send_first_payment_email which logs player_id. Consider documenting this intentional difference.

SOP COMPLIANCE

Branch named after issue -- Branch is monthly-payment-email-may, not 520-monthly-payment-email-may. Does not follow {issue-number}-{kebab-case-purpose} convention.
PR body follows template -- Has Summary, Changes, Test Plan, Review Checklist, Related sections.
Related references plan slug -- References westside-basketball project and westside-billing-status-2026-05 note, but no plan slug (acknowledged: "No plan slug").
No secrets committed -- No credentials, .env files, or API keys in the diff.
No unnecessary file changes -- PaymentLink model/migration/backfill/outstanding-balance endpoint belong to closed #518, not #520. Scope creep by bundling.

PROCESS OBSERVATIONS

Change failure risk: The subscription dedup filter is a financial safety guard (prevents double-billing already-paying parents). It deserves dedicated test coverage to prevent regression.
Deployment frequency: Bundling #518 work into #520 creates a larger atomic change. If the outstanding-balance endpoint has issues, rolling back also reverts the #520 email fix.
Dead code accumulation: Two proration functions are now orphaned. Leaving dead code increases maintenance burden and confuses future developers about intended behavior.

VERDICT: NOT APPROVED

Reason: New functionality (PaymentLink model, outstanding balance endpoint, dedup filter) has zero test coverage. The subscription dedup filter is a financial safety guard that must be tested. Add at minimum:

A test for admin_send_first_payment proving subscription_status=active players are excluded.
A test for /email/outstanding-balance endpoint (happy path + empty state).
Remove or mark the dead proration functions.

## PR #521 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Alembic / Stripe SDK This PR bundles three concerns into one branch: (1) rewriting the first-payment email copy to remove April proration references, (2) adding subscription dedup filtering on the blast endpoint, (3) adding an entirely new `PaymentLink` model + migration + outstanding balance email endpoint + backfill script. The first two map directly to issue #520; the third is prior-session work from closed issue #518 / PR #519. **Migration 048** (`alembic/versions/048_add_payment_links_table.py`): - Table schema is clean: proper FKs, indexes, unique constraint. - `ALTER TYPE emailtype ADD VALUE IF NOT EXISTS 'outstanding_balance'` -- correct PostgreSQL syntax for extending existing enums. - `downgrade()` drops the table and the `paymentlinkstatus` enum but does NOT remove the `outstanding_balance` value from the `emailtype` enum. PostgreSQL cannot remove individual enum values without recreating the type, so this is a known limitation, not a bug. Worth a comment in the migration though. **Model (`models.py`)**: - `PaymentLink` model aligns with the migration. Relationships declared without `back_populates`, which is consistent with the rest of the codebase (other models also use bare `relationship()`). - `PaymentLinkStatus` enum is clean. Matches `OrderStatus` shape. **Checkout (`routes/checkout.py`)**: - `_calculate_prorated_cents(monthly_fee)` replaced with `monthly_fee * 100`. The function `_calculate_prorated_cents` is now **dead code** at line 296 of checkout.py -- still defined but never called. Same for `_calculate_prorated_fee` at line 1012 of email.py. - The docstring on `first_payment_checkout` (line 313) still says "prorated first monthly payment" -- stale after this change. **Admin (`routes/admin.py`)**: - Subscription dedup filter using `Player.subscription_status.notin_([active, past_due])` is correct and directly addresses issue #520. - New `/email/outstanding-balance` endpoint: groups active `PaymentLink` rows by parent, builds item dicts, delegates to `send_outstanding_balance_email`. Auth-gated via `require_admin`. Good. **DRY concern -- outstanding balance grouping logic is duplicated**: - `routes/admin.py` lines 1376-1407: queries `PaymentLink`, groups by `parent_id`, builds item dicts, iterates. - `services/email_queries.py` lines 299-356: queries `PaymentLink`, groups by `parent_id`, builds HTML rows, iterates. - These do similar but not identical work (one builds dicts for `send_outstanding_balance_email`, the other builds pre-rendered HTML for the blast registry). This is not a security/auth path so it does not meet the BLOCKER threshold, but the divergence risk is real -- if the filter criteria change (e.g., add tenant filter, exclude canceled), both must be updated. **Email (`services/email.py`)**: - `send_outstanding_balance_email` is well-structured: HTML table with per-item "Pay Now" links, total summary, plain-text fallback, email logging. - Uses `escape()` on user-supplied data (`player_name`, `description`, `tenant.name`) -- XSS-safe for HTML email context. - The `stripe_payment_link_url` is rendered as an `href` without escaping. Stripe URLs are system-generated, not user-supplied, so this is acceptable. **Backfill script (`scripts/backfill_payment_links.py`)**: - Dry-run by default with `--commit` flag. Good safety pattern. - Stripe key loading: checks env var, then file at `~/secrets/basketball-api/stripe-api-key`. No hardcoded keys. - Handles errors per-player without aborting the batch. Commits or rolls back atomically at the end. - Uses `db.query(Product).get(order.product_id)` -- `.get()` is deprecated in SQLAlchemy 2.0 (should be `db.get(Product, order.product_id)`). Non-blocking but worth noting. ### BLOCKERS **1. No test coverage for new functionality.** Three pieces of new functionality have zero tests: - The `PaymentLink` model - The `/email/outstanding-balance` admin endpoint - The `send_outstanding_balance_email` service function - The `query_outstanding_balances` query function - The subscription dedup filter on `admin_send_first_payment` The existing test files (`test_admin.py`, `test_checkout.py`) do not cover any of these new paths. Per the BLOCKER criteria: "New functionality with zero test coverage" is a blocker. At minimum, the dedup filter on `admin_send_first_payment` needs a test proving that players with `subscription_status=active` are excluded from the blast query -- this is the core fix for issue #520 and a financial safety guard. ### NITS 1. **Dead code**: `_calculate_prorated_cents` in `checkout.py` (line 296) and `_calculate_prorated_fee` in `email.py` (line 1012) are now unreferenced. Remove them or add a deprecation comment. 2. **Stale docstring**: `first_payment_checkout` docstring (checkout.py line 313) still says "prorated first monthly payment" -- should say "full monthly payment" now. 3. **DRY opportunity**: The outstanding balance grouping/filtering logic in `admin.py` and `email_queries.py` could share a common helper. Not blocking, but reduces divergence risk on future filter changes. 4. **Deprecated SQLAlchemy API**: `db.query(Product).get(order.product_id)` in `backfill_payment_links.py` line 220 -- use `db.get(Product, order.product_id)` for SQLAlchemy 2.0 compatibility. 5. **Scope bundling**: The PR bundles the #520 fix (email copy + dedup + full fee) with the #518 PaymentLink feature (model + migration + outstanding balance endpoint + backfill). The PR body acknowledges this ("prior session work bundled"). This makes the PR harder to review and revert independently. 6. **Missing `player_id` on outstanding balance email log**: `send_outstanding_balance_email` creates an `EmailLog` without `player_id` (email.py line 2594-2600). This is technically correct since the email covers multiple players, but differs from `send_first_payment_email` which logs `player_id`. Consider documenting this intentional difference. ### SOP COMPLIANCE - [ ] Branch named after issue -- Branch is `monthly-payment-email-may`, not `520-monthly-payment-email-may`. Does not follow `{issue-number}-{kebab-case-purpose}` convention. - [x] PR body follows template -- Has Summary, Changes, Test Plan, Review Checklist, Related sections. - [ ] Related references plan slug -- References `westside-basketball` project and `westside-billing-status-2026-05` note, but no plan slug (acknowledged: "No plan slug"). - [x] No secrets committed -- No credentials, .env files, or API keys in the diff. - [ ] No unnecessary file changes -- PaymentLink model/migration/backfill/outstanding-balance endpoint belong to closed #518, not #520. Scope creep by bundling. ### PROCESS OBSERVATIONS - **Change failure risk**: The subscription dedup filter is a financial safety guard (prevents double-billing already-paying parents). It deserves dedicated test coverage to prevent regression. - **Deployment frequency**: Bundling #518 work into #520 creates a larger atomic change. If the outstanding-balance endpoint has issues, rolling back also reverts the #520 email fix. - **Dead code accumulation**: Two proration functions are now orphaned. Leaving dead code increases maintenance burden and confuses future developers about intended behavior. ### VERDICT: NOT APPROVED **Reason**: New functionality (PaymentLink model, outstanding balance endpoint, dedup filter) has zero test coverage. The subscription dedup filter is a financial safety guard that must be tested. Add at minimum: 1. A test for `admin_send_first_payment` proving `subscription_status=active` players are excluded. 2. A test for `/email/outstanding-balance` endpoint (happy path + empty state). 3. Remove or mark the dead proration functions.

forgejo_admin added 1 commit

2026-05-08 06:12:22 +00:00

fix: remove dead proration code and update checkout docstring

ci/woodpecker/pr/woodpecker Pipeline failed

Details

12ee4b0775

_calculate_prorated_cents and _calculate_prorated_fee are no longer
called after switching to full monthly fee. Docstring updated to
reflect the new behavior.

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

ci/woodpecker/pr/woodpecker Pipeline failed

Details

This pull request has changes conflicting with the target branch.

alembic/versions/048_add_payment_links_table.py
scripts/backfill_payment_links.py
src/basketball_api/models.py
src/basketball_api/services/email_queries.py

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin monthly-payment-email-may:monthly-payment-email-may

git switch monthly-payment-email-may

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff monthly-payment-email-may

git switch monthly-payment-email-may

git rebase main

git switch main

git merge --ff-only monthly-payment-email-may

git switch monthly-payment-email-may

git rebase main

git switch main

git merge --no-ff monthly-payment-email-may

git switch main

git merge --squash monthly-payment-email-may