fix(loans): promote the waitlist head when reservations fully subscribe the copies (#157)#199
Open
fabiodalez-dev wants to merge 1 commit into
Open
fix(loans): promote the waitlist head when reservations fully subscribe the copies (#157)#199fabiodalez-dev wants to merge 1 commit into
fabiodalez-dev wants to merge 1 commit into
Conversation
…be the copies (#157) The reservation-maintenance cron failed to convert any reservation into a pending loan when a fully-subscribed waitlist queued for a title — e.g. 1 copy + 2 active reservations promoted 0 instead of 1. Root cause: the promotion gate (ReservationManager::processBookAvailability → isDateRangeAvailable → CapacityService::hasFreeCapacity) counted EVERY active reservation overlapping the period as occupying a capacity unit, excluding only the one being promoted. With 1 copy and 2 queued reservations, promoting the head (queue position 1) still counted the trailing entry (position 2) as occupying the only copy, so occ == total and the head was never promoted. Both runs converted 0; F.21b (#157) saw 0 pending instead of 1. Fix: the promotion path now passes the promoted reservation's queue_position so the capacity gate ignores active reservations with a known queue_position strictly greater than it — lower-priority waitlist entries promoted in later runs as copies free up. Reservations AHEAD in the queue (lower position) still count, and a legacy NULL queue_position still counts (conservative — never overbook). The new parameter defaults to null, so the admin-creation gate and overbooked auditor sharing CapacityService are unchanged. Verified: PHPStan level 5 clean; cron repro now promotes exactly one head (run 1 → 1 pending, run 2 → still 1); E2E loan-reservation-complete 26/26 (F.21b passes), loan-overlap 35/35 (overbook prevention intact), loan-state-model 4/4.
📝 WalkthroughWalkthroughLa promozione di una prenotazione ora calcola la posizione di coda della testa e la passa al controllo di capacità. Il calcolo della disponibilità esclude le prenotazioni con ChangesPromozione prenotazione e gate di capacità
Effort di revisione🎯 3 (Moderate) | ⏱️ ~20 minuti 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🤖 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 `@app/Controllers/ReservationManager.php`:
- Around line 196-199: In ReservationManager::promoteNextReservation, the queue
reordering path still uses nextReservation['queue_position'] directly, which
turns NULL into 0 and corrupts positions. Reuse the resolved queue position
already stored in $headQueuePos, and when it is null, treat the
promotion/reorder as starting from 1 instead of passing a zero value into
updateQueuePositions. Make the fix in the promotion flow around
isDateRangeAvailable and updateQueuePositions so the same validated queue
position is used consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb211fa2-61d6-42d6-8158-d11bc58d2920
📒 Files selected for processing (2)
app/Controllers/ReservationManager.phpapp/Services/CapacityService.php
Comment on lines
+196
to
+199
| $headQueuePos = isset($nextReservation['queue_position']) | ||
| ? (int) $nextReservation['queue_position'] | ||
| : null; | ||
| if ($this->isDateRangeAvailable($bookId, $startDate, $endDate, (int) $nextReservation['id'], $headQueuePos)) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Gestisci anche il riordino quando queue_position è NULL.
Qui $headQueuePos può diventare null e la promozione può proseguire; più avanti però updateQueuePositions($bookId, (int) $nextReservation['queue_position']) trasforma NULL in 0, decrementando tutte le posizioni > 0 e creando code con posizione 0. Usa lo stesso valore risolto e riordina da 1 quando manca una posizione valida.
🐛 Fix proposto
- $headQueuePos = isset($nextReservation['queue_position'])
+ $headQueuePos = isset($nextReservation['queue_position'])
? (int) $nextReservation['queue_position']
: null;
if ($this->isDateRangeAvailable($bookId, $startDate, $endDate, (int) $nextReservation['id'], $headQueuePos)) {
@@
- $this->updateQueuePositions($bookId, (int) $nextReservation['queue_position']);
+ if ($headQueuePos !== null) {
+ $this->updateQueuePositions($bookId, $headQueuePos);
+ } else {
+ $this->reorderQueuePositions($bookId);
+ }🤖 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 `@app/Controllers/ReservationManager.php` around lines 196 - 199, In
ReservationManager::promoteNextReservation, the queue reordering path still uses
nextReservation['queue_position'] directly, which turns NULL into 0 and corrupts
positions. Reuse the resolved queue position already stored in $headQueuePos,
and when it is null, treat the promotion/reorder as starting from 1 instead of
passing a zero value into updateQueuePositions. Make the fix in the promotion
flow around isDateRangeAvailable and updateQueuePositions so the same validated
queue position is used consistently.
fabiodalez-dev
changed the title
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.
Problem
Reservation maintenance (
cron/full-maintenance.php→MaintenanceService::processScheduledReservations) promoted no reservation to a pending loan when a waitlist fully subscribed a title: 1 copy + 2 active reservations → 0 promoted instead of 1.Cause
The promotion gate (
ReservationManager::processBookAvailability→isDateRangeAvailable→CapacityService::hasFreeCapacity) counted every overlapping active reservation as occupying a capacity unit, excluding only the one being promoted. With 1 copy and 2 reservations, promoting the head (queue_position=1) still counted the second (queue_position=2) as occupying the only copy →occ == total→ head never promoted. Both runs converted 0; test F.21b (#157) saw 0 pending instead of 1.Fix
The promotion path now passes the promoted reservation's
queue_position, so the capacity gate ignores active reservations with a strictly greaterqueue_position(lower-priority waitlist entries, promoted in later runs as copies free up).queue_positionstill counts (conservative — never overbook).default null): the admin-creation gate and overbooked-auditor, which shareCapacityService, are unchanged.Verification
Surfaced while running the full E2E suite; pre-existing on
main, independent of the Swagger PR #197.