feat: add monthly ProductCategory, first_payment EmailType, seed prorated product #370

Merged

forgejo_admin merged 1 commit from 367-add-monthly-category-first-payment-email into main 2026-04-07 01:33:48 +00:00

Conversation 2 Commits 1 Files changed 2 +37

forgejo_admin commented 2026-04-07 01:27:16 +00:00

Owner

Copy link

Summary

Adds the monthly ProductCategory and first_payment EmailType enum values needed by the first-payment checkout flow, and seeds a prorated April product.

Changes

• src/basketball_api/models.py: Added monthly to ProductCategory enum, first_payment to EmailType enum
• alembic/versions/037_add_monthly_category_and_first_payment_email.py: New migration that ALTERs both enum types and INSERTs the "Monthly Fee — Prorated April" product (price_cents=0, one_time, tenant_id=1) with ON CONFLICT DO NOTHING for idempotency

Test Plan

• 60 related tests pass (admin, email_blast, contract, health)
• Pre-existing failures in TestCheckoutWebhook are on main, unrelated to this change
• Enum values verified via Python import: ProductCategory.monthly and EmailType.first_payment load correctly
• Migration is idempotent (ADD VALUE IF NOT EXISTS, ON CONFLICT DO NOTHING)

Review Checklist

• Models updated with new enum values
• Migration is idempotent and has downgrade
• ruff format and ruff check pass
• No route or service files touched (migration + models only)
• Tests pass (pre-existing webhook failures excluded)

Related Notes

None — pure schema/migration change.

Related

Closes #367

• Parent epic: #366 (First monthly payment email + Stripe checkout)
• Blocks: #368 (checkout endpoint), #369 (email + blast endpoint)

## Summary Adds the `monthly` ProductCategory and `first_payment` EmailType enum values needed by the first-payment checkout flow, and seeds a prorated April product. ## Changes - `src/basketball_api/models.py`: Added `monthly` to `ProductCategory` enum, `first_payment` to `EmailType` enum - `alembic/versions/037_add_monthly_category_and_first_payment_email.py`: New migration that ALTERs both enum types and INSERTs the "Monthly Fee — Prorated April" product (price_cents=0, one_time, tenant_id=1) with ON CONFLICT DO NOTHING for idempotency ## Test Plan - 60 related tests pass (admin, email_blast, contract, health) - Pre-existing failures in `TestCheckoutWebhook` are on main, unrelated to this change - Enum values verified via Python import: `ProductCategory.monthly` and `EmailType.first_payment` load correctly - Migration is idempotent (`ADD VALUE IF NOT EXISTS`, `ON CONFLICT DO NOTHING`) ## Review Checklist - [x] Models updated with new enum values - [x] Migration is idempotent and has downgrade - [x] ruff format and ruff check pass - [x] No route or service files touched (migration + models only) - [x] Tests pass (pre-existing webhook failures excluded) ## Related Notes None — pure schema/migration change. ## Related Closes #367 - Parent epic: #366 (First monthly payment email + Stripe checkout) - Blocks: #368 (checkout endpoint), #369 (email + blast endpoint)

forgejo_admin added 1 commit 2026-04-07 01:27:16 +00:00

feat: add monthly ProductCategory, first_payment EmailType, seed prorated product ... ece78b7baf

Migration 037 adds 'monthly' to ProductCategory enum, 'first_payment' to
EmailType enum, and seeds a "Monthly Fee — Prorated April" product with
price_cents=0 for the first-payment checkout flow.

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

forgejo_admin commented 2026-04-07 01:29:26 +00:00

Author

Owner

Copy link

PR #370 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Alembic (PostgreSQL)

This PR adds two enum values (ProductCategory.monthly, EmailType.first_payment) and a seed migration for a prorated April product. Two files changed, 37 additions, 0 deletions. Tight scope.

Migration correctness (037):

• ALTER TYPE ... ADD VALUE IF NOT EXISTS is the correct PostgreSQL pattern for enum extension. Good.
• down_revision = "036" matches the chain. Migration revision numbering is consistent with the project convention.
• Downgrade correctly deletes the seeded product row. PostgreSQL limitation on removing enum values is documented in the comment. Good.

ON CONFLICT DO NOTHING without a conflict target: The products table has no unique constraint on name or (tenant_id, name) -- only the auto-increment PK id. The bare ON CONFLICT DO NOTHING clause without a column target will only catch PK collisions (which cannot happen with auto-increment). This means the "idempotent" claim in the PR body and code comment is technically incorrect -- if this INSERT were somehow executed twice, it would create duplicate rows. In practice, Alembic migrations run exactly once, so this is not a real-world bug. However, adding ON CONFLICT (tenant_id, name) DO NOTHING (after adding a unique constraint) would make the idempotency claim truthful. Verdict: nit, not a blocker, since migrations are single-run by design.

Enum placement in models.py: first_payment is inserted between interest_notification and contract_offer, and monthly is appended after equipment. Both are clean insertions with no ordering issues.

Seed data: price_cents=0 is intentional for a prorated month. product_type='one_time' and category='monthly' are valid enum values. tenant_id=1 is hardcoded but consistent with the single-tenant pattern used throughout this codebase. description is nullable and correctly omitted.

BLOCKERS

None.

This is a pure enum extension + seed data migration. The "new functionality with zero test coverage" blocker does not apply here -- there is no new behavior to test. The enum values are schema-level additions consumed by downstream PRs (#368, #369) which will carry the behavioral tests.

NITS

1. Misleading idempotency comment: The migration comment says "Seed prorated April product (idempotent)" but ON CONFLICT DO NOTHING without a matching unique constraint does not provide true idempotency. Consider either (a) removing the "(idempotent)" claim, or (b) adding a conflict target like ON CONFLICT ON CONSTRAINT products_pkey DO NOTHING, or (c) using a WHERE NOT EXISTS subquery instead. This is a documentation accuracy issue, not a runtime bug.

2. No description for the seeded product: The product is inserted without a description. Downstream consumers (checkout page, email templates) may want a human-readable description. Consider whether this should be populated now or deferred to #368.

SOP COMPLIANCE

• Branch named after issue: 367-add-monthly-category-first-payment-email follows {issue-number}-{kebab-case-purpose} convention
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related references parent epic (#366) and downstream blockers (#368, #369)
• No secrets committed
• No scope creep -- only models.py and migration touched, exactly as described
• Related section does not reference a plan slug (marked "None" -- acceptable for a standalone migration ticket)

PROCESS OBSERVATIONS

• Clean decomposition: #367 (schema) blocks #368 (endpoint) and #369 (email). Each is independently reviewable and mergeable. Good DORA deployment frequency pattern.
• The PR correctly identifies pre-existing TestCheckoutWebhook failures on main as unrelated. This is good practice for avoiding false review blocks.
• Migration chain continuity (036 -> 037) should be verified at merge time to prevent head conflicts if other migration PRs land first.

VERDICT: APPROVED

## PR #370 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Alembic (PostgreSQL) This PR adds two enum values (`ProductCategory.monthly`, `EmailType.first_payment`) and a seed migration for a prorated April product. Two files changed, 37 additions, 0 deletions. Tight scope. **Migration correctness (037)**: - `ALTER TYPE ... ADD VALUE IF NOT EXISTS` is the correct PostgreSQL pattern for enum extension. Good. - `down_revision = "036"` matches the chain. Migration revision numbering is consistent with the project convention. - Downgrade correctly deletes the seeded product row. PostgreSQL limitation on removing enum values is documented in the comment. Good. **ON CONFLICT DO NOTHING without a conflict target**: The `products` table has no unique constraint on `name` or `(tenant_id, name)` -- only the auto-increment PK `id`. The bare `ON CONFLICT DO NOTHING` clause without a column target will only catch PK collisions (which cannot happen with auto-increment). This means the "idempotent" claim in the PR body and code comment is technically incorrect -- if this INSERT were somehow executed twice, it would create duplicate rows. In practice, Alembic migrations run exactly once, so this is not a real-world bug. However, adding `ON CONFLICT (tenant_id, name) DO NOTHING` (after adding a unique constraint) would make the idempotency claim truthful. **Verdict: nit, not a blocker**, since migrations are single-run by design. **Enum placement in models.py**: `first_payment` is inserted between `interest_notification` and `contract_offer`, and `monthly` is appended after `equipment`. Both are clean insertions with no ordering issues. **Seed data**: `price_cents=0` is intentional for a prorated month. `product_type='one_time'` and `category='monthly'` are valid enum values. `tenant_id=1` is hardcoded but consistent with the single-tenant pattern used throughout this codebase. `description` is nullable and correctly omitted. ### BLOCKERS None. This is a pure enum extension + seed data migration. The "new functionality with zero test coverage" blocker does not apply here -- there is no new behavior to test. The enum values are schema-level additions consumed by downstream PRs (#368, #369) which will carry the behavioral tests. ### NITS 1. **Misleading idempotency comment**: The migration comment says "Seed prorated April product (idempotent)" but `ON CONFLICT DO NOTHING` without a matching unique constraint does not provide true idempotency. Consider either (a) removing the "(idempotent)" claim, or (b) adding a conflict target like `ON CONFLICT ON CONSTRAINT products_pkey DO NOTHING`, or (c) using a `WHERE NOT EXISTS` subquery instead. This is a documentation accuracy issue, not a runtime bug. 2. **No `description` for the seeded product**: The product is inserted without a `description`. Downstream consumers (checkout page, email templates) may want a human-readable description. Consider whether this should be populated now or deferred to #368. ### SOP COMPLIANCE - [x] Branch named after issue: `367-add-monthly-category-first-payment-email` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references parent epic (#366) and downstream blockers (#368, #369) - [x] No secrets committed - [x] No scope creep -- only models.py and migration touched, exactly as described - [ ] Related section does not reference a plan slug (marked "None" -- acceptable for a standalone migration ticket) ### PROCESS OBSERVATIONS - Clean decomposition: #367 (schema) blocks #368 (endpoint) and #369 (email). Each is independently reviewable and mergeable. Good DORA deployment frequency pattern. - The PR correctly identifies pre-existing `TestCheckoutWebhook` failures on main as unrelated. This is good practice for avoiding false review blocks. - Migration chain continuity (036 -> 037) should be verified at merge time to prevent head conflicts if other migration PRs land first. ### VERDICT: APPROVED

forgejo_admin commented 2026-04-07 01:31:54 +00:00

Author

Owner

Copy link

PR #370 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic (PostgreSQL)

Two files changed, both expected:

• src/basketball_api/models.py -- adds monthly to ProductCategory enum, first_payment to EmailType enum
• alembic/versions/037_add_monthly_category_and_first_payment_email.py -- migration with ALTER TYPE ... ADD VALUE IF NOT EXISTS, product seed INSERT

Enum consistency: Python enum values ("monthly", "first_payment") match the SQL strings exactly. Correct.

Product seed values: tenant_id=1, name='Monthly Fee — Prorated April', price_cents=0, product_type='one_time', category='monthly', active=true. All match requirements.

Idempotency: ALTER TYPE ... ADD VALUE IF NOT EXISTS is genuinely idempotent. The ON CONFLICT DO NOTHING on the INSERT is technically a no-op because the products table has no unique constraint beyond the auto-increment PK -- see NITS below.

Migration chain: revision = "037", down_revision = "036". Main currently ends at 030. Migrations 031-036 presumably exist in other in-flight PRs/branches. This is fine as long as the merge order is coordinated.

Downgrade: Correctly deletes the seeded product row. Notes that PostgreSQL cannot remove enum values -- appropriate.

Scope: Only models and migration touched. No route or service files. Clean.

BLOCKERS

None.

No new functionality requiring test coverage (enum additions + data migration). No user input. No secrets. No auth changes. No DRY violations.

NITS

1. ON CONFLICT DO NOTHING without a conflict target is misleading: The products table has no unique constraint on (tenant_id, name) or any other column combination. The only unique column is the auto-increment id PK. This means ON CONFLICT DO NOTHING will never actually skip a duplicate -- it will always insert. In practice this is harmless because Alembic tracks revisions and will never re-run the migration, but calling it "idempotent" in the PR body is technically inaccurate. If true idempotency is desired, consider adding ON CONFLICT (id) DO NOTHING or better yet a unique constraint on (tenant_id, name) in a separate migration.

2. Migration numbering gap (030 -> 037): Six intermediate migrations (031-036) must be merged first or concurrently. If any of those fail to land, this migration will break the Alembic chain. Worth confirming the merge order.

SOP COMPLIANCE

• Branch named after issue (367-add-monthly-category-first-payment-email)
• PR body follows template (Summary, Changes, Test Plan, Related)
• Related references parent epic (#366) and downstream issues (#368, #369)
• No secrets committed
• No unrelated file changes
• Commit scope matches issue scope

PROCESS OBSERVATIONS

Clean, minimal migration PR. Good decomposition -- enum + seed separated from route work (#368) and email work (#369). The migration chain dependency (031-036 must land first) is the only coordination risk. Low change failure risk.

VERDICT: APPROVED

## PR #370 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic (PostgreSQL) Two files changed, both expected: - `src/basketball_api/models.py` -- adds `monthly` to `ProductCategory` enum, `first_payment` to `EmailType` enum - `alembic/versions/037_add_monthly_category_and_first_payment_email.py` -- migration with `ALTER TYPE ... ADD VALUE IF NOT EXISTS`, product seed INSERT **Enum consistency**: Python enum values (`"monthly"`, `"first_payment"`) match the SQL strings exactly. Correct. **Product seed values**: `tenant_id=1`, `name='Monthly Fee — Prorated April'`, `price_cents=0`, `product_type='one_time'`, `category='monthly'`, `active=true`. All match requirements. **Idempotency**: `ALTER TYPE ... ADD VALUE IF NOT EXISTS` is genuinely idempotent. The `ON CONFLICT DO NOTHING` on the INSERT is technically a no-op because the `products` table has no unique constraint beyond the auto-increment PK -- see NITS below. **Migration chain**: `revision = "037"`, `down_revision = "036"`. Main currently ends at `030`. Migrations 031-036 presumably exist in other in-flight PRs/branches. This is fine as long as the merge order is coordinated. **Downgrade**: Correctly deletes the seeded product row. Notes that PostgreSQL cannot remove enum values -- appropriate. **Scope**: Only models and migration touched. No route or service files. Clean. ### BLOCKERS None. No new functionality requiring test coverage (enum additions + data migration). No user input. No secrets. No auth changes. No DRY violations. ### NITS 1. **`ON CONFLICT DO NOTHING` without a conflict target is misleading**: The `products` table has no unique constraint on `(tenant_id, name)` or any other column combination. The only unique column is the auto-increment `id` PK. This means `ON CONFLICT DO NOTHING` will never actually skip a duplicate -- it will always insert. In practice this is harmless because Alembic tracks revisions and will never re-run the migration, but calling it "idempotent" in the PR body is technically inaccurate. If true idempotency is desired, consider adding `ON CONFLICT (id) DO NOTHING` or better yet a unique constraint on `(tenant_id, name)` in a separate migration. 2. **Migration numbering gap (030 -> 037)**: Six intermediate migrations (031-036) must be merged first or concurrently. If any of those fail to land, this migration will break the Alembic chain. Worth confirming the merge order. ### SOP COMPLIANCE - [x] Branch named after issue (`367-add-monthly-category-first-payment-email`) - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references parent epic (#366) and downstream issues (#368, #369) - [x] No secrets committed - [x] No unrelated file changes - [x] Commit scope matches issue scope ### PROCESS OBSERVATIONS Clean, minimal migration PR. Good decomposition -- enum + seed separated from route work (#368) and email work (#369). The migration chain dependency (031-036 must land first) is the only coordination risk. Low change failure risk. ### VERDICT: APPROVED

forgejo_admin merged commit 841397fe5b into main 2026-04-07 01:33:48 +00:00

forgejo_admin deleted branch 367-add-monthly-category-first-payment-email 2026-04-07 01:33:48 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-07 01:33:50 +00:00

feat: add monthly ProductCategory, first_payment EmailType, seed prorated product (#370)

forgejo_admin referenced this pull request 2026-04-07 01:38:15 +00:00

Checkout endpoint: GET /checkout/first-payment with Stripe redirect #368

forgejo_admin referenced this pull request 2026-04-07 01:38:43 +00:00

Email function + blast endpoint: send_first_payment_email + POST /admin/email/first-payment #369

forgejo_admin referenced this pull request 2026-04-07 01:38:57 +00:00

Checkout endpoint: GET /checkout/first-payment with Stripe redirect #368

forgejo_admin referenced this pull request 2026-04-07 17:13:04 +00:00

feat: add GET /checkout/first-payment endpoint with Stripe redirect #375

forgejo_admin referenced this pull request 2026-04-07 17:17:09 +00:00

feat: add GET /checkout/first-payment endpoint with Stripe redirect #375

forgejo_admin referenced this pull request from a commit 2026-04-07 18:15:25 +00:00

fix: add first_payment to EmailType test expected values

forgejo_admin referenced this pull request 2026-04-07 18:15:49 +00:00

Bug: CI broken — test_all_expected_values missing first_payment EmailType #381

forgejo_admin referenced this pull request 2026-04-07 18:15:56 +00:00

fix: add first_payment to EmailType test expected values #382

forgejo_admin referenced this pull request 2026-04-07 18:17:25 +00:00

fix: add first_payment to EmailType test expected values #382

forgejo_admin referenced this pull request 2026-04-07 19:15:01 +00:00

Bug: Migration 037 fails on fresh deploy — enum ADD VALUE in same transaction as INSERT #384

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

No description provided.