feat(cancelled-bookings): implement cancelled bookings functionality …#56
Merged
Conversation
…with archiving and migration
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThe PR introduces a ChangesBooking Cancellation Archival
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🧹 Nitpick comments (2)
src/migrations/1780002000000-CreateCancelledBookingsTable.ts (1)
25-33: 💤 Low valueConsider adding indexes on
original_booking_idandcancelled_at.The migration creates indexes on
field_id,user_id, andslot_id, which are good for foreign key lookups. However, two additional indexes might improve query performance:
original_booking_id: Useful for audit queries that trace back to the original booking record.cancelled_at: Useful for time-based reporting queries (e.g., "cancelled bookings in the last month").📋 Proposed addition
await queryRunner.query( `CREATE INDEX IF NOT EXISTS "IDX_cancelled_bookings_slot_id" ON "cancelled_bookings" ("slot_id")`, ); + await queryRunner.query( + `CREATE INDEX IF NOT EXISTS "IDX_cancelled_bookings_original_booking_id" ON "cancelled_bookings" ("original_booking_id")`, + ); + await queryRunner.query( + `CREATE INDEX IF NOT EXISTS "IDX_cancelled_bookings_cancelled_at" ON "cancelled_bookings" ("cancelled_at")`, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/migrations/1780002000000-CreateCancelledBookingsTable.ts` around lines 25 - 33, Add two additional CREATE INDEX statements via queryRunner.query in the migration to index original_booking_id and cancelled_at; specifically, add queries similar to the existing ones (use queryRunner.query) that create "IDX_cancelled_bookings_original_booking_id" on "cancelled_bookings" ("original_booking_id") and "IDX_cancelled_bookings_cancelled_at" on "cancelled_bookings" ("cancelled_at") so audit lookups and time-based queries benefit—place them alongside the existing index creations in the same migration file.src/booking/booking.service.ts (1)
152-164: 💤 Low valueDefensive check for
status <> 'cancelled'bookings.This pessimistic lock check excludes bookings with
status = 'cancelled'before creating a new booking. However, the new archival flow deletes cancelled bookings from the table (line 579), so ideally there should be no cancelled-status bookings remaining.This check appears defensive for a transition period where old cancelled bookings might still exist. If that's the intent, consider:
- Ensuring a data migration moves all existing
status = 'cancelled'bookings tocancelled_bookingstable (see comment on migration file).- After the transition period, this condition could be simplified to just check for existence of any booking for the slot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/booking/booking.service.ts` around lines 152 - 164, The current pessimistic lock query in bookingRepo.createQueryBuilder("booking") filters out bookings with status = 'cancelled', which is only needed during the archival transition; ensure your migration moves all existing cancelled bookings into the cancelled_bookings archive so cancelled rows no longer exist, and once migration/archival (the flow that deletes cancelled rows) is complete, simplify the check in bookingRepo (the activeBooking lookup) to just test for any existing booking for the slot and remove the "status <> :cancelled" clause and its parameter; keep throwing ConflictException("Slot already has an active booking") when any booking exists during the interim, and add a TODO comment referencing the migration for cleanup so the simplification can be applied later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/booking/entities/cancelled-booking.entity.ts`:
- Around line 84-85: The CancelledBooking entity's createdAt property uses
`@CreateDateColumn` which causes TypeORM to auto-generate a new timestamp and
overwrite copied values (the archival assignment createdAt: booking.createdAt in
booking.service.ts and membership-plan.controller.ts). Replace the
`@CreateDateColumn` on the createdAt property in the CancelledBooking entity with
a regular `@Column` configured for a timestamp/Date (and update imports
accordingly) so saved archived entities retain the original booking.createdAt
value. Ensure the property name remains createdAt and the column option name
"created_at" is preserved.
- Around line 24-40: The CancelledBooking entity currently uses onDelete:
"CASCADE" on the Field, FieldSlot and UserAccount relations (symbols:
CancelledBooking, field, slot, user, fieldId, slotId, userId) which will delete
archival rows; change this to preserve history by replacing onDelete: "CASCADE"
with onDelete: "SET NULL" (or remove the relationship entirely if you prefer a
pure snapshot), make the corresponding FK columns (fieldId, slotId, userId)
nullable and adjust the relation properties (field?, slot?, user?) to be
optional so deletes of Field/FieldSlot/UserAccount null out the FKs instead of
removing CancelledBooking records. Ensure DB column definitions and entity types
are updated to reflect nullable FKs and use SET NULL behavior.
In `@src/migrations/1780002000000-CreateCancelledBookingsTable.ts`:
- Around line 5-23: The migration creates the "cancelled_bookings" table but
omits foreign key constraints for field_id, slot_id, and user_id; modify the
migration (CreateCancelledBookingsTable) to add explicit FOREIGN KEY constraints
referencing the appropriate parent tables (e.g., fields(id), slots(id),
users(id)) after the CREATE TABLE (using queryRunner.query to ALTER TABLE ...
ADD CONSTRAINT ... FOREIGN KEY ... ON DELETE CASCADE or the chosen action), and
update the down() method to DROP those constraints before dropping the
"cancelled_bookings" table so referential integrity is enforced and the rollback
removes the FK constraints first.
---
Nitpick comments:
In `@src/booking/booking.service.ts`:
- Around line 152-164: The current pessimistic lock query in
bookingRepo.createQueryBuilder("booking") filters out bookings with status =
'cancelled', which is only needed during the archival transition; ensure your
migration moves all existing cancelled bookings into the cancelled_bookings
archive so cancelled rows no longer exist, and once migration/archival (the flow
that deletes cancelled rows) is complete, simplify the check in bookingRepo (the
activeBooking lookup) to just test for any existing booking for the slot and
remove the "status <> :cancelled" clause and its parameter; keep throwing
ConflictException("Slot already has an active booking") when any booking exists
during the interim, and add a TODO comment referencing the migration for cleanup
so the simplification can be applied later.
In `@src/migrations/1780002000000-CreateCancelledBookingsTable.ts`:
- Around line 25-33: Add two additional CREATE INDEX statements via
queryRunner.query in the migration to index original_booking_id and
cancelled_at; specifically, add queries similar to the existing ones (use
queryRunner.query) that create "IDX_cancelled_bookings_original_booking_id" on
"cancelled_bookings" ("original_booking_id") and
"IDX_cancelled_bookings_cancelled_at" on "cancelled_bookings" ("cancelled_at")
so audit lookups and time-based queries benefit—place them alongside the
existing index creations in the same migration file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeeb0a5c-4d8a-4fea-8ef6-4e800f54da52
📒 Files selected for processing (7)
src/booking/booking.module.tssrc/booking/booking.service.tssrc/booking/entities/cancelled-booking.entity.tssrc/booking/membership-plan.controller.tssrc/data-source.tssrc/migrations/1780001000000-MakeBookingsSlotIdPartialUnique.tssrc/migrations/1780002000000-CreateCancelledBookingsTable.ts
added 2 commits
May 15, 2026 15:22
…ion for nullable fields and foreign key constraints
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
1 participant
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.
…with archiving and migration
Summary by CodeRabbit
Release Notes
New Features
Improvements