Feature/on delete edit functionality#55
Merged
Merged
Conversation
added 5 commits
May 12, 2026 17:27
…nd bearer auth; enhance effective date handling in FieldsService and migrations
…rule_book_history
… and rule books; add corresponding API documentation
📝 WalkthroughWalkthroughThis PR adds rule-book versioning and history, a new FieldRuleBookHistory entity and migrations, soft-delete endpoints for fields/rule-books/memberships, refactors slot sync to resolve prices from effective rule versions, updates DTOs and docs (bulk booking payload now includes userName/phoneNumber), and wires the new entity into modules and the data source. ChangesSoft-delete endpoints & membership cancellation
Rule-book versioning + history
Slot price resolution using effective versions
Infrastructure wiring & data-source
Bulk booking contract & docs updates
Sequence DiagramsequenceDiagram
participant Client
participant FieldsController
participant FieldsService
participant FieldSlotSyncService
participant Database
Client->>FieldsController: PATCH /fields/:fieldId/rule-books/:ruleBookId
FieldsController->>FieldsService: updateFieldRuleBook(account, fieldId, ruleBookId, dto)
FieldsService->>FieldsService: validate appliedEffectiveDate >= previousEffectiveDate
FieldsService->>Database: BEGIN TRANSACTION
FieldsService->>Database: UPSERT FieldRuleBookHistory (previous version)
FieldsService->>Database: UPDATE field_rule_books SET effective_date...
FieldsService->>Database: COMMIT
FieldsService->>FieldSlotSyncService: syncFieldDate for affected dates
FieldSlotSyncService->>Database: SELECT histories <= slotDate for ruleBookIds
FieldSlotSyncService->>FieldSlotSyncService: resolveRuleBooksForDate -> choose latest version per ruleBookId
FieldSlotSyncService->>Database: UPDATE slot prices/status based on resolved versions
alt Sync succeeds
FieldsService-->>Client: { success: true, slotsUpdated: true }
else Sync fails
FieldsService-->>Client: { success: true, slotsUpdated: false, message: "..." }
end
sequenceDiagram
participant Admin
participant FieldsController
participant FieldsService
participant Database
Admin->>FieldsController: DELETE /fields/:fieldId
FieldsController->>FieldsService: deleteField(account, fieldId)
FieldsService->>Database: BEGIN TRANSACTION (pessimistic lock)
FieldsService->>Database: COUNT active fields for owner
alt Enough active fields remain
FieldsService->>Database: UPDATE fields SET isActive=false WHERE id=...
FieldsService->>Database: UPDATE field_rule_books SET isActive=false WHERE fieldId=...
FieldsService->>Database: UPDATE membership_plans SET isActive=false, endDate=TODAY
FieldsService->>Database: COMMIT
FieldsService-->>Admin: { success: true, fieldId, message }
else Not allowed (would remove last active field)
FieldsService->>Database: ROLLBACK
FieldsService-->>Admin: { statusCode: 400, message }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fields/fields.service.ts (1)
1017-1075:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLock and re-read the rule book inside the transaction.
Line 1017 loads state before the transaction, then Line 1051+ versions/saves it later. Concurrent updates can snapshot the same prior state and produce lost updates or incorrect history rows.
Suggested direction
- const existingRuleBook = await this.fieldRuleBooksRepository.findOne({ - where: { id: ruleBookId, fieldId }, - }); + // Keep only existence checks outside if needed; load mutable state inside TX with lock. ... - const savedRuleBook = - await this.fieldRuleBooksRepository.manager.transaction( + const savedRuleBook = + await this.fieldRuleBooksRepository.manager.transaction( async (manager) => { const ruleBookRepo = manager.getRepository(FieldRuleBook); const historyRepo = manager.getRepository(FieldRuleBookHistory); + const lockedRuleBook = await ruleBookRepo + .createQueryBuilder("ruleBook") + .setLock("pessimistic_write") + .where("ruleBook.id = :ruleBookId AND ruleBook.field_id = :fieldId", { ruleBookId, fieldId }) + .getOne(); + + if (!lockedRuleBook) { + throw new NotFoundException("Rule book not found"); + } + + const previousEffectiveDate = + lockedRuleBook.effectiveDate ?? + lockedRuleBook.createdAt.toISOString().split("T")[0]; await historyRepo.upsert( { - ruleBookId: existingRuleBook.id, + ruleBookId: lockedRuleBook.id, effectiveFromDate: previousEffectiveDate, - ruleName: existingRuleBook.ruleName, + ruleName: lockedRuleBook.ruleName, ... }, ["ruleBookId", "effectiveFromDate"], ); - existingRuleBook.ruleName = normalizedRuleBook.ruleName; + lockedRuleBook.ruleName = normalizedRuleBook.ruleName; ... - return ruleBookRepo.save(existingRuleBook); + return ruleBookRepo.save(lockedRuleBook); }, );🤖 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/fields/fields.service.ts` around lines 1017 - 1075, You load existingRuleBook before starting the transaction which allows concurrent updates to cause lost updates; move the read/lock into the transaction callback: inside the fieldRuleBooksRepository.manager.transaction callback use manager.getRepository(FieldRuleBook) to re-fetch the rule book by id and fieldId with a row lock (pessimistic_write/SELECT ... FOR UPDATE), then read that locked instance to create the history entry in FieldRuleBookHistory and apply normalizedRuleBook changes to that same instance before saving via ruleBookRepo.save; keep existing usage of normalizeCreateFieldRuleBookInput, appliedEffectiveDate logic, and historyRepo.upsert but base them on the re-read locked entity instead of the pre-transaction existingRuleBook.
🤖 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 `@ApiDocs/HelloFutsal/environments/Dev.bru`:
- Line 4: The Dev.bru environment file contains a committed bearer token under
the key "token"; remove the token value and replace it with a non-secret
placeholder (e.g. TOKEN_PLACEHOLDER) in the Dev.bru file, add Dev.bru (or the
environment files pattern) to .gitignore, rotate/revoke the exposed JWT
immediately, and purge the secret from repository history using a tool like git
filter-repo or BFG to remove the leaked value from past commits.
In `@ApiDocs/HelloFutsal/Rule` Book/DeleteRuleBook/DeleteRuleBook.bru:
- Line 8: The URL uses the templated variable {{ruleBookId}} which does not
match the environment variable name rulebookId; update the URL template to use
{{rulebookId}} (and any other occurrences such as the second instance noted) so
the request uses the same variable name as the environment and will resolve
correctly.
In `@BULK_BOOKING_API.md`:
- Around line 143-149: Remove the contradictory "409 Conflict" responses that
state "User with this phone number already exists" and instead keep the single
contract that describes the upsert/reuse behavior; specifically delete the JSON
blocks under the "409 Conflict" header(s) that return statusCode 409 and that
message (the repeated blocks referenced in the comment) so the docs reflect that
existing users are reused rather than treated as an error, and ensure only the
upsert/reuse response is documented across the file.
- Around line 9-11: The fenced code block containing the endpoint "POST
/bookings/bulk/time-range" lacks a language hint; edit that fenced block and
change the opening backticks to include the language token (e.g., add "http" so
it becomes ```http) so markdownlint MD040 is satisfied and the block is properly
highlighted.
In `@README.md`:
- Around line 97-98: Add a dedicated "⚠️ DANGER: Production Database Reset"
section around the RESET_PROD_CONFIRM=RESET_PROD npm run db:reset:prod command:
move the command out of the main flow, prepend multiple explicit warnings about
irreversible data loss, list approved scenarios when it may be used and when it
must not be used, require documented backups and explicit authorization steps
(who must sign off) before running, and note that this command should ideally
live only in internal operations runbooks (or be omitted from public README)
rather than the main README.
In `@src/fields/fields.service.ts`:
- Around line 1129-1162: Move the "at least one active field" check inside the
same transaction that deactivates the field so the check and updates are atomic:
inside the existing this.fieldsRepository.manager.transaction callback, use the
transaction manager (manager) to query/count active fields for the owner (e.g.,
via manager.getRepository(Field).createQueryBuilder("f").where("f.ownerId =
:ownerId", { ownerId: account.id }).andWhere("f.isActive =
true").setLock("pessimistic_write") ) and evaluate the count there, throwing the
BadRequestException from inside the transaction if count <= 1; then proceed to
update Field, FieldRuleBook, and MembershipPlan using the same transaction
manager and include ownerId in the update predicates where appropriate to ensure
the lock/ownership is respected.
- Around line 1187-1195: deleteRuleBook currently deactivates the rule via
fieldRuleBooksRepository.update but never triggers a slot resync, leaving
upcoming slots with stale prices; after the update in deleteRuleBook (use the
existing ruleBook variable and its fieldId), call and await the slot
synchronization routine (e.g.,
this.slotService.resyncSlotsForField(ruleBook.fieldId) or the project's
equivalent method such as syncSlotsForField/resyncSlots) and handle errors (log
and/or propagate) so slots for that field are immediately recalculated before
returning the success response.
---
Outside diff comments:
In `@src/fields/fields.service.ts`:
- Around line 1017-1075: You load existingRuleBook before starting the
transaction which allows concurrent updates to cause lost updates; move the
read/lock into the transaction callback: inside the
fieldRuleBooksRepository.manager.transaction callback use
manager.getRepository(FieldRuleBook) to re-fetch the rule book by id and fieldId
with a row lock (pessimistic_write/SELECT ... FOR UPDATE), then read that locked
instance to create the history entry in FieldRuleBookHistory and apply
normalizedRuleBook changes to that same instance before saving via
ruleBookRepo.save; keep existing usage of normalizeCreateFieldRuleBookInput,
appliedEffectiveDate logic, and historyRepo.upsert but base them on the re-read
locked entity instead of the pre-transaction existingRuleBook.
🪄 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: 61e0d731-330f-49e9-bb07-038ffb2998af
📒 Files selected for processing (26)
ApiDocs/HelloFutsal/Booking/BulkBookSlots.bruApiDocs/HelloFutsal/Create Field/DeleteField/DeleteField.bruApiDocs/HelloFutsal/MembershipSlots/DeleteMembership/DeleteMembership.bruApiDocs/HelloFutsal/Rule Book/CreateFieldRuleBook.bruApiDocs/HelloFutsal/Rule Book/DeleteRuleBook/DeleteRuleBook.bruApiDocs/HelloFutsal/Rule Book/EditRuleBook/EditTimeRange.bruApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookAllSlots.bruApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookSpecificSlots.bruApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookTimeRange.bruApiDocs/HelloFutsal/VerifyOTP/Admin OTP Verify.bruApiDocs/HelloFutsal/environments/Dev.bruBULK_BOOKING_API.mdREADME.mdsrc/app.module.tssrc/booking/membership-plan.controller.tssrc/data-source.tssrc/fields/cron/field-slot-generator.tssrc/fields/cron/field-slot-sync.service.tssrc/fields/dto/create-field-rule-book.dto.tssrc/fields/entities/field-rule-book-history.entity.tssrc/fields/entities/field-rule-book.entity.tssrc/fields/fields.controller.tssrc/fields/fields.module.tssrc/fields/fields.service.tssrc/migrations/1780000000004-CreateFieldRuleBookHistory.tssrc/migrations/1780000000005-AddEffectiveDateAndVersionFieldsToRuleBooks.ts
💤 Files with no reviewable changes (4)
- ApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookAllSlots.bru
- ApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookTimeRange.bru
- ApiDocs/HelloFutsal/Booking/BulkBookSlots.bru
- ApiDocs/HelloFutsal/Rule Book/UpdateFieldRuleBookSpecificSlots.bru
| fieldId: 1bdfdb6b-41d8-4e39-a38c-398bdca0ba39 | ||
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIzMDIyMWFkOS0zZjgyLTQzZDgtYjQ1OS1kZGYyMGY2YzgzOTAiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODMwMDMyOSwiZXhwIjoxNzc4Mzg2NzI5fQ.GHeUq7DIOmimuj6eBVFtFq6HiFXf9oSQP8RS0pW1sCU | ||
| fieldId: 7e64459c-4a92-490c-bfdd-4ab660a626e9 | ||
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIzZjljZWYwZi0zZGQ0LTQwOGMtYTI2Ni0wMDJjN2E3MjlhODUiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODYzNzYxNywiZXhwIjoxNzc4NzI0MDE3fQ.lF51reUEi07ST1GxobnaRq_nQ9vHWKrmRwRx9yryfYU |
There was a problem hiding this comment.
Remove committed bearer token from tracked environment file.
A real JWT is committed in source control. Even if expired, this is a secrets-handling breach and encourages unsafe reuse.
Suggested fix
- token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...
+ token: YOUR_TOKEN_HERE📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIzZjljZWYwZi0zZGQ0LTQwOGMtYTI2Ni0wMDJjN2E3MjlhODUiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODYzNzYxNywiZXhwIjoxNzc4NzI0MDE3fQ.lF51reUEi07ST1GxobnaRq_nQ9vHWKrmRwRx9yryfYU | |
| token: YOUR_TOKEN_HERE |
🤖 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 `@ApiDocs/HelloFutsal/environments/Dev.bru` at line 4, The Dev.bru environment
file contains a committed bearer token under the key "token"; remove the token
value and replace it with a non-secret placeholder (e.g. TOKEN_PLACEHOLDER) in
the Dev.bru file, add Dev.bru (or the environment files pattern) to .gitignore,
rotate/revoke the exposed JWT immediately, and purge the secret from repository
history using a tool like git filter-repo or BFG to remove the leaked value from
past commits.
…andling and slot resynchronization
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/fields/fields.service.ts (2)
1195-1208: 💤 Low valueInconsistent slot sync error handling between
updateFieldRuleBookanddeleteRuleBook.
updateFieldRuleBookreturns{ slotsUpdated: false, message: "..." }when slot sync fails, allowing the caller to know the update succeeded but sync didn't. Here,deleteRuleBookthrows the error, leaving the rule book deactivated but the caller receiving an error response.Consider aligning the behavior: either both throw on sync failure, or both return a partial-success response.
Option: Return partial success like updateFieldRuleBook
try { await this.fieldSlotSyncService.syncFieldWindow( ruleBook.fieldId, 0, this.initialSlotWindowDays, ); } catch (error) { this.logger.error( `Failed to resync slots after deleting rule book id=${ruleBookId} for fieldId=${fieldId}`, error instanceof Error ? error.stack : String(error), ); - throw error; + return { + success: true, + ruleBookId, + message: "Rule book deleted successfully, but slot synchronization failed. Please retry slot sync.", + slotsUpdated: false, + }; } return { success: true, ruleBookId, message: "Rule book deleted successfully", + slotsUpdated: true, };🤖 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/fields/fields.service.ts` around lines 1195 - 1208, The deleteRuleBook flow currently catches sync errors from fieldSlotSyncService.syncFieldWindow and rethrows, causing callers to get an error even though the rule book was deactivated; align its behavior with updateFieldRuleBook by returning a partial-success response instead of throwing: inside deleteRuleBook replace the catch that rethrows with a return object like { slotsUpdated: false, message: "Failed to resync slots after deleting rule book", details: error instanceof Error ? error.stack : String(error) } (or follow your existing updateFieldRuleBook response shape), ensuring you reference the same symbols fieldSlotSyncService.syncFieldWindow and deleteRuleBook so callers can distinguish rule-book deactivation success from slot-sync failure.
1034-1036: 💤 Low valuePotential timezone mismatch when deriving
previousEffectiveDatefromcreatedAt.
toISOString()returns a UTC timestamp. If the database/application uses a different timezone, the extracted date could differ from the intended local date (e.g., 11 PM local on Jan 2nd becomes Jan 3rd UTC).Consider using a timezone-aware approach or ensuring
createdAtis stored/interpreted consistently witheffectiveDate.Example fix using local date extraction
const previousEffectiveDate = existingRuleBook.effectiveDate ?? - existingRuleBook.createdAt.toISOString().split("T")[0]; + FieldSlotGenerator.formatDateToString(existingRuleBook.createdAt);Where
formatDateToStringformats the Date using the same timezone convention asgetCurrentDateString().🤖 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/fields/fields.service.ts` around lines 1034 - 1036, previousEffectiveDate is derived using existingRuleBook.createdAt.toISOString() which forces UTC and can shift the local date; replace that extraction with a timezone-aware local-date formatter (or the project's existing helper like formatDateToString/getCurrentDateString) and use it when computing previousEffectiveDate instead of toISOString(). Update the assignment for previousEffectiveDate (the code referencing existingRuleBook.effectiveDate ?? existingRuleBook.createdAt...) to call the local-date formatter so createdAt is interpreted in the same timezone convention as effectiveDate.
🤖 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.
Nitpick comments:
In `@src/fields/fields.service.ts`:
- Around line 1195-1208: The deleteRuleBook flow currently catches sync errors
from fieldSlotSyncService.syncFieldWindow and rethrows, causing callers to get
an error even though the rule book was deactivated; align its behavior with
updateFieldRuleBook by returning a partial-success response instead of throwing:
inside deleteRuleBook replace the catch that rethrows with a return object like
{ slotsUpdated: false, message: "Failed to resync slots after deleting rule
book", details: error instanceof Error ? error.stack : String(error) } (or
follow your existing updateFieldRuleBook response shape), ensuring you reference
the same symbols fieldSlotSyncService.syncFieldWindow and deleteRuleBook so
callers can distinguish rule-book deactivation success from slot-sync failure.
- Around line 1034-1036: previousEffectiveDate is derived using
existingRuleBook.createdAt.toISOString() which forces UTC and can shift the
local date; replace that extraction with a timezone-aware local-date formatter
(or the project's existing helper like formatDateToString/getCurrentDateString)
and use it when computing previousEffectiveDate instead of toISOString(). Update
the assignment for previousEffectiveDate (the code referencing
existingRuleBook.effectiveDate ?? existingRuleBook.createdAt...) to call the
local-date formatter so createdAt is interpreted in the same timezone convention
as effectiveDate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b888c0cf-2b39-4785-b9ad-b897e774cdc4
📒 Files selected for processing (3)
ApiDocs/HelloFutsal/VerifyOTP/Admin OTP Verify.bruApiDocs/HelloFutsal/environments/Dev.brusrc/fields/fields.service.ts
✅ Files skipped from review due to trivial changes (1)
- ApiDocs/HelloFutsal/VerifyOTP/Admin OTP Verify.bru
🚧 Files skipped from review as they are similar to previous changes (1)
- ApiDocs/HelloFutsal/environments/Dev.bru
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.
Summary by CodeRabbit
New Features
Documentation