Bulk booking#50
Merged
Merged
Conversation
… checks Co-authored-by: Copilot <copilot@github.com>
…ith pricing history tracking
…ncellations, price upgrades, and slot modifications
…ew prices; optimize query for membership bookings
… validation and price upgrade improvements
|
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 (9)
📝 WalkthroughWalkthroughThis PR introduces membership plan lifecycle management with pricing history tracking, and bulk booking operations. New ChangesMembership Pricing History & Bulk Booking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 20
🧹 Nitpick comments (8)
src/booking/booking.service.ts (3)
324-326: ⚡ Quick winDrop
anyfor the DTOs.Both
bulkConfirmDto: anyandbulkBookDto: anydiscard the type information from the validated DTOs. NestJS still validates the runtime payload via the controller's parameter type, but the service body can no longer rely on the compiler to catch field renames or shape drift. ImportBulkConfirmBookingsDto/BulkBookSlotsDtoand type the parameters explicitly.Also applies to: 667-669
🤖 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 324 - 326, Replace the loose any types on the service methods with the proper DTO types: import BulkConfirmBookingsDto and BulkBookSlotsDto and change the method signatures so bulkConfirmBookings(account: AuthenticatedAccount, bulkConfirmDto: BulkConfirmBookingsDto) and the corresponding bulkBook* method (the one using bulkBookDto) accept BulkBookSlotsDto instead of any; update any internal references if needed to satisfy the new types so the compiler catches shape drift.
686-686: 💤 Low valueUseless
try/catchwrapper.The outer block only does
} catch (error) { throw error; }, which is a no-op and just adds noise. Drop the wrapper.Diff
- try { - return await this.fieldSlotsRepository.manager.transaction( + return await this.fieldSlotsRepository.manager.transaction( async (manager) => { ... }, - ); - } catch (error) { - throw error; - } + );Also applies to: 859-862
🤖 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` at line 686, Remove the redundant outer try/catch blocks that simply `catch (error) { throw error; }` (they are no-ops); delete those try and catch clauses and leave the inner logic intact so exceptions propagate naturally. Apply this change to both occurrences of the useless wrapper in the BookingService implementation (the try/catch starting at the shown `try {` and the second redundant block noted around lines 859-862).
401-440: ⚡ Quick winDiscount validation logic duplicates
confirmBooking.The discount/extra computation here is essentially copy-pasted from
confirmBooking(lines 248-285). Extracting a private helper (e.g.,applyAmountDecisions(booking, baseAmount, totalAmount, discountFlag?)) would remove the duplication and ensure the two flows stay in sync if rules change.🤖 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 401 - 440, The discount/extra amount logic duplicated between this block and confirmBooking should be extracted into a single private helper to avoid drift; create a private method (e.g., applyAmountDecisions(booking: BookingDto, baseAmount: number, totalAmount: number, discountFlag?: boolean)) that encapsulates the existing decision tree (handling undefined discountFlag, validation throws when discount flag contradicts amounts, and setting booking.discount, booking.discountAmount, booking.extraAmount via formatAmount), then replace the duplicated code in both confirmBooking and the current location with calls to this new helper, passing it the booking object and the optional it.discount flag.src/booking/membership-plan.controller.ts (3)
123-128: ⚡ Quick winType
managerasEntityManager.Using
manager: anythrows away typing for every TypeORM call inside these helpers (and the call sites already pass anEntityManagerfrommanager.transaction). ImportingEntityManagerfromtypeormand replacinganykeeps type safety acrossgetRepository,save, etc., and matches the rest of the codebase.Also applies to: 188-196
🤖 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/membership-plan.controller.ts` around lines 123 - 128, Change the untyped manager parameters to a typed TypeORM EntityManager: import EntityManager from "typeorm" and replace manager: any with manager: EntityManager in the ensureNoMembershipScheduleConflicts method and the other helper methods flagged (the ones around lines 188-196) so all internal calls like getRepository, save, and transaction-aware queries have proper typing and IDE/type-checker support; update signatures of ensureNoMembershipScheduleConflicts and the other referenced functions accordingly.
568-568: 💤 Low value
appliesImmediatelyis effectivelyeffectiveDate === today.Given the guard at lines 562-566 throws when
effectiveDate < today, the only wayeffectiveDate <= todayis satisfied is equality. The expression is fine, but the variable name suggests behavior the guard precludes — considerconst isEffectiveToday = effectiveDate.getTime() === today.getTime()for clarity.🤖 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/membership-plan.controller.ts` at line 568, The variable appliesImmediately in membership-plan.controller.ts is misleading because earlier guard prevents effectiveDate < today, so only equality remains; rename and change its computation to be explicit equality by using getTime comparison: replace const appliesImmediately = effectiveDate <= today; with a clear isEffectiveToday boolean computed as effectiveDate.getTime() === today.getTime() (and update any downstream uses to the new name) so the intent matches the guard in the surrounding method (look for appliesImmediately usage in the same function/controller).
405-439: 💤 Low valueDispatcher exclusivity rules are partially relaxed but the error message says otherwise.
hasTimeRangeUpdate && hasBasicPlanUpdateis intentionally not in the rejection list (basic + slot updates are combined insideperformSlotTimeUpdate), and the error message at line 437-438 acknowledges that. However, the actual ordering at lines 442-467 hashasBasicPlanUpdateonly run when none of the other branches match, so a request with{userName, timeRange}correctly goes throughperformSlotTimeUpdate, while{userName, endDate}goes through cancellation and silently drops theuserNamechange. Worth either documenting thatendDateandeffectiveFromDate+newPricerequests ignore basic fields, or enforcing it explicitly.🤖 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/membership-plan.controller.ts` around lines 405 - 439, The current dispatcher silently ignores basic plan fields when paired with cancellation (hasEndDate) or price-upgrade (hasPriceUpgrade); update the exclusivity logic in membership-plan.controller.ts so requests that include basic plan updates together with endDate or effectiveFromDate+newPrice are rejected explicitly: add checks that (hasBasicPlanUpdate && hasEndDate) and (hasBasicPlanUpdate && hasPriceUpgrade) cause a BadRequestException (and update the error message to reflect this), or alternatively merge basic field handling into the cancellation/price-upgrade handlers if you intend to allow them; refer to the hasBasicPlanUpdate, hasEndDate, hasPriceUpgrade flags and performSlotTimeUpdate decision path to locate where to enforce the change.src/fields/cron/field-slot-cron.service.ts (2)
150-162: 💤 Low valuePush
endDatefilter into the DB query.
active: trueis already applied at the DB, butendDate >= todayInNepalDateis filtered in JS afterwards. Plans with staleendDateare still pulled into memory along with theirfield/userrelations. Combining both filters in one query keeps the result set tight and the log line still computesplans.lengthvsactivePlans.lengthfrom a single tighter result if you keep two queries, or just collapse to one.Proposed query
- const plans = await this.membershipPlanRepo.find({ - where: { active: true }, - relations: ["field", "user"], - }); - - // Filter plans that are still within their active period - const activePlans = plans.filter( - (plan) => !plan.endDate || plan.endDate >= todayInNepalDate, - ); + const activePlans = await this.membershipPlanRepo + .createQueryBuilder("plan") + .leftJoinAndSelect("plan.field", "field") + .leftJoinAndSelect("plan.user", "user") + .where("plan.active = :active", { active: true }) + .andWhere( + "(plan.end_date IS NULL OR plan.end_date >= :today)", + { today: todayInNepalDate }, + ) + .getMany();🤖 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/cron/field-slot-cron.service.ts` around lines 150 - 162, The code currently fetches all plans via membershipPlanRepo.find and then filters activePlans in memory using todayInNepalDate; instead push the endDate predicate into the DB query so stale rows and their relations (field/user) are not loaded. Update the membershipPlanRepo.find call (the one that currently sets where: { active: true } and relations: ["field","user"]) to include an endDate >= todayInNepalDate condition (or endDate IS NULL OR endDate >= todayInNepalDate depending on desired semantics) so the DB returns only truly active plans; keep the existing logging but compute counts from the returned result set(s) associated with the updated query and remove the in-memory filter (activePlans) or, if you need both total vs in-period counts, run a second lightweight count query instead of loading all stale relations.
274-289: The slot↔booking unique relationship is already enforced;getOne()is safe.The database enforces a UNIQUE constraint on
slot_id(both at migration and ORM level), guaranteeing at most one booking per slot. While this prevents race conditions andgetOne()errors, the global constraint is stricter than necessary given the code filters forstatus != 'cancelled'. For semantic clarity, consider a partial unique index on(slot_id) WHERE status != 'cancelled'if you intend to allow multiple bookings per slot when one is cancelled.🤖 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/cron/field-slot-cron.service.ts` around lines 274 - 289, The database currently enforces a global UNIQUE on slot_id, making the Booking lookup using Booking repository and createQueryBuilder(...).getOne() safe; to match the code's intent (only excluding cancelled bookings) change the DB constraint to a partial unique index on slot_id WHERE status != 'cancelled' so cancelled bookings can coexist, and update any migration/ORM schema accordingly (referencing the Booking entity and the query filtering on "booking.status != :cancelled" / "booking.slot_id"). Ensure migrations add the partial index and remove the old global unique constraint so the semantics align with the existingSlot check that returns false when an active (non-cancelled) existingBooking is found.
🤖 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/Booking/BulkBookSlots.bru`:
- Around line 26-27: The example payload uses non-zero-padded times; update the
example values for the startTime and endTime fields in the BulkBookSlots example
so they use zero-padded "HH:mm" format (e.g., change "6:00" to "06:00" and
"11:00" to "11:00" if not already zero-padded) to match validators that expect
two-digit hours; locate and edit the startTime and endTime entries in the
BulkBookSlots example JSON to enforce the "HH:mm" format.
In `@ApiDocs/HelloFutsal/environments/Dev.bru`:
- Line 4: The committed JWT value under the token key in Dev.bru is a live
credential; remove the literal token and replace it with a placeholder (e.g.,
token: <YOUR_JWT_HERE>) and wire the code that reads Dev.bru to load the real
secret from an environment variable or secret manager instead; after removing
the token, rotate/invalidate the exposed JWT immediately and update any docs or
CI that referenced the file to use the new secret-loading mechanism (reference
the token entry in Dev.bru to locate the change).
In `@ApiDocs/HelloFutsal/MembershipSlots/UpdateMembership.bru`:
- Around line 10-15: Update the auth declaration and headers to match other
booking docs: replace "auth: none" with "auth: bearer", remove the inline
"Authorization: Bearer {{token}}" header, and add an "auth:bearer" block (with
the bearer token variable) consistent with how other endpoints (e.g.,
UpdateMembership) declare authentication so the file uses explicit bearer auth
instead of sending Authorization in headers while stating none.
In `@ApiDocs/HelloFutsal/VerifyOTP/Admin` OTP Verify.bru:
- Around line 23-24: Replace the hardcoded "requestId" and "otp" JSON values in
the Admin OTP Verify.bru (and similarly in User OTP Verify.bru) with environment
variable placeholders (e.g., use Bruno variable syntax for requestId and otp)
and remove literal credentials from the request body; instead add those
variables to the Bruno environment configuration so requests reference env vars
(look for the "requestId" and "otp" fields in the .bru files and update them to
use the environment placeholders rather than fixed strings).
In `@src/booking/booking.service.ts`:
- Around line 339-347: The current split assigns the same rounded perSlotAmount
(computed from rootTotalRaw) to every booking which loses cents; replace this
with integer-cent arithmetic: convert bulkConfirmDto.totalAmount (rootTotalRaw)
to cents (Math.round), compute base = Math.floor(cents / count) and remainder =
cents - base * count, then compute per-slot cents per index (add remainder to
the last or distribute across slots) and convert back to dollars for each
booking instead of using the single perSlotAmount value; apply the same fix to
the other split site (the similar logic around the occurrence at the other block
referenced, e.g., where perSlotAmount is used at the later code around lines
442-443) and update functions/variables rootTotalRaw, perSlotAmount, and the
assignment code that builds each slot’s amount accordingly.
- Around line 738-755: The current code applies setLock("pessimistic_write") to
getMany() on slotRepository (slotQuery) over an unbounded date/time window
(startDate/endDate and optional startTime/endTime), which can hold many row
locks and cause contention; change approach by (1) enforcing a maximum range
size in the incoming DTO validation for startDate/endDate (and
startTime/endTime) to limit how many slots can be selected, (2) remove the broad
setLock("pessimistic_write") on slotQuery.getMany() and instead iterate
candidate slot ids and for each slot perform a targeted SELECT ... FOR UPDATE
(or queryBuilder.setLock("pessimistic_write") on a single-slot query) inside its
own savepoint/transactional unit to lock only one row at a time, and (3)
eliminate the per-slot N+1 membership-plan lookups by prefetching relevant
membership plans once (e.g., load plans for the field or matching criteria
before the slot loop) and match them in memory when processing each slot
(referencing slotRepository, slotQuery, setLock("pessimistic_write"), getMany,
and the membership-plan lookup loop).
- Around line 475-550: cancelBooking currently only resets slot.status but
leaves membership metadata (slot.slotType and slot.membershipPlanId) intact;
update cancelBooking to clear membership fields the same way
performMembershipCancellation does by setting slot.slotType and
slot.membershipPlanId back to their non-membership values (e.g., null or the
default open type) before saving with slotRepository.save so the slot won't be
treated as a membership slot by bulkBookSlots/cron; reference the cancelBooking
function and the performMembershipCancellation logic as the guide for which
fields to reset and where to persist the change.
- Around line 760-806: bulkBookSlots and createBooking currently set slot.price
from matchingPlan.perSlotPrice and ignore MembershipPricingHistory; change both
to look up the applicable MembershipPricingHistory for the slot date (similar to
field-slot-cron.service logic): query MembershipPricingHistory (use
effective_from_date <= :slotDate and order by effective_from_date DESC, limit 1)
for the matching plan and if a history record exists use its perSlotPrice for
slot.price (fallback to matchingPlan.perSlotPrice if no history); update the
code paths in bulkBookSlots (where matchingPlan is applied) and in createBooking
(the single-booking flow) to use this effective price and preserve
slot.slotType/membershipPlanId/bookingType behavior.
In `@src/booking/dto/bulk-book-slots.dto.ts`:
- Around line 14-32: Add cross-field validation to ensure the date/time range is
not reversed: implement a class-level validator (e.g., create a
ValidatorConstraint like BulkRangeValidator and apply it with `@Validate` on the
DTO class that contains startDate, endDate, startTime, endTime) and in it parse
startDate/endDate as ISO dates and startTime/endTime as HH:mm; enforce endDate
>= startDate, and if dates are equal and both times provided enforce endTime >
startTime (also handle when times are optional by only checking times if both
present). Attach this validator to the BulkBookSlotsDto so bulkBookSlots
receives a rejected request instead of silently matching zero slots.
- Around line 14-18: Replace the loose `@IsDateString`() validators on startDate
and endDate with the project's strict DateYYYYMMDDConstraint: import
DateYYYYMMDDConstraint and apply it via class-validator's
`@Validate`(DateYYYYMMDDConstraint) (or the project's established wrapper) to both
startDate and endDate properties in the bulk booking DTO so they only accept
YYYY-MM-DD strings; keep the property names startDate and endDate unchanged.
In `@src/booking/dto/bulk-confirm-bookings.dto.ts`:
- Around line 10-11: Add numeric and range validation to the totalAmount
property by applying class-validator and class-transformer decorators: keep
`@IsOptional`(), add `@Type`(() => Number) to ensure numeric transformation, add
`@IsNumber`({ maxDecimalPlaces: 2 }, { message: 'totalAmount must be a number with
at most 2 decimal places' }) and `@IsPositive`({ message: 'totalAmount must be a
positive number' }) to enforce positivity and two-decimal precision; update the
property declaration totalAmount?: number; in the BulkConfirmBookingsDto class
accordingly.
- Around line 13-16: The bookings property currently allows an empty array and
bypasses DTO validation; add the `@ArrayNotEmpty`() decorator to the bookings
property (the array of BulkConfirmItem) in bulk-confirm-bookings.dto.ts to
enforce non-empty arrays at the DTO boundary, and import ArrayNotEmpty from
class-validator if not already imported; update the property declaration for
bookings to include `@ArrayNotEmpty`() alongside `@IsArray`(), `@ValidateNested`({
each: true }) and `@Type`(() => BulkConfirmItem).
In `@src/booking/dto/cancel-membership-plan.dto.ts`:
- Around line 4-5: Replace the loose `@IsDateString`() on the endDate field with
the stricter custom validator by importing and using DateYYYYMMDDConstraint (the
same one used in create-membership-plan.dto.ts); update the decorator on endDate
from `@IsDateString`() to `@Validate`(DateYYYYMMDDConstraint) and add the
corresponding import for DateYYYYMMDDConstraint so endDate only accepts
YYYY-MM-DD date-only strings.
In `@src/booking/dto/update-membership-plan.dto.ts`:
- Around line 29-32: Update the DTO so currency fields match the persistence
layer: change perSlotPrice and newPrice in UpdateMembershipPlanDto from number
to string (to align with the entity's numeric(12,2) columns) and replace any
usage of `@IsNumber`({ maxDecimalPlaces: 2 }) with a string-safe validator (either
class-validator's `@IsDecimal`({ decimal_digits: '2' }) or a small custom
validator that enforces two decimal places), and remove any controller-side
.toFixed(2) conversions that were added to coerce numbers—this ensures type
consistency (perSlotPrice, newPrice) and avoids the known maxDecimalPlaces bug.
In `@src/booking/dto/upgrade-membership-price.dto.ts`:
- Around line 4-5: Replace the loose `@IsDateString`() validator on the
effectiveFromDate property with the stricter custom DateYYYYMMDDConstraint
decorator to enforce exact YYYY-MM-DD format; import DateYYYYMMDDConstraint from
its module and apply it to the effectiveFromDate field in the
UpgradeMembershipPrice DTO (effectiveFromDate) so payloads with full ISO
datetimes are rejected.
- Around line 7-9: The newPrice property in UpgradeMembershipPriceDto currently
has `@IsNumber`() and `@IsPositive`() but lacks decimal-place validation; update the
decorator on the newPrice field (property newPrice in the DTO class) to use
`@IsNumber`({ maxDecimalPlaces: 2 }) alongside `@IsPositive`() so incoming values
are validated to at most two decimal places before persistence.
In `@src/booking/membership-plan.controller.ts`:
- Around line 882-895: performMembershipDetailUpdate currently mutates
membership.perSlotPrice directly (and has an unreachable timeRange branch) which
bypasses pricing history; instead, when dto.perSlotPrice is provided call the
existing performPriceUpgrade flow (or create a MembershipPricingHistory row via
the same logic used by performPriceUpgrade) so the date-effective price is
recorded (do not directly assign membership.perSlotPrice); also remove the
dto.timeRange branch in performMembershipDetailUpdate (timeRange updates are
routed to performSlotTimeUpdate) to avoid skipping conflict validation and slot
sync—use validateMembershipScheduleShape/transformTimeRangeToStorageFormat only
inside performSlotTimeUpdate if needed.
- Around line 766-784: The date string used for SQL comparisons is computed in
UTC; replace the current new Date()/toISOString() logic inside the
removedTimeWindows loop with a Kathmandu-localized date using Luxon (e.g.,
DateTime.now().setZone("Asia/Kathmandu").startOf('day').toISODate()) so the
value passed to the bookingRepo query reflects Asia/Kathmandu midnight. Make the
same replacement in performMembershipCancellation and performPriceUpgrade where
similar today/todayString is computed, ensuring all three locations (the
removedTimeWindows loop, performMembershipCancellation, performPriceUpgrade) use
DateTime.now().setZone("Asia/Kathmandu").startOf('day').toISODate() for
consistent date comparisons with getPlayedSlots.
In `@src/migrations/1780000000003-CreateMembershipPricingHistory.ts`:
- Around line 45-50: The index "idx_membership_pricing_plan_date" on columns
["membership_plan_id","effective_from_date"] must be unique to prevent multiple
rows for the same plan/date; modify the migration's indices entry (in
CreateMembershipPricingHistory migration) to create a UNIQUE index/constraint
instead of a non-unique one so the DB enforces one effective price per
membership_plan_id+effective_from_date combination.
- Line 19: The migration uses gen_random_uuid() (see the default:
"gen_random_uuid()" in CreateMembershipPricingHistory) but the project only
installs uuid-ossp elsewhere; either add CREATE EXTENSION IF NOT EXISTS
"pgcrypto" in an earlier migration (so gen_random_uuid() is available) or change
this migration to use uuid_generate_v4() to match the existing pattern; update
the CreateMembershipPricingHistory migration accordingly so the UUID function
and extension usage are consistent with other migrations.
---
Nitpick comments:
In `@src/booking/booking.service.ts`:
- Around line 324-326: Replace the loose any types on the service methods with
the proper DTO types: import BulkConfirmBookingsDto and BulkBookSlotsDto and
change the method signatures so bulkConfirmBookings(account:
AuthenticatedAccount, bulkConfirmDto: BulkConfirmBookingsDto) and the
corresponding bulkBook* method (the one using bulkBookDto) accept
BulkBookSlotsDto instead of any; update any internal references if needed to
satisfy the new types so the compiler catches shape drift.
- Line 686: Remove the redundant outer try/catch blocks that simply `catch
(error) { throw error; }` (they are no-ops); delete those try and catch clauses
and leave the inner logic intact so exceptions propagate naturally. Apply this
change to both occurrences of the useless wrapper in the BookingService
implementation (the try/catch starting at the shown `try {` and the second
redundant block noted around lines 859-862).
- Around line 401-440: The discount/extra amount logic duplicated between this
block and confirmBooking should be extracted into a single private helper to
avoid drift; create a private method (e.g., applyAmountDecisions(booking:
BookingDto, baseAmount: number, totalAmount: number, discountFlag?: boolean))
that encapsulates the existing decision tree (handling undefined discountFlag,
validation throws when discount flag contradicts amounts, and setting
booking.discount, booking.discountAmount, booking.extraAmount via formatAmount),
then replace the duplicated code in both confirmBooking and the current location
with calls to this new helper, passing it the booking object and the optional
it.discount flag.
In `@src/booking/membership-plan.controller.ts`:
- Around line 123-128: Change the untyped manager parameters to a typed TypeORM
EntityManager: import EntityManager from "typeorm" and replace manager: any with
manager: EntityManager in the ensureNoMembershipScheduleConflicts method and the
other helper methods flagged (the ones around lines 188-196) so all internal
calls like getRepository, save, and transaction-aware queries have proper typing
and IDE/type-checker support; update signatures of
ensureNoMembershipScheduleConflicts and the other referenced functions
accordingly.
- Line 568: The variable appliesImmediately in membership-plan.controller.ts is
misleading because earlier guard prevents effectiveDate < today, so only
equality remains; rename and change its computation to be explicit equality by
using getTime comparison: replace const appliesImmediately = effectiveDate <=
today; with a clear isEffectiveToday boolean computed as effectiveDate.getTime()
=== today.getTime() (and update any downstream uses to the new name) so the
intent matches the guard in the surrounding method (look for appliesImmediately
usage in the same function/controller).
- Around line 405-439: The current dispatcher silently ignores basic plan fields
when paired with cancellation (hasEndDate) or price-upgrade (hasPriceUpgrade);
update the exclusivity logic in membership-plan.controller.ts so requests that
include basic plan updates together with endDate or effectiveFromDate+newPrice
are rejected explicitly: add checks that (hasBasicPlanUpdate && hasEndDate) and
(hasBasicPlanUpdate && hasPriceUpgrade) cause a BadRequestException (and update
the error message to reflect this), or alternatively merge basic field handling
into the cancellation/price-upgrade handlers if you intend to allow them; refer
to the hasBasicPlanUpdate, hasEndDate, hasPriceUpgrade flags and
performSlotTimeUpdate decision path to locate where to enforce the change.
In `@src/fields/cron/field-slot-cron.service.ts`:
- Around line 150-162: The code currently fetches all plans via
membershipPlanRepo.find and then filters activePlans in memory using
todayInNepalDate; instead push the endDate predicate into the DB query so stale
rows and their relations (field/user) are not loaded. Update the
membershipPlanRepo.find call (the one that currently sets where: { active: true
} and relations: ["field","user"]) to include an endDate >= todayInNepalDate
condition (or endDate IS NULL OR endDate >= todayInNepalDate depending on
desired semantics) so the DB returns only truly active plans; keep the existing
logging but compute counts from the returned result set(s) associated with the
updated query and remove the in-memory filter (activePlans) or, if you need both
total vs in-period counts, run a second lightweight count query instead of
loading all stale relations.
- Around line 274-289: The database currently enforces a global UNIQUE on
slot_id, making the Booking lookup using Booking repository and
createQueryBuilder(...).getOne() safe; to match the code's intent (only
excluding cancelled bookings) change the DB constraint to a partial unique index
on slot_id WHERE status != 'cancelled' so cancelled bookings can coexist, and
update any migration/ORM schema accordingly (referencing the Booking entity and
the query filtering on "booking.status != :cancelled" / "booking.slot_id").
Ensure migrations add the partial index and remove the old global unique
constraint so the semantics align with the existingSlot check that returns false
when an active (non-cancelled) existingBooking is found.
🪄 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: fbf92677-b7c7-4bbb-810b-50b986169c08
📒 Files selected for processing (23)
ApiDocs/HelloFutsal/Booking/BulkBookSlots.bruApiDocs/HelloFutsal/Booking/BulkConfirmBookings.bruApiDocs/HelloFutsal/Booking/CancelBooking.bruApiDocs/HelloFutsal/Booking/CancelMembership.bruApiDocs/HelloFutsal/MembershipSlots/UpdateMembership.bruApiDocs/HelloFutsal/VerifyOTP/Admin OTP Verify.bruApiDocs/HelloFutsal/environments/Dev.brusrc/booking/booking.controller.tssrc/booking/booking.module.tssrc/booking/booking.service.tssrc/booking/dto/bulk-book-slots.dto.tssrc/booking/dto/bulk-confirm-bookings.dto.tssrc/booking/dto/cancel-membership-plan.dto.tssrc/booking/dto/date-yyyymmdd.constraint.tssrc/booking/dto/update-membership-plan.dto.tssrc/booking/dto/upgrade-membership-price.dto.tssrc/booking/entities/membership-plan.entity.tssrc/booking/entities/membership-pricing-history.entity.tssrc/booking/membership-plan.controller.tssrc/fields/cron/field-slot-cron.service.tssrc/fields/fields.module.tssrc/migrations/1780000000002-AddEndDateToMembershipPlans.tssrc/migrations/1780000000003-CreateMembershipPricingHistory.ts
Comment on lines
+26
to
+27
| "startTime": "6:00", | ||
| "endTime": "11:00", |
There was a problem hiding this comment.
Use zero-padded HH:mm in example payloads.
On Line 26, startTime is "6:00" while most validators/examples expect "06:00" format. Keep both times zero-padded to avoid validation failures when running this request directly.
🤖 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/Booking/BulkBookSlots.bru` around lines 26 - 27, The
example payload uses non-zero-padded times; update the example values for the
startTime and endTime fields in the BulkBookSlots example so they use
zero-padded "HH:mm" format (e.g., change "6:00" to "06:00" and "11:00" to
"11:00" if not already zero-padded) to match validators that expect two-digit
hours; locate and edit the startTime and endTime entries in the BulkBookSlots
example JSON to enforce the "HH:mm" format.
| fieldId: 3066550b-b4b3-4098-ad5c-8dd25a703fe0 | ||
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI4MTNlNzNhYi1iYzE1LTQ0ZWUtODgxZC1kMTZiNzQzOWQ5NDQiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3Nzg4MDc1NCwiZXhwIjoxNzc3OTY3MTU0fQ.r-zjgdL-KOFvqFl7_bM3jH7qMOSxJU-cGzqYl2VHDB4 | ||
| fieldId: 1bdfdb6b-41d8-4e39-a38c-398bdca0ba39 | ||
| token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIzMDIyMWFkOS0zZjgyLTQzZDgtYjQ1OS1kZGYyMGY2YzgzOTAiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODMwMDMyOSwiZXhwIjoxNzc4Mzg2NzI5fQ.GHeUq7DIOmimuj6eBVFtFq6HiFXf9oSQP8RS0pW1sCU |
There was a problem hiding this comment.
Remove committed JWT and rotate it immediately
A live token is checked into source. This is a credential exposure and should be replaced with a local placeholder plus secret rotation.
Suggested fix
- token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...
+ token: <SET_LOCALLY_DO_NOT_COMMIT>📝 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.eyJzdWIiOiIzMDIyMWFkOS0zZjgyLTQzZDgtYjQ1OS1kZGYyMGY2YzgzOTAiLCJlbWFpbCI6bnVsbCwibW9iaWxlTnVtYmVyIjoiOTg2Nzc1NDczOCIsInJvbGUiOiJhZG1pbiIsImlhdCI6MTc3ODMwMDMyOSwiZXhwIjoxNzc4Mzg2NzI5fQ.GHeUq7DIOmimuj6eBVFtFq6HiFXf9oSQP8RS0pW1sCU | |
| token: <SET_LOCALLY_DO_NOT_COMMIT> |
🤖 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 committed JWT value
under the token key in Dev.bru is a live credential; remove the literal token
and replace it with a placeholder (e.g., token: <YOUR_JWT_HERE>) and wire the
code that reads Dev.bru to load the real secret from an environment variable or
secret manager instead; after removing the token, rotate/invalidate the exposed
JWT immediately and update any docs or CI that referenced the file to use the
new secret-loading mechanism (reference the token entry in Dev.bru to locate the
change).
Comment on lines
+10
to
+15
| auth: none | ||
| } | ||
|
|
||
| headers { | ||
| Authorization: Bearer {{token}} | ||
| Content-Type: application/json |
There was a problem hiding this comment.
Auth mode is inconsistent with the actual request headers.
Line 10 sets auth: none, while Line 14 still sends a bearer token. Prefer auth: bearer + auth:bearer block for consistency with other booking docs and to avoid accidental unauthenticated copies.
🤖 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/MembershipSlots/UpdateMembership.bru` around lines 10 -
15, Update the auth declaration and headers to match other booking docs: replace
"auth: none" with "auth: bearer", remove the inline "Authorization: Bearer
{{token}}" header, and add an "auth:bearer" block (with the bearer token
variable) consistent with how other endpoints (e.g., UpdateMembership) declare
authentication so the file uses explicit bearer auth instead of sending
Authorization in headers while stating none.
Comment on lines
+23
to
+24
| "requestId": "dcec5e4b-303c-4fd2-ac10-204f6e872c04", | ||
| "otp": "160433" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for potential hardcoded credentials in .bru files
# Search for OTP patterns and UUIDs in body sections
rg -n -C2 '"otp":\s*"[0-9]+"' --glob '*.bru'
rg -n -C2 '"requestId":\s*"[a-f0-9-]+"' --glob '*.bru'Repository: hellofutsal/HelloFutsal
Length of output: 1511
Replace hardcoded OTP and requestId values with environment variable placeholders across all API documentation.
Hardcoded credentials appear in at least two .bru files (Admin OTP Verify.bru and User OTP Verify.bru). While these may be test values, committing credential patterns to version control is a security risk. Use environment variables instead:
body:json {
{
- "requestId": "dcec5e4b-303c-4fd2-ac10-204f6e872c04",
- "otp": "160433"
+ "requestId": "{{requestId}}",
+ "otp": "{{otp}}"
}
}Define these variables in Bruno environment configuration files rather than in API request definitions.
📝 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
| "requestId": "dcec5e4b-303c-4fd2-ac10-204f6e872c04", | |
| "otp": "160433" | |
| "requestId": "{{requestId}}", | |
| "otp": "{{otp}}" |
🤖 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/VerifyOTP/Admin` OTP Verify.bru around lines 23 - 24,
Replace the hardcoded "requestId" and "otp" JSON values in the Admin OTP
Verify.bru (and similarly in User OTP Verify.bru) with environment variable
placeholders (e.g., use Bruno variable syntax for requestId and otp) and remove
literal credentials from the request body; instead add those variables to the
Bruno environment configuration so requests reference env vars (look for the
"requestId" and "otp" fields in the .bru files and update them to use the
environment placeholders rather than fixed strings).
…ation and pricing logic
This was referenced May 13, 2026
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
Release Notes