fix(issue-143): add CHECK constraint enforcing AFTER_TTL ↔ cardTtlMinutes invariant#147
Merged
Merged
Conversation
…utes Adds a Postgres CHECK constraint so the database itself rejects any User row where cancelPolicy = AFTER_TTL but cardTtlMinutes is NULL or non-positive. This is defense-in-depth on top of the application-level handling added in PR #136 — it closes the gap that any future code path writing User directly (scripts, ad-hoc updates, new endpoints, typos in another module) could re-introduce the broken state. The migration backfills any existing AFTER_TTL rows missing a TTL with the 60-minute default already used by the Telegram menu, so the migration is safe to run on existing data. Closes #143
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA database migration backfills ChangesData Integrity Enforcement
Sequence Diagram(s)(Skipped — change is a DB constraint + tests; does not introduce multi-component control flow requiring a sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@prisma/migrations/20260503000000_add_user_after_ttl_check_constraint/migration.sql`:
- Around line 16-19: The DB CHECK currently only enforces that WHEN
"cancelPolicy" = 'AFTER_TTL' then "cardTtlMinutes" must be non-null and >0;
update the constraint to make the relationship bidirectional so non-'AFTER_TTL'
rows cannot have a TTL either. Modify the CHECK in the migration that references
"cancelPolicy" and "cardTtlMinutes" so it requires either (cancelPolicy =
'AFTER_TTL' AND cardTtlMinutes IS NOT NULL AND cardTtlMinutes > 0) OR
(cancelPolicy <> 'AFTER_TTL' AND cardTtlMinutes IS NULL), matching the API
validation in src/api/routes/users.ts.
In `@tests/integration/db/userAfterTtlConstraint.test.ts`:
- Around line 94-104: Add create-path tests that assert constraint violations
when CardCancelPolicy is AFTER_TTL and cardTtlMinutes is non-positive: add at
least two cases calling prisma.user.create (like the existing test using
expectConstraintViolation) with cardTtlMinutes set to 0 and to a negative value,
keeping the same structure and using CardCancelPolicy.AFTER_TTL so the migration
rule is fully covered for create paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf2164b0-9c80-45aa-b654-47746459100a
📒 Files selected for processing (2)
prisma/migrations/20260503000000_add_user_after_ttl_check_constraint/migration.sqltests/integration/db/userAfterTtlConstraint.test.ts
📝 CodeRabbit Chat: Implement requested code changes
JonasBaeumer
approved these changes
May 3, 2026
JonasBaeumer
left a comment
JonasBaeumer
left a comment
Owner
There was a problem hiding this comment.
LGTM, please check the PR review comments and fix the CI issues before merge
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/db/userAfterTtlConstraint.test.ts`:
- Around line 15-48: This test file uses the real Prisma client (prisma) and
performs live DB operations via createUser()/prisma.user.create,
expectConstraintViolation(await ...) assertions, and cleanup in afterAll
(prisma.user.deleteMany, prisma.$disconnect), violating the rule that *.test.ts
must mock external deps; replace the direct prisma import with a mocked Prisma
client and stub prisma.user.create, prisma.user.update, prisma.user.deleteMany
and prisma.$disconnect (or wrap createUser to call an injected client) so tests
run purely in-memory; keep the same helpers (createUser,
expectConstraintViolation, createdUserIds, CONSTRAINT_NAME) but have them
operate against the mocked methods and ensure createdUserIds is cleared between
tests rather than performing real deletes.
- Around line 119-156: The first test case inside the describe('accepts valid
states') block has an await call to createUser run directly in the describe
scope; wrap that block in an async it(...) (e.g. it('accepts valid AFTER_TTL
state', async () => { const user = await createUser({...}); expect(...); }));
keep the existing assertions and references to CardCancelPolicy and createUser
unchanged; also ensure subsequent async tests (which call prisma.user.update)
remain async. Finally, because these tests use a live Prisma client
(prisma.user.update/create), either mock Prisma in this test (replace real calls
with mocked implementations) or move the file to an integration test location
and rename it so it isn’t treated as a unit test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1bb0334-1e39-4081-8e6d-78a22824e0a5
📒 Files selected for processing (1)
tests/integration/db/userAfterTtlConstraint.test.ts
Wrap the AFTER_TTL positive TTL case in an it() block so the integration test is syntactically valid and actually executes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/db/userAfterTtlConstraint.test.ts (1)
15-48:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
*.test.tsguideline conflict: this suite performs live DB I/O via Prisma.Line 15 imports the real client, and Line 26/59/73/87/96/108/147 plus Line 45/47 execute real DB operations. That conflicts with the repository rule for
**/*.test.tsfiles requiring mocked external dependencies and no database/network access.As per coding guidelines,
**/*.test.ts: Every module must have unit tests with mocked external dependencies and no database/network access.Also applies to: 50-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/db/userAfterTtlConstraint.test.ts` around lines 15 - 48, This test imports the real Prisma client (prisma) and performs live DB I/O in createUser, expectConstraintViolation, and afterAll (prisma.user.create, prisma.user.deleteMany, prisma.$disconnect), which violates the repo rule that **/*.test.ts must mock external dependencies; replace the real client with a mocked test double: update the import of prisma to a mocked version (or use jest.mock/vi.mock) and stub prisma.user.create, prisma.user.deleteMany and prisma.$disconnect to return controlled values/errors, refactor createUser to call the mocked prisma, and ensure expectConstraintViolation asserts against the mocked error containing CONSTRAINT_NAME; alternatively move this file to an integration test folder and keep the real client if integration tests are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/db/userAfterTtlConstraint.test.ts`:
- Around line 119-154: Add a new acceptance test in
tests/integration/db/userAfterTtlConstraint.test.ts that covers the atomic
transition into AFTER_TTL with a positive TTL: create an initial user using
createUser with a non-AFTER_TTL policy and cardTtlMinutes null, then perform a
single prisma.user.update to set cancelPolicy to CardCancelPolicy.AFTER_TTL and
cardTtlMinutes to a positive integer, and assert the returned record has
cancelPolicy === CardCancelPolicy.AFTER_TTL and cardTtlMinutes === that integer;
place this alongside the existing tests that use createUser and
prisma.user.update so it mirrors the symmetric case already covered.
---
Duplicate comments:
In `@tests/integration/db/userAfterTtlConstraint.test.ts`:
- Around line 15-48: This test imports the real Prisma client (prisma) and
performs live DB I/O in createUser, expectConstraintViolation, and afterAll
(prisma.user.create, prisma.user.deleteMany, prisma.$disconnect), which violates
the repo rule that **/*.test.ts must mock external dependencies; replace the
real client with a mocked test double: update the import of prisma to a mocked
version (or use jest.mock/vi.mock) and stub prisma.user.create,
prisma.user.deleteMany and prisma.$disconnect to return controlled
values/errors, refactor createUser to call the mocked prisma, and ensure
expectConstraintViolation asserts against the mocked error containing
CONSTRAINT_NAME; alternatively move this file to an integration test folder and
keep the real client if integration tests are allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce8a40ad-6c47-4e20-b85d-654abbc26d2b
📒 Files selected for processing (1)
tests/integration/db/userAfterTtlConstraint.test.ts
JonasBaeumer
approved these changes
May 6, 2026
Symmetric to the existing 'switch away from AFTER_TTL' test, this asserts the CHECK constraint accepts an atomic update that flips cancelPolicy to AFTER_TTL while setting a positive cardTtlMinutes in the same statement. Pins the intended behaviour against future migrations or trigger-based rewrites that might evaluate the constraint mid-update.
Collaborator
Author
|
Addressed the remaining CodeRabbit threads in 6fde9b0:
All four threads resolved. CI green: Lint, Type Check, Unit (392), Integration (incl. the new test, 51s), CodeQL. |
Collaborator
Author
|
@JonasBaeumer this PR is ready from my side: approved, all checks green, and all review threads resolved. is blocked for me by the base-branch policy, so it needs a maintainer/admin merge when you have a minute. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #143. Follow-up to #89 / PR #136.
Summary
Adds a Postgres
CHECKconstraint on theUsertable so the database itself rejects any row wherecancelPolicy = 'AFTER_TTL'butcardTtlMinutesisNULLor non-positive.PR #136 closed the application-level loopholes (the Telegram menu now clears the TTL on policy switch, and the orchestrator falls back to
IMMEDIATEcancellation if it ever encounters the broken state at checkout time). What was still missing was a database-level guard — any future code path that writesUserdirectly (a script, a migration, a new endpoint, a typo in another module) could silently re-introduce the broken state.This PR adds that belt-and-braces guard. The runtime fallback from PR #136 is intentionally untouched — the constraint is defense-in-depth, not a replacement.
Changes
prisma/migrations/20260503000000_add_user_after_ttl_check_constraint/migration.sqlAFTER_TTLrows missing a TTL with60minutes (the same default the Telegram menu uses), so the migration is safe to run on existing data instead of failing on a constraint violation.user_after_ttl_requires_cardttlminutesCHECKconstraint:tests/integration/db/userAfterTtlConstraint.test.ts— new integration test that pins the constraint behaviour:AFTER_TTLwhilecardTtlMinutesisNULL, that clearcardTtlMinuteson anAFTER_TTLuser, and that setcardTtlMinutesto0on anAFTER_TTLuser.AFTER_TTL+NULLcardTtlMinutes.AFTER_TTLwith a positive TTL, all non-AFTER_TTLpolicies withNULLTTL, and atomic policy-and-TTL updates that keep the invariant satisfied.schema.prismais intentionally unchanged — Prisma 5 does not modelCHECKconstraints, so the migration is the canonical source.Acceptance criteria (from #143)
AFTER_TTLrow missing a TTL with the 60-minute default).prisma.user.update({ ... cancelPolicy: 'AFTER_TTL', cardTtlMinutes: null })rejects with a constraint-violation error (plus the create path and the0/ clear-TTL paths).src/orchestrator/intentService.tsandsrc/telegram/menuHandler.ts.Test plan
Other pre-existing integration failures observed locally (e.g.
tests/integration/stripe/reconciliation.test.ts, tracked in #88) are unrelated to this change and reproduce onmainwithout this branch applied.Summary by CodeRabbit
Bug Fixes
Tests