forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 88 Pull requests 9 Projects Releases Packages Wiki Activity Actions

feat: cherry-pick first-payment blast + skip_proration to main #469

Open
forgejo_admin wants to merge 3 commits from 466-cherry-pick-first-payment into main
pull from: 466-cherry-pick-first-payment
merge into: forgejo_admin:main
Conversation 1 Commits 3 Files changed 6 +378 -109
forgejo_admin commented 2026-04-13 00:16:58 +00:00
Owner
Copy link

Summary

Cherry-picks 3 commits from 369-first-payment-email-blast onto main. Branch was deployed to prod but never merged (#466).

Changes

  • src/basketball_api/services/email.py ��� send_first_payment_email() with skip_proration kwarg, _calculate_prorated_fee(), branded HTML email
  • src/basketball_api/routes/admin.py �� POST /admin/email/first-payment blast endpoint with skip_proration query param
  • src/basketball_api/routes/checkout.py — GET /checkout/first-payment route with prorate query param for flat-fee charging
  • src/basketball_api/models.py — first_payment added to EmailType enum
  • tests/test_first_payment_email.py — email send tests including skip_proration
  • tests/test_first_payment_blast.py — blast endpoint + checkout route tests

Test Plan

  • CI passes
  • POST /admin/email/first-payment?skip_proration=true sends flat fee email
  • GET /checkout/first-payment?token=X&prorate=false charges exact monthly_fee * 100 cents
  • Send test email to draneylucas@gmail.com, verify on phone
  • Send Jaxon Gerber payment email to fatkid816@gmail.com

Review Checklist

  • Syntax check passes on all modified files
  • Cherry-pick conflicts resolved (1 in email.py, 5 in checkout.py)
  • ruff format + ruff check
  • pytest passes

Related Notes

  • Forgejo issue: #466
  • sop-contract-offer — contract offer SOP
  • sop-email-send — email approval workflow
  • project-westside-basketball — parent project

Closes #466

## Summary Cherry-picks 3 commits from `369-first-payment-email-blast` onto main. Branch was deployed to prod but never merged (#466). ## Changes - `src/basketball_api/services/email.py` ��� `send_first_payment_email()` with `skip_proration` kwarg, `_calculate_prorated_fee()`, branded HTML email - `src/basketball_api/routes/admin.py` �� `POST /admin/email/first-payment` blast endpoint with `skip_proration` query param - `src/basketball_api/routes/checkout.py` — `GET /checkout/first-payment` route with `prorate` query param for flat-fee charging - `src/basketball_api/models.py` — `first_payment` added to EmailType enum - `tests/test_first_payment_email.py` — email send tests including skip_proration - `tests/test_first_payment_blast.py` — blast endpoint + checkout route tests ## Test Plan - [ ] CI passes - [ ] `POST /admin/email/first-payment?skip_proration=true` sends flat fee email - [ ] `GET /checkout/first-payment?token=X&prorate=false` charges exact monthly_fee * 100 cents - [ ] Send test email to draneylucas@gmail.com, verify on phone - [ ] Send Jaxon Gerber payment email to fatkid816@gmail.com ## Review Checklist - [x] Syntax check passes on all modified files - [x] Cherry-pick conflicts resolved (1 in email.py, 5 in checkout.py) - [ ] ruff format + ruff check - [ ] pytest passes ## Related Notes - Forgejo issue: #466 - `sop-contract-offer` — contract offer SOP - `sop-email-send` — email approval workflow - `project-westside-basketball` — parent project Closes #466
Post-practice first-payment email with prorated fee calculation (same formula
as westside-contracts) and branded HTML. Blast endpoint targets signed players
with contract tokens, supports test_email filtering.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
"Our job is to create opportunities. Your job is to put in the work."

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-13 00:19:52 +00:00
Author
Owner
Copy link

PR #469 Review

DOMAIN REVIEW

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

Cherry-pick conflict resolution: The PR cherry-picks 3 commits from 369-first-payment-email-blast onto main. The PR body acknowledges 1 conflict in email.py and 5 in checkout.py. However, the merge produced a critical duplication in admin.py that was not resolved.

Changes reviewed:

  1. models.py -- first_payment enum value added to EmailType. Clean.
  2. routes/admin.py -- New blast endpoint with skip_proration support. Duplicated (see BLOCKERS).
  3. routes/checkout.py -- first_payment_checkout rewritten: returns CheckoutResponse JSON instead of RedirectResponse 307 redirect, adds prorate query param, removes Order creation, removes contract_status == signed filter, imports _calculate_prorated_fee from email service.
  4. services/email.py -- send_first_payment_email gains skip_proration kwarg, email copy updated, conditional proration logic added.
  5. tests/test_first_payment_blast.py -- Tests for blast skip_proration flag, checkout route (prorated, flat, invalid token).
  6. tests/test_first_payment_email.py -- Tests for email send with/without skip_proration.

BLOCKERS

1. Duplicate admin_send_first_payment function and FirstPaymentBlastResponse class in admin.py

The existing main branch already has FirstPaymentBlastResponse (line 978) and admin_send_first_payment (line 984) with @router.post("/email/first-payment"). The cherry-pick appends a SECOND copy of both after line 1021. The second definition:

  • Adds skip_proration parameter (the new behavior)
  • Has @router.post("/email/first-payment") again -- duplicate route registration

In Python, the second class/function definition silently shadows the first. FastAPI will register the route twice, with undefined behavior on which handler receives requests. This is a cherry-pick conflict that was not resolved -- the old version should have been REPLACED, not duplicated.

File: /home/ldraney/basketball-api/src/basketball_api/routes/admin.py, lines 978-1090 (two copies).

2. PR is not mergeable

Forgejo reports mergeable: false. The PR cannot be merged in its current state. Conflicts must be resolved.

3. Removed contract_status == signed guard in checkout route -- security/business logic concern

The old first_payment_checkout filtered by Player.contract_status == ContractStatus.signed. The new version only filters by contract_token. This means a player who has NOT signed their contract (but has a token assigned) can now access the payment checkout. This may be intentional (to allow payment before contract signing completes), but it is a behavioral change that weakens a business-logic guard. If intentional, it should be documented in the PR body.

4. Removed Order creation from checkout flow -- data integrity concern

The old checkout route created an Order record (with OrderStatus.pending), linked it to the Stripe session via stripe_checkout_session_id, and prevented duplicate orders. The new version creates a Stripe session with NO corresponding Order record. This means:

  • No duplicate payment prevention
  • No order tracking in the database
  • The webhook that processes checkout.session.completed events may expect an order_id in metadata (the old code set metadata.order_id; the new code does not)

If this is intentional simplification, it needs to be called out and the webhook handler verified. As-is, this risks orphaned Stripe payments with no database record.

NITS

  1. Hardcoded "April" in fee labels (checkout.py lines in diff: "Prorated April fee", "April fee"). The existing codebase has this pattern throughout, so this is consistent -- but it will need updating when May rolls around. Consider a TODO comment or config value.

  2. Cross-module import of private function: _calculate_prorated_fee (underscore-prefixed, conventionally private) is imported from services.email into routes/checkout.py. If this is now a shared utility, consider renaming it without the underscore or moving it to a shared module.

  3. _calculate_prorated_fee receives player.monthly_fee directly in checkout.py (_calculate_prorated_fee(player.monthly_fee)), and the function handles None internally (fee = monthly_fee or 200). Meanwhile, the checkout route also does monthly_fee = player.monthly_fee or 200 separately for the flat-fee path. The default-200 logic is duplicated across two code paths.

  4. Test coverage is good: Tests cover skip_proration=true/false for both the blast endpoint and the email service, plus checkout route prorated/flat/invalid-token paths. Well structured.

SOP COMPLIANCE

  • Branch named after issue: 466-cherry-pick-first-payment follows {issue-number}-{kebab-case-purpose}
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related references plan slug / project context
  • No secrets committed
  • No unnecessary file changes -- the duplicate function in admin.py is a cherry-pick artifact that must be resolved
  • Tests exist for new functionality

PROCESS OBSERVATIONS

  • Change failure risk: HIGH -- The duplicate function definition in admin.py will cause runtime issues. The removed Order creation breaks the payment tracking pipeline. These are cherry-pick conflict resolution errors that need careful manual review.
  • Deployment frequency impact: This PR unblocks first-payment billing with the skip_proration feature, which is operationally needed. But it cannot ship until the duplicates are resolved and the Order removal is validated against the webhook handler.
  • Documentation gap: The PR body does not explain WHY the contract_status == signed filter and Order creation were removed. These are significant behavioral changes that need justification.

VERDICT: NOT APPROVED

Two critical blockers must be resolved:

  1. Remove the duplicate FirstPaymentBlastResponse class and admin_send_first_payment function in admin.py (keep only the new version with skip_proration)
  2. Address the mergeable: false status
  3. Clarify and validate the removal of Order creation and contract_status guard -- either restore them or document why they were intentionally removed and verify the Stripe webhook handler still works
## PR #469 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Stripe / Gmail SDK **Cherry-pick conflict resolution**: The PR cherry-picks 3 commits from `369-first-payment-email-blast` onto main. The PR body acknowledges 1 conflict in email.py and 5 in checkout.py. However, the merge produced a critical duplication in `admin.py` that was not resolved. **Changes reviewed**: 1. `models.py` -- `first_payment` enum value added to `EmailType`. Clean. 2. `routes/admin.py` -- New blast endpoint with `skip_proration` support. **Duplicated** (see BLOCKERS). 3. `routes/checkout.py` -- `first_payment_checkout` rewritten: returns `CheckoutResponse` JSON instead of `RedirectResponse` 307 redirect, adds `prorate` query param, removes `Order` creation, removes `contract_status == signed` filter, imports `_calculate_prorated_fee` from email service. 4. `services/email.py` -- `send_first_payment_email` gains `skip_proration` kwarg, email copy updated, conditional proration logic added. 5. `tests/test_first_payment_blast.py` -- Tests for blast skip_proration flag, checkout route (prorated, flat, invalid token). 6. `tests/test_first_payment_email.py` -- Tests for email send with/without skip_proration. ### BLOCKERS **1. Duplicate `admin_send_first_payment` function and `FirstPaymentBlastResponse` class in `admin.py`** The existing main branch already has `FirstPaymentBlastResponse` (line 978) and `admin_send_first_payment` (line 984) with `@router.post("/email/first-payment")`. The cherry-pick appends a SECOND copy of both after line 1021. The second definition: - Adds `skip_proration` parameter (the new behavior) - Has `@router.post("/email/first-payment")` again -- duplicate route registration In Python, the second class/function definition silently shadows the first. FastAPI will register the route twice, with undefined behavior on which handler receives requests. This is a cherry-pick conflict that was not resolved -- the old version should have been REPLACED, not duplicated. File: `/home/ldraney/basketball-api/src/basketball_api/routes/admin.py`, lines 978-1090 (two copies). **2. PR is not mergeable** Forgejo reports `mergeable: false`. The PR cannot be merged in its current state. Conflicts must be resolved. **3. Removed `contract_status == signed` guard in checkout route -- security/business logic concern** The old `first_payment_checkout` filtered by `Player.contract_status == ContractStatus.signed`. The new version only filters by `contract_token`. This means a player who has NOT signed their contract (but has a token assigned) can now access the payment checkout. This may be intentional (to allow payment before contract signing completes), but it is a behavioral change that weakens a business-logic guard. If intentional, it should be documented in the PR body. **4. Removed `Order` creation from checkout flow -- data integrity concern** The old checkout route created an `Order` record (with `OrderStatus.pending`), linked it to the Stripe session via `stripe_checkout_session_id`, and prevented duplicate orders. The new version creates a Stripe session with NO corresponding `Order` record. This means: - No duplicate payment prevention - No order tracking in the database - The webhook that processes `checkout.session.completed` events may expect an `order_id` in metadata (the old code set `metadata.order_id`; the new code does not) If this is intentional simplification, it needs to be called out and the webhook handler verified. As-is, this risks orphaned Stripe payments with no database record. ### NITS 1. **Hardcoded "April" in fee labels** (`checkout.py` lines in diff: `"Prorated April fee"`, `"April fee"`). The existing codebase has this pattern throughout, so this is consistent -- but it will need updating when May rolls around. Consider a TODO comment or config value. 2. **Cross-module import of private function**: `_calculate_prorated_fee` (underscore-prefixed, conventionally private) is imported from `services.email` into `routes/checkout.py`. If this is now a shared utility, consider renaming it without the underscore or moving it to a shared module. 3. **`_calculate_prorated_fee` receives `player.monthly_fee` directly** in checkout.py (`_calculate_prorated_fee(player.monthly_fee)`), and the function handles `None` internally (`fee = monthly_fee or 200`). Meanwhile, the checkout route also does `monthly_fee = player.monthly_fee or 200` separately for the flat-fee path. The default-200 logic is duplicated across two code paths. 4. **Test coverage is good**: Tests cover skip_proration=true/false for both the blast endpoint and the email service, plus checkout route prorated/flat/invalid-token paths. Well structured. ### SOP COMPLIANCE - [x] Branch named after issue: `466-cherry-pick-first-payment` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references plan slug / project context - [x] No secrets committed - [ ] No unnecessary file changes -- the duplicate function in admin.py is a cherry-pick artifact that must be resolved - [x] Tests exist for new functionality ### PROCESS OBSERVATIONS - **Change failure risk: HIGH** -- The duplicate function definition in admin.py will cause runtime issues. The removed Order creation breaks the payment tracking pipeline. These are cherry-pick conflict resolution errors that need careful manual review. - **Deployment frequency impact**: This PR unblocks first-payment billing with the skip_proration feature, which is operationally needed. But it cannot ship until the duplicates are resolved and the Order removal is validated against the webhook handler. - **Documentation gap**: The PR body does not explain WHY the `contract_status == signed` filter and `Order` creation were removed. These are significant behavioral changes that need justification. ### VERDICT: NOT APPROVED Two critical blockers must be resolved: 1. Remove the duplicate `FirstPaymentBlastResponse` class and `admin_send_first_payment` function in admin.py (keep only the new version with `skip_proration`) 2. Address the `mergeable: false` status 3. Clarify and validate the removal of `Order` creation and `contract_status` guard -- either restore them or document why they were intentionally removed and verify the Stripe webhook handler still works
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
This pull request has changes conflicting with the target branch.
  • src/basketball_api/models.py
  • src/basketball_api/routes/checkout.py
  • src/basketball_api/services/email.py
  • tests/test_first_payment_email.py
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 466-cherry-pick-first-payment:466-cherry-pick-first-payment
git switch 466-cherry-pick-first-payment

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 466-cherry-pick-first-payment
git switch 466-cherry-pick-first-payment
git rebase main
git switch main
git merge --ff-only 466-cherry-pick-first-payment
git switch 466-cherry-pick-first-payment
git rebase main
git switch main
git merge --no-ff 466-cherry-pick-first-payment
git switch main
git merge --squash 466-cherry-pick-first-payment
git switch main
git merge --ff-only 466-cherry-pick-first-payment
git switch main
git merge 466-cherry-pick-first-payment
git push origin main
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
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!469
No description provided.