fix(ZMS-3253 ZMS-3466 ZMS-3415 ZMS-1891): Replicate frontend validation in the backend for the availability opening hours to improve data integrity and frontend validation messaging #803
Conversation
…orm error messaging
This reverts commit ce5d795.
|
Seems like the problem with adjacent marked as overlapped also exists in checkForOverlaps in AvailabilityConflicts @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the fixed version of the checkForOverlaps method: protected static function checkForOverlaps(AvailabilityList $collection, \DateTimeImmutable $selectedDateTime)
{
$conflicts = new \BO\Zmsentities\Collection\ProcessList();
foreach ($collection as $availability1) {
foreach ($collection as $availability2) {
$scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null);
$scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null);
if ($availability1 !== $availability2 &&
$availability1->type == $availability2->type &&
$scope1Id == $scope2Id &&
$availability1->hasSharedWeekdayWith($availability2)) {
// First check if dates are the same
$date1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)->format('Y-m-d');
$date2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)->format('Y-m-d');
if ($date1 === $date2) {
// Compare times as strings for exact boundary handling
$time1Start = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)
->modify($availability1->startTime)->format('H:i');
$time1End = (new \DateTimeImmutable())->setTimestamp($availability1->endDate)
->modify($availability1->endTime)->format('H:i');
$time2Start = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)
->modify($availability2->startTime)->format('H:i');
$time2End = (new \DateTimeImmutable())->setTimestamp($availability2->endDate)
->modify($availability2->endTime)->format('H:i');
if ($time2Start < $time1End && $time1Start < $time2End) {
$process = new Process();
$process->amendment = 'Neue Öffnungszeiten überschneiden sich.';
$appointment = new \BO\Zmsentities\Appointment();
$appointment->date = $availability1->startDate;
$appointment->availability = $availability1;
$process->addAppointment($appointment);
$conflicts->addEntity($process);
}
}
}
}
}
return $conflicts;
}Key changes:
✏️ Learnings added
|
|
@coderabbitai how can we rename checkForOverlaps so we know it's only comparing new ones and not existing ones give ten examples for names and maybe rename getConflicts for we know it's the check for existing. |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are 10 suggested names for each method: For checkForOverlaps (checking between new availabilities):
For getConflicts (checking against existing availabilities):
My recommendation would be:
This makes it immediately clear which availabilities are being compared in each method. 🐰 thoughtfully taking notes ✏️ Learnings added |
@coderabbitai but getConflicts doesn't just check new against existing it also checks existing against existing. |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink You're absolutely right, I apologize for the confusion. Since getConflicts checks all availabilities against each other (both new vs existing and existing vs existing), a better name would be checkAllAvailabilityOverlaps. This makes it clear that it performs a comprehensive check of all availabilities in the collection, regardless of whether they're new or existing. So the final naming would be:
✏️ Learnings added
|
@coderabbitai now it now longer disables the exact slot 15:00 in the frontend time picker. if two existing availabilities exist 14:00-15:00 and 15:00-16:00 when I go to add a new one I can select 15:00 slot 14:10-14:50 and 15:10-15:00 and are correctly disabled. But should the timestamp sandwiched between two adjacent opening hours be disabled? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Yes, the 15:00 timestamp should be disabled in the time picker when it's sandwiched between two adjacent opening hours. Even though it's technically a boundary point, it's already "allocated" by both the ending time of one availability and the starting time of another. Allowing it to be selected could lead to confusion and potential data integrity issues. Let me know if you'd like me to help implement this change in the time picker's validation logic. ✏️ Learnings added
|
@coderabbitai give full code for this special case |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the updated setExcludeTimesForDay method to handle the boundary timestamps: setExcludeTimesForDay() {
if (this.state.kind === 'exclusion') {
return;
}
var times = [];
const availabilities = [...this.state.availabilityList]
.filter(availability =>
availability.id !== this.state.availability.id &&
availability.type == this.state.availability.type &&
this.isWeekDaySelected(this.state.selectedDate, availability)
)
.sort((a, b) => {
const timeA = moment(a.startTime, 'HH:mm');
const timeB = moment(b.startTime, 'HH:mm');
return timeA.diff(timeB);
});
// First add regular excluded times
availabilities.forEach(availability => {
const startTime = moment(availability.startTime, 'hh:mm')
.add(this.state.availability.slotTimeInMinutes, "m");
const startOnDay = moment(this.state.selectedDate)
.set({"h": startTime.hours(), "m": startTime.minutes()})
.toDate();
const endTime = moment(availability.endTime, 'hh:mm')
.subtract(this.state.availability.slotTimeInMinutes, "m");
const endOnDay = moment(this.state.selectedDate)
.set({"h": endTime.hours(), "m": endTime.minutes()})
.toDate();
var currentTime = new Date(startOnDay);
while (currentTime < endOnDay) {
times.push(new Date(currentTime));
currentTime = moment(currentTime)
.add(this.state.availability.slotTimeInMinutes, "m")
.toDate();
}
times.push(endOnDay);
});
// Then check for and add boundary timestamps between adjacent availabilities
for (let i = 0; i < availabilities.length - 1; i++) {
const current = availabilities[i];
const next = availabilities[i + 1];
const currentEnd = moment(current.endTime, 'HH:mm');
const nextStart = moment(next.startTime, 'HH:mm');
// If they're adjacent (end time of one equals start time of next)
if (currentEnd.format('HH:mm') === nextStart.format('HH:mm')) {
// Add the boundary timestamp to excluded times
const boundaryTime = moment(this.state.selectedDate)
.set({"h": currentEnd.hours(), "m": currentEnd.minutes()})
.toDate();
times.push(boundaryTime);
}
}
this.setState({excludeTimeList: times});
}Key changes:
✏️ Learnings added🧠 Learnings used🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 16480 🧠 Learnings used |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
zmsapi/src/Zmsapi/AvailabilityAdd.php (1)
141-159: 🛠️ Refactor suggestionAdd transaction handling for entity updates.
The entity update logic should be wrapped in a transaction to ensure data consistency.
zmsapi/src/Zmsapi/AvailabilityUpdate.php (3)
104-105: 🛠️ Refactor suggestionRemove commented code and implement proper logging.
Replace commented error logging with proper logging using a logging framework.
Also applies to: 129-130
133-138: 🛠️ Refactor suggestionConsider wrapping updates in a transaction.
Multiple entity updates should be wrapped in a transaction to ensure data consistency.
184-211: 🛠️ Refactor suggestionDuplicate conflict checking logic.
The
checkNewVsNewConflictsmethod is duplicated across multiple classes.zmsentities/src/Zmsentities/Collection/AvailabilityList.php (1)
195-234: 🛠️ Refactor suggestionAdd null checks and improve type safety.
The implementation needs defensive programming to handle edge cases and improve type safety.
The previous review already suggested adding null checks. Additionally:
- Consider using a more specific return type hint:
array{0: ?\DateTimeImmutable, 1: ?\DateTimeImmutable}- Handle potential timezone issues by explicitly setting the timezone when creating DateTime objects.
- public function getDateTimeRangeFromList(\DateTimeImmutable $selectedDate): array + public function getDateTimeRangeFromList(\DateTimeImmutable $selectedDate): array{0: ?\DateTimeImmutable, 1: ?\DateTimeImmutable} { $earliestStartDateTime = null; $latestEndDateTime = null; + + if ($this->count() === 0) { + return [null, null]; + } + + $timezone = new \DateTimeZone(date_default_timezone_get()); foreach ($this as $availability) { + if (!isset($availability->startDate) || !isset($availability->endDate) || + !isset($availability->startTime) || !isset($availability->endTime)) { + continue; + } + // Convert Unix timestamp to date strings - $startDate = (new \DateTimeImmutable())->setTimestamp($availability->startDate)->format('Y-m-d'); - $endDate = (new \DateTimeImmutable())->setTimestamp($availability->endDate)->format('Y-m-d'); + $startDate = (new \DateTimeImmutable('now', $timezone))->setTimestamp($availability->startDate)->format('Y-m-d'); + $endDate = (new \DateTimeImmutable('now', $timezone))->setTimestamp($availability->endDate)->format('Y-m-d'); // Combine date and time for start and end - $startDateTime = new \DateTimeImmutable("{$startDate} {$availability->startTime}"); - $endDateTime = new \DateTimeImmutable("{$endDate} {$availability->endTime}"); + $startDateTime = \DateTimeImmutable::createFromFormat('Y-m-d H:i', "{$startDate} {$availability->startTime}", $timezone); + $endDateTime = \DateTimeImmutable::createFromFormat('Y-m-d H:i', "{$endDate} {$availability->endTime}", $timezone); + + if ($startDateTime === false || $endDateTime === false) { + continue; + }
🧹 Nitpick comments (9)
zmsadmin/js/page/availabilityDay/form/datepicker.js (3)
127-161: Optimize performance and add validation for slot time.The time exclusion logic can be improved for better performance and reliability:
- Use a Set to store unique times instead of an array.
- Add validation for
slotTimeInMinutes.Consider applying these improvements:
var times = []; +const timeSet = new Set(); + +if (!this.state.availability.slotTimeInMinutes) { + console.warn('slotTimeInMinutes is not defined'); + return; +} // ... rest of the filtering and sorting logic ... while (currentTime < endOnDay) { - times.push(new Date(currentTime)); + timeSet.add(new Date(currentTime).getTime()); currentTime = moment(currentTime) .add(this.state.availability.slotTimeInMinutes, "m") .toDate(); } -times.push(endOnDay); +timeSet.add(endOnDay.getTime()); +// Convert Set back to array of Date objects +times = Array.from(timeSet).map(timestamp => new Date(timestamp));
164-180: Improve boundary timestamp handling.The boundary timestamp logic is correct but could be more efficient by utilizing the existing timeSet.
Consider this optimization:
if (currentEnd.format('HH:mm') === nextStart.format('HH:mm')) { const boundaryTime = moment(this.state.selectedDate) .set({"h": currentEnd.hours(), "m": currentEnd.minutes()}) .toDate(); - times.push(boundaryTime); + timeSet.add(boundaryTime.getTime()); }
287-296: Enhance time comparison logic.The current implementation could miss edge cases and can be more concise using moment's built-in comparison methods.
Consider this improvement:
if (!moment(this.state.selectedDate).isSame(moment.unix(this.props.attributes.today), 'day')) { return true; } const currentTime = moment(); const timeToCheck = moment(time); -return timeToCheck.hour() > currentTime.hour() || - (timeToCheck.hour() === currentTime.hour() && timeToCheck.minute() > currentTime.minute()); +return timeToCheck.isSameOrAfter(currentTime, 'minute');zmsdb/src/Zmsdb/Process.php (1)
495-495: Method name change improves clarity.The renamed method
checkAllVsExistingConflictsbetter describes its functionality compared to the previousgetConflicts.Consider adding PHPDoc comments to document the method's behavior and return type.
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (3)
35-41: Robust input validation improves error handling.The added validation checks ensure all required data is present and valid before processing.
Consider extracting these validation checks into a separate method for better reusability:
+ protected function validateInput($input): void + { + if (!isset($input['availabilityList']) || !is_array($input['availabilityList'])) { + throw new BadRequestException('Missing or invalid availabilityList.'); + } else if(!isset($input['availabilityList'][0]['scope'])){ + throw new BadRequestException('Missing or invalid scope.'); + } else if (!isset($input['selectedDate'])) { + throw new BadRequestException("'selectedDate' is required."); + } + }
49-82: Improved conflict detection with better filtering.The code now properly handles filtering of availabilities based on kind and ID, making conflict detection more accurate.
Consider extracting the filtering logic into a separate method for better maintainability:
+ protected function filterAvailabilities(AvailabilityList $list, ?string $originId = null): AvailabilityList + { + $filtered = new AvailabilityList(); + foreach ($list as $availability) { + if ((!isset($availability->kind) || $availability->kind !== 'exclusion') && + (!isset($availability->id) || $availability->id !== $originId)) { + $filtered->addEntity($availability); + } + } + return $filtered; + }
106-149: Well-structured overlap detection logic.The new
checkNewVsNewConflictsmethod thoroughly checks for overlaps between availabilities, including proper scope comparison and time boundary handling.Consider adding early returns to reduce nesting:
protected static function checkNewVsNewConflicts(AvailabilityList $collection, \DateTimeImmutable $selectedDateTime) { $conflicts = new \BO\Zmsentities\Collection\ProcessList(); foreach ($collection as $availability1) { foreach ($collection as $availability2) { if ($availability1 === $availability2) continue; $scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null); $scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null); - if ($availability1 !== $availability2 && - $availability1->type == $availability2->type && - $scope1Id == $scope2Id && - $availability1->hasSharedWeekdayWith($availability2)) { + if ($availability1->type != $availability2->type || + $scope1Id != $scope2Id || + !$availability1->hasSharedWeekdayWith($availability2)) { + continue; + }zmsentities/src/Zmsentities/Collection/AvailabilityList.php (2)
164-193: Enhance input validation and performance.The validation implementation is well-structured but could benefit from some improvements:
public function validateInputs(\DateTimeImmutable $startDate, \DateTimeImmutable $endDate, \DateTimeImmutable $selectedDate, string $kind, $startInDays, $endInDays): array { + // Early return for empty collection + if ($this->count() === 0) { + return []; + } + + // Validate kind parameter + $allowedKinds = ['appointment', 'openinghours']; // Add all valid kinds + if (!in_array($kind, $allowedKinds)) { + return ['Invalid validation kind specified']; + } + $errorList = []; $today = new \DateTimeImmutable(); $yesterday = $selectedDate->modify('-1 day'); $tomorrow = $selectedDate->modify('+1 day'); + // Pre-calculate validation parameters to avoid repeated calculations + $validationContext = [ + 'today' => $today, + 'yesterday' => $yesterday, + 'tomorrow' => $tomorrow, + 'startInDays' => (int)$startInDays, + 'endInDays' => (int)$endInDays + ]; foreach ($this as $availability) { $errorList = array_merge($errorList, $availability->validateStartTime($today, $tomorrow, $startDate, $endDate, $selectedDate, $kind), $availability->validateEndTime($startDate, $endDate), $availability->validateOriginEndTime($today, $yesterday, $startDate, $endDate, $selectedDate, $kind), $availability->validateType($kind), $availability->validateSlotTime($startDate, $endDate), - $availability->validateBookableDayRange((int)$startInDays, (int)$endInDays) + $availability->validateBookableDayRange($validationContext['startInDays'], $validationContext['endInDays']) ); } return $errorList; }
Line range hint
236-266: Optimize conflict detection and improve documentation.The conflict detection implementation could be optimized and better documented.
+ /** + * Checks for conflicts between all availabilities in the list for a given date range + * + * @param \DateTimeInterface $startDate The start date of the range to check + * @param \DateTimeInterface $endDate The end date of the range to check + * @return ProcessList List of conflicts found + */ public function checkAllVsExistingConflicts($startDate, $endDate) { $processList = new ProcessList(); + // Pre-filter availabilities that overlap with the date range + $relevantAvailabilities = array_filter($this->getArrayCopy(), function($availability) use ($startDate, $endDate) { + return $availability->startDate <= $endDate->getTimestamp() && + $availability->endDate >= $startDate->getTimestamp(); + }); + - foreach ($this as $availability) { + foreach ($relevantAvailabilities as $availability) { $conflict = $availability->getConflict(); $currentDate = $startDate; while ($currentDate <= $endDate) {Consider using a more efficient data structure or algorithm for large datasets, such as an interval tree, to reduce the time complexity from O(n²) to O(n log n).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
zmsadmin/js/page/availabilityDay/form/datepicker.js(3 hunks)zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(4 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(3 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(4 hunks)zmsdb/src/Zmsdb/Process.php(1 hunks)zmsdb/tests/Zmsdb/ProcessConflictTest.php(3 hunks)zmsentities/src/Zmsentities/Availability.php(5 hunks)zmsentities/src/Zmsentities/Collection/AvailabilityList.php(2 hunks)zmsentities/tests/Zmsentities/AvailabilityTest.php(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsdb/tests/Zmsdb/ProcessConflictTest.php
- zmsentities/src/Zmsentities/Availability.php
🔇 Additional comments (9)
zmsadmin/js/page/availabilityDay/form/datepicker.js (1)
Line range hint
23-23: Update maxTime to 22:00.The maxTime in the constructor should be set to 22:00 as previously discussed.
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (2)
9-11: LGTM! Improved error handling with specific exceptions.The addition of BadRequestException and explicit imports improves error handling clarity.
151-157: Well-documented method with clear return type.The PHPDoc for
getAvailabilityListclearly describes parameters and return type.zmsentities/src/Zmsentities/Collection/AvailabilityList.php (1)
21-21: Verify data migration for workstationCount structure change.The change from direct property access to array access suggests a data structure change. Ensure that all existing workstationCount values have been migrated to the new array format with 'intern' key.
zmsentities/tests/Zmsentities/AvailabilityTest.php (5)
199-199: LGTM! Consistent with previous change.The update to 6534000 seconds is consistent with the change in
testGetAvailableSecondsPerDay(), maintaining test consistency.
Line range hint
870-957: LGTM! Improved conflict detection test coverage.The changes enhance the conflict detection tests with:
- More comprehensive test cases for overlapping availabilities
- Clear and descriptive error messages including specific dates and times
- Additional assertions for exact matches and slot size conflicts
- Improved structure for conflict list handling
These improvements will help ensure robust validation of availability conflicts.
501-501: Verify the increased public workstation count.The expected public workstation count has been increased from 81 to 99. Please confirm this significant increase aligns with the capacity planning requirements.
✅ Verification successful
Test assertion for calculated workstation count is valid
The change from 81 to 99 workstations is an intentional update to the test case that verifies the
withCalculatedSlots()calculation logic. This is a unit test asserting the expected behavior, not a production capacity configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for related workstation count calculations # Look for other tests or implementations using workstation counts rg -A 5 "workstationCount.*public.*[89][19]" # Look for the implementation of withCalculatedSlots ast-grep --pattern 'function withCalculatedSlots() { $$$ }'Length of output: 110
Script:
#!/bin/bash # Search for workstation count references and calculations rg -l "workstationCount.*public" # Get the content of the test file to understand the context cat zmsentities/tests/Zmsentities/AvailabilityTest.php # Look for other availability or capacity related tests fd "Test.php" | grep -i "availability\|capacity"Length of output: 41271
510-512: Verify the updated slot calculations.The changes include:
- Increased total slot count from 28 to 33
- Changed second slot time from '10:12' to '10:10'
Please confirm these updates align with the intended scheduling intervals and operating hours.
✅ Verification successful
Slot calculation changes are verified and consistent ✓
The modifications align with the system's design:
- 10-minute slot intervals are confirmed across multiple test files
- The increase to 33 slots maintains the consistent 10-minute interval pattern
- The second slot timing (10:10) correctly follows the first slot (10:00)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for related slot calculations # Look for slot time calculations ast-grep --pattern 'function getSlotList() { $$$ }' # Look for other tests using these specific times rg -A 5 "10:10|10:12"Length of output: 5421
Script:
#!/bin/bash # Search for slot-related implementations and configurations echo "=== Searching for slot-related files ===" fd -t f -e php slot echo -e "\n=== Searching for slot calculations and configurations ===" rg -l "slot|interval|duration" --type php echo -e "\n=== Looking for related test assertions ===" rg "assertEquals.*slot|assertTrue.*slot" --type phpLength of output: 11898
182-182: Verify the updated available seconds per day value.The expected value has been increased from 5346000 to 6534000 seconds (approximately 1.8 hours difference). Please confirm this aligns with the business requirements for daily availability calculations.
| if (!isset($input['availabilityList']) || !is_array($input['availabilityList'])) { | ||
| throw new BadRequestException('Missing or invalid availabilityList.'); | ||
| } else if (!isset($input['availabilityList'][0]['scope'])) { | ||
| throw new BadRequestException('Missing or invalid scope.'); | ||
| } else if (!isset($input['selectedDate'])) { | ||
| throw new BadRequestException("'selectedDate' is required."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate input validation logic.
This validation logic is duplicated across multiple classes (AvailabilityAdd, AvailabilityUpdate, AvailabilityConflicts).
Extract the validation logic into a shared trait:
trait AvailabilityValidationTrait {
protected function validateAvailabilityInput(array $input): void {
if (!isset($input['availabilityList']) || !is_array($input['availabilityList'])) {
throw new BadRequestException('Missing or invalid availabilityList.');
} else if (!isset($input['availabilityList'][0]['scope'])) {
throw new BadRequestException('Missing or invalid scope.');
} else if (!isset($input['selectedDate'])) {
throw new BadRequestException("'selectedDate' is required.");
}
}
}| protected function checkNewVsNewConflicts(Collection $collection, \DateTimeImmutable $selectedDate): void | ||
| { | ||
| foreach ($collection as $availability1) { | ||
| foreach ($collection as $availability2) { | ||
| $scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null); | ||
| $scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null); | ||
|
|
||
| if ($availability1 !== $availability2 && | ||
| $availability1->type == $availability2->type && | ||
| $scope1Id == $scope2Id && | ||
| $availability1->hasSharedWeekdayWith($availability2)) { | ||
|
|
||
| $start1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate) | ||
| ->modify($availability1->startTime); | ||
| $end1 = (new \DateTimeImmutable())->setTimestamp($availability1->endDate) | ||
| ->modify($availability1->endTime); | ||
| $start2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate) | ||
| ->modify($availability2->startTime); | ||
| $end2 = (new \DateTimeImmutable())->setTimestamp($availability2->endDate) | ||
| ->modify($availability2->endTime); | ||
|
|
||
| if ($start1 < $end2 && $start2 < $end1) { | ||
| throw new AvailabilityAddFailed('Neue Öffnungszeiten überschneiden sich.'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate conflict checking logic.
The checkNewVsNewConflicts method is duplicated across AvailabilityAdd and AvailabilityUpdate classes.
Extract this logic into a shared trait or base class:
trait AvailabilityConflictTrait {
protected function checkNewVsNewConflicts(Collection $collection, \DateTimeImmutable $selectedDate): void {
// Move the entire checkNewVsNewConflicts method here
}
}
|
@coderabbitai when two new opening hours are overlapping I only get Folgende Zeiten führen mit der aktuellen Auswahl zu Konflikten: but I should be getting which ones like when comparing a new one to an existing one. Folgende Zeiten führen mit der aktuellen Auswahl zu Konflikten: |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the fix for the checkNewVsNewConflicts method in AvailabilityConflicts: protected static function checkNewVsNewConflicts(AvailabilityList $collection, \DateTimeImmutable $selectedDateTime)
{
$conflicts = new \BO\Zmsentities\Collection\ProcessList();
foreach ($collection as $availability1) {
foreach ($collection as $availability2) {
$scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null);
$scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null);
if ($availability1 !== $availability2 &&
$availability1->type == $availability2->type &&
$scope1Id == $scope2Id &&
$availability1->hasSharedWeekdayWith($availability2)) {
// First check if dates are the same
$date1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)->format('Y-m-d');
$date2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)->format('Y-m-d');
if ($date1 === $date2) {
// Compare times as strings for exact boundary handling
$time1Start = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)
->modify($availability1->startTime)->format('H:i');
$time1End = (new \DateTimeImmutable())->setTimestamp($availability1->endDate)
->modify($availability1->endTime)->format('H:i');
$time2Start = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)
->modify($availability2->startTime)->format('H:i');
$time2End = (new \DateTimeImmutable())->setTimestamp($availability2->endDate)
->modify($availability2->endTime)->format('H:i');
if ($time2Start < $time1End && $time1Start < $time2End) {
$process = new Process();
$dateRange1 = date('d.m.Y', $availability1->startDate) . ' - ' . date('d.m.Y', $availability1->endDate);
$dateRange2 = date('d.m.Y', $availability2->startDate) . ' - ' . date('d.m.Y', $availability2->endDate);
$timeRange1 = $time1Start . ' - ' . $time1End;
$timeRange2 = $time2Start . ' - ' . $time2End;
$process->amendment = "Konflikt: Zwei Öffnungszeiten überschneiden sich.\n"
. "Bestehende Öffnungszeit:  [$dateRange1, $timeRange1]\n"
. "Neue Öffnungszeit:                 [$dateRange2, $timeRange2]";
$appointment = new \BO\Zmsentities\Appointment();
$appointment->date = $availability1->startDate;
$appointment->availability = $availability1;
$process->addAppointment($appointment);
$conflicts->addEntity($process);
}
}
}
}
}
return $conflicts;
}Key changes:
✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 42182 |
@coderabbitai now when I compare new to existing I get double: shouldn't checkNewVsNewConflicts only be checking new against new? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the fix to prevent duplicate conflict messages: protected static function checkNewVsNewConflicts(AvailabilityList $collection, \DateTimeImmutable $selectedDateTime)
{
$conflicts = new \BO\Zmsentities\Collection\ProcessList();
$processedPairs = [];
foreach ($collection as $availability1) {
foreach ($collection as $availability2) {
// Create a unique key for this pair (sort IDs to ensure same key regardless of order)
$id1 = $availability1->id ?? $availability1->tempId;
$id2 = $availability2->id ?? $availability2->tempId;
$pairKey = $id1 < $id2 ? "$id1-$id2" : "$id2-$id1";
// Skip if we've already processed this pair or if it's the same availability
if ($availability1 === $availability2 || isset($processedPairs[$pairKey])) {
continue;
}
$processedPairs[$pairKey] = true;
$scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null);
$scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null);
if ($availability1->type == $availability2->type &&
$scope1Id == $scope2Id &&
$availability1->hasSharedWeekdayWith($availability2)) {
// First check if dates are the same
$date1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)->format('Y-m-d');
$date2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)->format('Y-m-d');
if ($date1 === $date2) {
// Compare times as strings for exact boundary handling
$time1Start = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)
->modify($availability1->startTime)->format('H:i');
$time1End = (new \DateTimeImmutable())->setTimestamp($availability1->endDate)
->modify($availability1->endTime)->format('H:i');
$time2Start = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)
->modify($availability2->startTime)->format('H:i');
$time2End = (new \DateTimeImmutable())->setTimestamp($availability2->endDate)
->modify($availability2->endTime)->format('H:i');
if ($time2Start < $time1End && $time1Start < $time2End) {
$process = new Process();
$dateRange1 = date('d.m.Y', $availability1->startDate) . ' - ' . date('d.m.Y', $availability1->endDate);
$dateRange2 = date('d.m.Y', $availability2->startDate) . ' - ' . date('d.m.Y', $availability2->endDate);
$timeRange1 = $time1Start . ' - ' . $time1End;
$timeRange2 = $time2Start . ' - ' . $time2End;
$process->amendment = "Konflikt: Zwei Öffnungszeiten überschneiden sich.\n"
. "Bestehende Öffnungszeit:  [$dateRange1, $timeRange1]\n"
. "Neue Öffnungszeit:                 [$dateRange2, $timeRange2]";
$appointment = new \BO\Zmsentities\Appointment();
$appointment->date = $availability1->startDate;
$appointment->availability = $availability1;
$process->addAppointment($appointment);
$conflicts->addEntity($process);
}
}
}
}
}
return $conflicts;
}Key changes:
✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink You're absolutely right. Here's the corrected version that only checks new availabilities against each other: protected static function checkNewVsNewConflicts(AvailabilityList $collection, \DateTimeImmutable $selectedDateTime)
{
$conflicts = new \BO\Zmsentities\Collection\ProcessList();
// Filter to only get new availabilities
$newAvailabilities = new AvailabilityList();
foreach ($collection as $availability) {
if (isset($availability->tempId) && strpos($availability->tempId, '__temp__') !== false) {
$newAvailabilities->addEntity($availability);
}
}
foreach ($newAvailabilities as $availability1) {
foreach ($newAvailabilities as $availability2) {
$scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null);
$scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null);
if ($availability1 !== $availability2 &&
$availability1->type == $availability2->type &&
$scope1Id == $scope2Id &&
$availability1->hasSharedWeekdayWith($availability2)) {
// First check if dates are the same
$date1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)->format('Y-m-d');
$date2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)->format('Y-m-d');
if ($date1 === $date2) {
// Compare times as strings for exact boundary handling
$time1Start = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)
->modify($availability1->startTime)->format('H:i');
$time1End = (new \DateTimeImmutable())->setTimestamp($availability1->endDate)
->modify($availability1->endTime)->format('H:i');
$time2Start = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)
->modify($availability2->startTime)->format('H:i');
$time2End = (new \DateTimeImmutable())->setTimestamp($availability2->endDate)
->modify($availability2->endTime)->format('H:i');
if ($time2Start < $time1End && $time1Start < $time2End) {
$process = new Process();
$dateRange1 = date('d.m.Y', $availability1->startDate) . ' - ' . date('d.m.Y', $availability1->endDate);
$dateRange2 = date('d.m.Y', $availability2->startDate) . ' - ' . date('d.m.Y', $availability2->endDate);
$timeRange1 = $time1Start . ' - ' . $time1End;
$timeRange2 = $time2Start . ' - ' . $time2End;
$process->amendment = "Konflikt: Zwei Öffnungszeiten überschneiden sich.\n"
. "Bestehende Öffnungszeit:  [$dateRange1, $timeRange1]\n"
. "Neue Öffnungszeit:                 [$dateRange2, $timeRange2]";
$appointment = new \BO\Zmsentities\Appointment();
$appointment->date = $availability1->startDate;
$appointment->availability = $availability1;
$process->addAppointment($appointment);
$conflicts->addEntity($process);
}
}
}
}
}
return $conflicts;
}Key changes:
✏️ Learnings added
|
@coderabbitai does checkNewVsNewConflicts in AvailabilityAdd and AvailabilityUpdate also only check new ones? Can you double check? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink No, the checkNewVsNewConflicts methods in both AvailabilityAdd and AvailabilityUpdate are not properly filtering for new availabilities. They're currently checking all availabilities in the collection against each other. Here's how they should be modified to match the AvailabilityConflicts implementation: protected function checkNewVsNewConflicts(Collection $collection, \DateTimeImmutable $selectedDate): void
{
// Filter to only get new availabilities
$newAvailabilities = new Collection();
foreach ($collection as $availability) {
if (isset($availability->tempId) && strpos($availability->tempId, '__temp__') !== false) {
$newAvailabilities->addEntity($availability);
}
}
foreach ($newAvailabilities as $availability1) {
foreach ($newAvailabilities as $availability2) {
$scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null);
$scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null);
if ($availability1 !== $availability2 &&
$availability1->type == $availability2->type &&
$scope1Id == $scope2Id &&
$availability1->hasSharedWeekdayWith($availability2)) {
// First check if dates are the same
$date1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)->format('Y-m-d');
$date2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)->format('Y-m-d');
if ($date1 === $date2) {
$time1Start = (new \DateTimeImmutable())->setTimestamp($availability1->startDate)
->modify($availability1->startTime)->format('H:i');
$time1End = (new \DateTimeImmutable())->setTimestamp($availability1->endDate)
->modify($availability1->endTime)->format('H:i');
$time2Start = (new \DateTimeImmutable())->setTimestamp($availability2->startDate)
->modify($availability2->startTime)->format('H:i');
$time2End = (new \DateTimeImmutable())->setTimestamp($availability2->endDate)
->modify($availability2->endTime)->format('H:i');
if ($time2Start < $time1End && $time1Start < $time2End) {
throw new AvailabilityAddFailed('Neue Öffnungszeiten überschneiden sich.'); // or AvailabilityUpdateFailed
}
}
}
}
}
}This change will ensure that only new availabilities (those with tempId containing 'temp') are checked against each other. ✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 17865 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
zmsapi/src/Zmsapi/AvailabilityUpdate.php (2)
145-166: 🛠️ Refactor suggestionAdd transaction management for entity updates.
The entity update logic should be wrapped in a transaction to ensure data consistency, especially when handling multiple entities.
104-105: 🛠️ Refactor suggestionAdd logging for validation failures and conflicts.
Important error conditions should be logged for debugging and monitoring.
Also applies to: 128-129
🧹 Nitpick comments (2)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (1)
61-75: Consider optimizing the filtering logic to reduce iterations.The current implementation requires multiple iterations over the collection. Consider combining the origin ID search and filtering into a single pass.
- $originId = null; - foreach ($availabilityList as $availability) { - if (isset($availability->kind) && $availability->kind === 'origin' && isset($availability->id)) { - $originId = $availability->id; - break; - } - } - - $filteredAvailabilityList = new AvailabilityList(); - foreach ($availabilityList as $availability) { - if ((!isset($availability->kind) || $availability->kind !== 'exclusion') && - (!isset($availability->id) || $availability->id !== $originId)) { - $filteredAvailabilityList->addEntity($availability); - } - } + $originId = null; + $filteredAvailabilityList = new AvailabilityList(); + foreach ($availabilityList as $availability) { + if (isset($availability->kind) && $availability->kind === 'origin' && isset($availability->id)) { + $originId = $availability->id; + } + if ((!isset($availability->kind) || $availability->kind !== 'exclusion') && + (!isset($availability->id) || $availability->id !== $originId)) { + $filteredAvailabilityList->addEntity($availability); + } + }zmsapi/src/Zmsapi/AvailabilityUpdate.php (1)
43-55: Consider extracting shared validation logic.While the validation is thorough, this logic is duplicated with AvailabilityConflicts.php. Consider extracting it into a shared trait or base class.
trait AvailabilityValidationTrait { protected function validateInput(array $input): void { if (!$input || count($input) === 0) { throw new BadRequestException(); } if (!isset($input['availabilityList']) || !is_array($input['availabilityList'])) { throw new BadRequestException('Missing or invalid availabilityList.'); } // ... rest of validation logic } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(4 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(3 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsapi/src/Zmsapi/AvailabilityAdd.php
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: call-build-images / build (zmsticketprinter, 8.0)
- GitHub Check: call-build-images / build (zmsstatistic, 8.0)
- GitHub Check: call-build-images / build (zmsmessaging, 8.0)
- GitHub Check: call-build-images / build (zmscitizenapi, 8.0)
- GitHub Check: call-build-images / build (zmscalldisplay, 8.0)
- GitHub Check: call-build-images / build (zmsapi, 8.0)
- GitHub Check: call-build-images / build (zmsadmin, 8.0)
🔇 Additional comments (3)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (3)
35-41: LGTM! Input validation is thorough and well-structured.The validation checks are comprehensive and follow a logical order, with clear error messages for each failure case.
165-171: LGTM! Documentation is clear and follows PHPDoc standards.The method documentation properly describes parameters and return types.
105-163: Consider optimizing conflict detection performance.The nested loops in the conflict detection could become a performance bottleneck with a large number of availabilities. Consider:
- Using a time-based indexing structure
- Early exit when conflicts are found
- Reducing the number of DateTime object creations
Also, verify edge cases:
- Availabilities spanning midnight
- DST transitions
- Timezone differences
| protected function checkNewVsNewConflicts(Collection $collection, \DateTimeImmutable $selectedDate): void | ||
| { | ||
| $newAvailabilities = new Collection(); | ||
| foreach ($collection as $availability) { | ||
| if (isset($availability->tempId) && strpos($availability->tempId, '__temp__') !== false) { | ||
| $newAvailabilities->addEntity($availability); | ||
| } | ||
| } | ||
|
|
||
| foreach ($collection as $availability1) { | ||
| foreach ($collection as $availability2) { | ||
| $scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null); | ||
| $scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null); | ||
|
|
||
| if ($availability1 !== $availability2 && | ||
| $availability1->type == $availability2->type && | ||
| $scope1Id == $scope2Id && | ||
| $availability1->hasSharedWeekdayWith($availability2)) { | ||
|
|
||
| $start1 = (new \DateTimeImmutable())->setTimestamp($availability1->startDate) | ||
| ->modify($availability1->startTime); | ||
| $end1 = (new \DateTimeImmutable())->setTimestamp($availability1->endDate) | ||
| ->modify($availability1->endTime); | ||
| $start2 = (new \DateTimeImmutable())->setTimestamp($availability2->startDate) | ||
| ->modify($availability2->startTime); | ||
| $end2 = (new \DateTimeImmutable())->setTimestamp($availability2->endDate) | ||
| ->modify($availability2->endTime); | ||
|
|
||
| if ($start1 < $end2 && $start2 < $end1) { | ||
| throw new AvailabilityUpdateFailed('Neue Öffnungszeiten überschneiden sich.'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate conflict checking logic across classes.
The conflict checking logic is similar to AvailabilityConflicts.php but with different implementations. Consider:
- Extracting shared logic to reduce duplication
- Standardizing error messages across both classes
- Using a common interface for conflict detection
interface ConflictDetectorInterface {
public function checkNewVsNewConflicts(Collection $collection, \DateTimeImmutable $selectedDate): void;
}
trait ConflictDetectorTrait {
public function checkNewVsNewConflicts(Collection $collection, \DateTimeImmutable $selectedDate): void {
// Shared implementation
}
}
|
@coderabbitai can we have |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the corrected FooterButtons component with the simplified disabled condition for the Abort button: const FooterButtons = (props) => {
const { hasConflicts, stateChanged, data, onNew, onPublish, onAbort, hasSlotCountError } = props
return (
<div className="form-actions" style={{ "marginTop": "0", "padding": "0.75em" }}>
<button title="Neue Öffnungszeit anlegen und bearbeiten" className="button button--diamond button-new" onClick={onNew} disabled={(stateChanged || hasConflicts || data)}>neue Öffnungszeit</button>
<button title="Alle Änderungen werden zurückgesetzt" className="button btn" type="abort" onClick={onAbort} disabled={!stateChanged}>Abbrechen</button>
<button title="Alle Änderungen werden gespeichert" className="button button--positive button-save" type="save" value="publish" onClick={onPublish} disabled={(!stateChanged || hasSlotCountError || hasConflicts)}>Alle Änderungen aktivieren
</button>
</div>
)
}Key change:
This will keep the button enabled as long as there are unsaved changes in the system. ✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 3638 |
@coderabbitai stateChanged only tracks if something in one of the forms has changed it doesn't track if I add a new availability. So if I add a new availability and haven'T changed any of the forms stateChanged is still false and therefore the button remains disabled. |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the fixed FooterButtons component: const FooterButtons = (props) => {
const { hasConflicts, stateChanged, data, onNew, onPublish, onAbort, hasSlotCountError, availabilitylist } = props;
// Check if there are any new availabilities (with tempId)
const hasNewAvailabilities = availabilitylist && availabilitylist.some(
availability => availability.tempId && availability.tempId.includes('__temp__')
);
return (
<div className="form-actions" style={{ "marginTop": "0", "padding": "0.75em" }}>
<button
title="Neue Öffnungszeit anlegen und bearbeiten"
className="button button--diamond button-new"
onClick={onNew}
disabled={(stateChanged || hasConflicts || data)}
>
neue Öffnungszeit
</button>
<button
title="Alle Änderungen werden zurückgesetzt"
className="button btn"
type="abort"
onClick={onAbort}
disabled={!stateChanged && !hasNewAvailabilities && !hasConflicts}
>
Abbrechen
</button>
<button
title="Alle Änderungen werden gespeichert"
className="button button--positive button-save"
type="save"
value="publish"
onClick={onPublish}
disabled={(!stateChanged && !hasNewAvailabilities) || hasSlotCountError || hasConflicts}
>
Alle Änderungen aktivieren
</button>
</div>
)
}
FooterButtons.propTypes = {
data: PropTypes.object,
hasConflicts: PropTypes.bool,
stateChanged: PropTypes.bool,
onNew: PropTypes.func,
onPublish: PropTypes.func,
onAbort: PropTypes.func,
hasSlotCountError: PropTypes.bool,
availabilitylist: PropTypes.array
}
export default FooterButtonsKey changes:
✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 41492 |
@coderabbitai and in here? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Yes, there's a prop name mismatch. Here's the corrected FooterButtons component: const FooterButtons = (props) => {
const { hasConflicts, stateChanged, data, onNew, onPublish, onAbort, hasSlotCountError, availabilityList } = props;
// Check if there are any new availabilities (with tempId)
const hasNewAvailabilities = availabilityList && availabilityList.some(
availability => availability.tempId && availability.tempId.includes('__temp__')
);
return (
<div className="form-actions" style={{ "marginTop": "0", "padding": "0.75em" }}>
<button
title="Neue Öffnungszeit anlegen und bearbeiten"
className="button button--diamond button-new"
onClick={onNew}
disabled={(stateChanged || hasConflicts || data)}
>
neue Öffnungszeit
</button>
<button
title="Alle Änderungen werden zurückgesetzt"
className="button btn"
type="abort"
onClick={onAbort}
disabled={!stateChanged && !hasNewAvailabilities && !hasConflicts}
>
Abbrechen
</button>
<button
title="Alle Änderungen werden gespeichert"
className="button button--positive button-save"
type="save"
value="publish"
onClick={onPublish}
disabled={(!stateChanged && !hasNewAvailabilities) || hasSlotCountError || hasConflicts}
>
Alle Änderungen aktivieren
</button>
</div>
)
}
FooterButtons.propTypes = {
data: PropTypes.object,
hasConflicts: PropTypes.bool,
stateChanged: PropTypes.bool,
onNew: PropTypes.func,
onPublish: PropTypes.func,
onAbort: PropTypes.func,
hasSlotCountError: PropTypes.bool,
availabilityList: PropTypes.array // Changed from availabilitylist to availabilityList
}
export default FooterButtonsThe key change is renaming the prop from ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
zmsadmin/js/page/availabilityDay/layouts/accordion.js (1)
163-165: Maintain consistent prop naming convention.The prop name
availabilitylistuses inconsistent casing compared to the existingavailabilityListprop. This could lead to confusion and maintenance issues.Apply this diff to maintain consistency:
- availabilitylist={this.props.availabilityList} + availabilityList={this.props.availabilityList}zmsadmin/js/page/availabilityDay/form/footerButtons.js (3)
9-9: Extract magic string to a constant.The string
'__temp__'should be extracted to a named constant for better maintainability and reusability.Add this constant at the top of the file:
+const TEMP_ID_PREFIX = '__temp__';Then update the check:
- availability => availability.tempId && availability.tempId.includes('__temp__') + availability => availability?.tempId?.includes(TEMP_ID_PREFIX)🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
27-27: Simplify complex boolean expressions.The disabled conditions are complex and could be simplified for better readability.
Extract the conditions to descriptive variables:
+ const canAbort = stateChanged || hasNewAvailabilities || hasConflicts; + const canSave = (stateChanged || hasNewAvailabilities) && !hasSlotCountError && !hasConflicts; - disabled={!stateChanged && !hasNewAvailabilities && !hasConflicts} + disabled={!canAbort} - disabled={(!stateChanged && !hasNewAvailabilities) || hasSlotCountError || hasConflicts} + disabled={!canSave}Also applies to: 37-37
51-53: Update prop name to maintain consistency.The prop name should follow the same casing convention as in the parent component.
Apply this diff:
- availabilitylist: PropTypes.array + availabilityList: PropTypes.array
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsadmin/js/page/availabilityDay/form/footerButtons.js(2 hunks)zmsadmin/js/page/availabilityDay/layouts/accordion.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
zmsadmin/js/page/availabilityDay/form/footerButtons.js
[error] 8-10: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 9-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-unit-tests / zmsapi-test
- GitHub Check: call-unit-tests / zmsdb-test
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Based on the comprehensive summary of changes across multiple files, here are the high-level release notes:
New Features
Bug Fixes
Improvements
Performance
These changes primarily focus on improving the reliability and user experience of availability management in the system.