Feature zmskvr 75 optimize calculate slot #891
Conversation
WalkthroughThis pull request introduces two sets of changes. In the query class, a new constant is added to provide an SQL query that retrieves the most recent "free" slot date for a given availability. In the core Slot class, a new constant defines the maximum days for slot calculations, a helper method retrieves the last generated slot date (defaulting to a baseline if none exists), and the slot generation method is updated with refined logic to determine when new slots should be generated and when the processing loop should terminate. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Slot as Slot Class
participant DB as Database
Caller->>Slot: writeByAvailability(availability)
Note over Slot: Retrieve last generated slot date
Slot->>Slot: getLastGeneratedSlotDate(availability)
Slot-->>Slot: lastGeneratedSlotDate returned
Slot->>Slot: Check if availability > lastGeneratedSlotDate
alt New slots needed
loop For each valid slot date
Slot->>DB: Execute QUERY_LAST_IN_AVAILABILITY query
DB-->>Slot: Return latest free slot date
Slot->>Slot: Process slot generation
end
end
Slot-->>Caller: Update availability with new slots
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (1)
zmsdb/src/Zmsdb/Slot.php (1)
468-483: Add PHPDoc block and return type hint.The method would benefit from better documentation.
Apply this diff to improve documentation:
+ /** + * Get the date of the last generated slot for a given availability + * + * @param AvailabilityEntity $availability The availability to check + * @return \DateTimeImmutable The date of the last generated slot or 1970-01-01 12:00 if none exists + */ - private function getLastGeneratedSlotDate(AvailabilityEntity $availability) + private function getLastGeneratedSlotDate(AvailabilityEntity $availability): \DateTimeImmutable
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsdb/src/Zmsdb/Query/Slot.php(1 hunks)zmsdb/src/Zmsdb/Slot.php(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
zmsdb/src/Zmsdb/Query/Slot.php (1)
23-28: LGTM!The SQL query is well-structured, using appropriate date formatting and ordering to retrieve the most recent slot date.
zmsdb/src/Zmsdb/Slot.php (2)
23-23: LGTM!The constant
MAX_DAYS_OF_SLOT_CALCULATIONprovides a clear upper limit for slot calculations.
180-180: Optimization looks good!The changes effectively limit slot calculation to 180 days and avoid regenerating existing slots by tracking the last generated slot date.
Also applies to: 184-184, 224-224
|
|
||
| return $status || ($cancelledSlots > 0); | ||
| return $status || (isset($cancelledSlots) && $cancelledSlots > 0); |
There was a problem hiding this comment.
Fix potential undefined variable access.
The return statement references $cancelledSlots which might be undefined if $generateNew is false.
Apply this diff to fix the issue:
- return $status || (isset($cancelledSlots) && $cancelledSlots > 0);
+ $cancelledSlotsCount = isset($cancelledSlots) ? $cancelledSlots : 0;
+ return $status || $cancelledSlotsCount > 0;📝 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.
| return $status || (isset($cancelledSlots) && $cancelledSlots > 0); | |
| $cancelledSlotsCount = isset($cancelledSlots) ? $cancelledSlots : 0; | |
| return $status || $cancelledSlotsCount > 0; |
|
@coderabbitai create an issue (not related to this one) but something I've thought about: sequenceDiagram
participant U as User
participant B as Booking System
participant S as Slot System
participant D as Database
rect rgb(240, 240, 240)
Note over S,D: Regeneration Process Started
S->>D: Cancel existing slots
Note right of D: Critical Window<br/>Starts Here
U->>B: Attempt to book slot
B->>D: Check slot availability
Note right of D: Slot appears<br/>'cancelled'
D-->>B: Slot not available
B-->>U: Booking failed
S->>D: Generate new slots
Note right of D: Critical Window<br/>Ends Here
Note over S,D: Same slot would have<br/>been valid before/after<br/>regeneration
end
The diagram shows how a valid booking attempt could fail if it happens during the brief window between slot cancellation and regeneration. This is particularly problematic because:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ThomasAFink I've created a new issue to track this race condition problem in the slot booking system. The issue includes your sequence diagram and explanation, along with some initial investigation points. You'll be notified of the issue creation and can track it separately from this PR. 🏁 Script executed: Length of output: 1807 @ThomasAFink The issue has been created successfully at #894. You can track and discuss the race condition problem there. ✏️ Learnings added
|
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit