fix: correct cleanup logic for unsubmitted forms#14
Conversation
WalkthroughRefactors unsubmitted-forms cleanup to iterate per-token with detailed logging and token masking, strict "older than 7 days" filtering, selective field retrieval, guarded handling for missing entityId, per-token transactional deletions (corpus, relationships, tokens, conditional entity delete), expanded relationship criteria, resilient per-token error handling, and final job status and completion log. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Job as Cleanup Job
participant DB as Database
participant Log as Logger
Note over Job,Log: Startup
Job->>Log: log start (masked tokens)
Job->>DB: Query tokens older than 7 days (select token, entityId, productId)
DB-->>Job: Token list
loop For each token
Job->>Log: log token discovered (masked)
Job->>Job: try per-token processing
alt entityId missing
Job->>DB: Delete token
Job->>Log: log token-only deletion
else entityId present
Job->>DB: Query incomplete relationships (entity_id, status in [new,draft,in_progress])
alt relationships found
rect rgba(220,245,220,0.6)
Note right of Job: Per-token transaction
Job->>DB: Delete corpus items for entity
Job->>DB: Delete incomplete relationships for entity
Job->>DB: Delete tokens for entity
Job->>DB: Delete entity if no relationships remain
end
Job->>Log: log cleanup success
else no relationships
Job->>DB: Delete stale token
Job->>Log: log no-relationship token deletion
end
end
opt error
Job->>Log: log error (does not abort loop)
end
end
Note over Job,Log: Completion
Job->>Log: log completion with processed count
Job->>DB: Update job status (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (3)
54-105: Isolate per-token failures so one bad record doesn’t abort the entire job.
Wrap the per-token work in try/catch and continue.Apply this diff:
- for (const token of expiredTokens) { + for (const token of expiredTokens) { + try { // ISSUE 2: Limited status checking // ORIGINAL: Only checks for "new" status // PROBLEM: Misses forms with other incomplete statuses like "draft", "in_progress" // FIX: Check for all possible incomplete statuses - + const relationship = await prisma.relationship.findFirst({ where: { product_id: token.productId, // ORIGINAL (flawed): status: "new", // FIXED: Include all incomplete form statuses status: { in: ["new", "draft", "in_progress"] }, }, }); if (relationship) { // ISSUE 3: Incorrect deletion order (potential foreign key constraints) // ORIGINAL ORDER: relationship → token → corpus → entity // PROBLEM: May violate foreign key constraints if entities are deleted before dependencies // FIX: Delete in reverse dependency order (dependencies first, parent records last) - + await prisma.$transaction([ // FIXED ORDER 1: Delete corpus items first (dependencies of entity) prisma.new_corpus.deleteMany({ where: { entity_id: token.entityId || "", }, }), // FIXED ORDER 2: Delete relationship (depends on product_id and entity) prisma.relationship.delete({ where: { id: relationship.id }, }), // FIXED ORDER 3: Delete the token (depends on productId and entityId) prisma.publicFormsTokens.delete({ where: { token: token.token }, }), // FIXED ORDER 4: Delete entity last (parent record) prisma.entity.delete({ where: { id: token.entityId || "" }, }), ]); // ISSUE 4: Missing logging for debugging and monitoring // ORIGINAL: No progress logging // FIX: Add informative logging console.log(`✅ Cleaned up unsubmitted form for entity: ${token.entityId}`); } else { // ORIGINAL: No logging for skipped tokens // FIX: Add logging for transparency - console.log(`➖ No incomplete relationship found for token: ${token.token}`); + console.log(`➖ No incomplete relationship found for token: ${token.token}`); } - } + } catch (e) { + console.error(`⚠️ Failed to clean token ${token.token}`, e); + } + }Note: Combine with the transactional fix suggested above to remove the empty-string fallbacks.
43-52: Reduce payload: select only needed fields.
Prevents loading unused columns and improves throughput.Apply this diff:
- const expiredTokens = await prisma.publicFormsTokens.findMany({ - where: { - createdAt: { - // ORIGINAL (flawed): gte: sevenDaysAgo, lt: sevenDaysAgoPlusOneDay - // PROBLEM: This creates a 24-hour window for exactly 7-day-old tokens only - // FIXED: Use 'lt' to get ALL tokens older than 7 days - lt: sevenDaysAgo, // LESS THAN 7 days ago (includes all older tokens) - }, - }, - }); + const expiredTokens = await prisma.publicFormsTokens.findMany({ + where: { + createdAt: { lt: sevenDaysAgo }, + }, + select: { token: true, entityId: true, productId: true }, + });If large volumes are expected, consider paginating with cursor + take to avoid long transactions.
43-52: Consider indexes to keep the job fast at scale.
- Index publicFormsTokens.createdAt for the lt scan.
- Index relationship on (entity_id, status).
- Index publicFormsTokens on (entityId).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
🔇 Additional comments (3)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts (3)
41-41: Seven-days-ago calculation is now correct — LGTM.
46-50: Older-than-7-days filter is correct — LGTM.
Switching to lt: sevenDaysAgo fixes the “exactly 7 days” bug.
60-66: Verify Prisma schema definitions
Ensure:
- the
Statusenum in your Prisma schema includesnew,draft, andin_progress- the
Relationshipmodel’s fields match your code (productIdvs.product_id,entityIdvs.entity_id)- the Tokens model defines
productIdandentityIdexactly as used here
|
Hi @GoffAC , could you please review this fix for the unsubmitted forms cleanup logic? Summary of changes:
Looking forward to your review! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/unsubmitted_forms/cleanup_unsubmitted_forms.ts(1 hunks)
1 participant
Description
Fixes critical issues in the scheduled job that cleans up unsubmitted forms older than 7 days.
Changes Made
Critical Fixes
Impact
Testing
Related Issues
Fixes logic errors in form cleanup scheduling
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Chores