Bugfix zms 3253 3466 3415 1891 validation opening hours of the same appointment type must not overlap #810
Conversation
This file contains 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
added 30 commits
November 12, 2024 12:25
…orm error messaging
This reverts commit ce5d795.
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are the missing overlap test cases we should add to AvailabilityTest: public function testPartialOverlaps()
{
$entity1 = new Availability([
'id' => '1',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '08:00:00',
'endTime' => '12:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test overlap at start
$entity2 = new Availability([
'id' => '2',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '07:00:00',
'endTime' => '09:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test overlap at end
$entity3 = new Availability([
'id' => '3',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '11:00:00',
'endTime' => '13:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test completely contained
$entity4 = new Availability([
'id' => '4',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '09:00:00',
'endTime' => '11:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$entity1, $entity2, $entity3, $entity4]);
$conflicts = $list->checkAllVsExistingConflicts(
new \DateTimeImmutable('2024-01-15 00:00:00'),
new \DateTimeImmutable('2024-01-15 23:59:59')
);
$this->assertEquals(3, $conflicts->count(), "Should detect all three types of overlaps");
}
public function testEdgeCaseOverlaps()
{
// Test back-to-back times
$entity1 = new Availability([
'id' => '1',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '08:00:00',
'endTime' => '12:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
$entity2 = new Availability([
'id' => '2',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '12:00:00',
'endTime' => '16:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test overlap across midnight
$entity3 = new Availability([
'id' => '3',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-16'),
'startTime' => '23:00:00',
'endTime' => '01:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$entity1, $entity2, $entity3]);
$conflicts = $list->checkAllVsExistingConflicts(
new \DateTimeImmutable('2024-01-15 00:00:00'),
new \DateTimeImmutable('2024-01-16 23:59:59')
);
$this->assertEquals(0, $conflicts->count(), "Back-to-back times should not be considered overlapping");
}
public function testWeekdayOverlaps()
{
// Test same weekday, different weeks
$entity1 = new Availability([
'id' => '1',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '08:00:00',
'endTime' => '12:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []],
'repeat' => ['afterWeeks' => '1']
]);
$entity2 = new Availability([
'id' => '2',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-22'),
'endDate' => strtotime('2024-01-22'),
'startTime' => '08:00:00',
'endTime' => '12:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []],
'repeat' => ['afterWeeks' => '2']
]);
// Test different weekdays, same times
$entity3 = new Availability([
'id' => '3',
'weekday' => ['tuesday' => '3'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '08:00:00',
'endTime' => '12:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$entity1, $entity2, $entity3]);
$conflicts = $list->checkAllVsExistingConflicts(
new \DateTimeImmutable('2024-01-15 00:00:00'),
new \DateTimeImmutable('2024-01-22 23:59:59')
);
$this->assertEquals(0, $conflicts->count(), "Different weeks or weekdays should not conflict");
}These tests cover:
Would you like me to create an issue to track adding these tests? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 50506 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (4)
42-49: Consider using early returns for better readability.The conflict detection setup could be more readable with early returns and intermediate variable names that describe their purpose.
Consider this structure:
- $conflictedList = []; - - $availabilityList = (new AvailabilityList())->addData($input['availabilityList']); - $selectedDateTime = (new \DateTimeImmutable($input['selectedDate']))->modify(\App::$now->format('H:i:s')); - - $conflictList = new \BO\Zmsentities\Collection\ProcessList(); - $overlapConflicts = $availabilityList->hasNewVsNewConflicts($selectedDateTime); - $conflictList->addList($overlapConflicts); + $conflictedList = []; + $selectedDateTime = (new \DateTimeImmutable($input['selectedDate'])) + ->modify(\App::$now->format('H:i:s')); + + $availabilityList = (new AvailabilityList()) + ->addData($input['availabilityList']); + + // Check for conflicts between new availabilities + $conflictList = new \BO\Zmsentities\Collection\ProcessList(); + $newVsNewConflicts = $availabilityList->hasNewVsNewConflicts($selectedDateTime); + if ($newVsNewConflicts->count() > 0) { + $conflictList->addList($newVsNewConflicts); + return [ + 'conflictList' => $conflictList->toConflictListByDay(), + 'conflictIdList' => $this->extractConflictIds($newVsNewConflicts) + ]; + }
60-74: Extract filtering logic into a dedicated method.The filtering logic for exclusions and origin IDs could be moved to a separate method for better maintainability.
Consider this refactor:
+ /** + * Filter out exclusions and original availability from the list + * + * @param AvailabilityList $availabilityList + * @param string|null $originId + * @return AvailabilityList + */ + protected function filterAvailabilityList(AvailabilityList $availabilityList, ?string $originId): AvailabilityList + { + $filteredList = new AvailabilityList(); + foreach ($availabilityList as $availability) { + if ((!isset($availability->kind) || $availability->kind !== 'exclusion') && + (!isset($availability->id) || $availability->id !== $originId)) { + $filteredList->addEntity($availability); + } + } + return $filteredList; + }Then use it like:
- $filteredAvailabilityList = new AvailabilityList(); - foreach ($availabilityList as $availability) { - if ((!isset($availability->kind) || $availability->kind !== 'exclusion') && - (!isset($availability->id) || $availability->id !== $originId)) { - $filteredAvailabilityList->addEntity($availability); - } - } + $filteredAvailabilityList = $this->filterAvailabilityList($availabilityList, $originId);
76-81: Consider caching date range calculations.The date range calculation and subsequent conflict check could benefit from caching if these values are used multiple times.
Consider storing the date range as class properties:
+ /** @var \DateTimeImmutable */ + protected $earliestStartDateTime; + + /** @var \DateTimeImmutable */ + protected $latestEndDateTime;
82-89: Extract conflict ID collection logic.The conflict ID collection logic could be moved to a dedicated method for better reusability.
Consider this refactor:
+ /** + * Extract unique conflict IDs from the conflict list + * + * @param ProcessList $conflictList + * @return array + */ + protected function extractConflictIds(ProcessList $conflictList): array + { + $conflictedList = []; + foreach ($conflictList as $conflict) { + $availabilityId = ($conflict->getFirstAppointment()->getAvailability()->getId()) ? + $conflict->getFirstAppointment()->getAvailability()->getId() : + $conflict->getFirstAppointment()->getAvailability()->tempId; + if (!in_array($availabilityId, $conflictedList)) { + $conflictedList[] = $availabilityId; + } + } + return $conflictedList; + }zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (4)
1959-2037: Consider parameterized tests for different weekday combinations.The weekday conflict test could be expanded to cover more combinations using data providers.
Consider using a data provider:
+ /** + * @return array + */ + public function weekdayProvider(): array + { + return [ + 'different_days' => [ + ['monday' => '2'], + ['tuesday' => '4'], + false + ], + 'same_days' => [ + ['monday' => '2'], + ['monday' => '2'], + true + ], + // Add more combinations + ]; + } + + /** + * @dataProvider weekdayProvider + */ + public function testWeekdayConflicts(array $weekday1, array $weekday2, bool $expectConflict)
2039-2117: Consider parameterized tests for different availability types.Similar to weekday tests, type conflict testing could be parameterized.
Consider using a data provider:
+ /** + * @return array + */ + public function typeProvider(): array + { + return [ + 'different_types' => [ + 'appointment', + 'openinghours', + false + ], + 'same_types' => [ + 'appointment', + 'appointment', + true + ], + // Add more combinations + ]; + } + + /** + * @dataProvider typeProvider + */ + public function testTypeConflicts(string $type1, string $type2, bool $expectConflict)
2119-2138: Add more invalid input test cases.The invalid input test could be expanded to cover more error scenarios.
Consider adding tests for:
- Missing weekday
- Invalid date formats
- Invalid time formats
- Missing type
- Invalid scope data
2140-2244: Consider testing edge cases in multiple conflicts.The multiple conflicts test could be expanded to cover edge cases.
Consider testing:
- Overlapping time ranges
- Adjacent time slots
- Nested time ranges
- Cross-day conflicts
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(3 hunks)zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php(6 hunks)zmsentities/src/Zmsentities/Availability.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsentities/src/Zmsentities/Availability.php
🧰 Additional context used
📓 Learnings (1)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-01-31T13:14:15.144Z
Learning: The `hasNewVsNewConflicts` method in `AvailabilityList` class should use `$this` to access the collection instead of accepting a collection parameter since it's already in the collection class.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
🔇 Additional comments (3)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (2)
34-40: LGTM! Input validation is thorough and provides clear error messages.The input validation checks are comprehensive, covering all required fields with specific error messages.
54-58: Consider error handling for future availability retrieval.The code fetches future availabilities but doesn't handle potential errors from the API call.
Run this script to check for error handling in similar API calls:
zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (1)
14-26: LGTM! Test setup is well structured.The API call mocking is properly configured with clear parameters and fixture data.
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink The test is failing because each overlap is being counted twice - once from each availability's perspective. For example:
We have two options:
Which approach would you prefer? I can show the code changes for either option. ✏️ Learnings added |
@coderabbitai 1. Update the test to expect 6 conflicts (current behavior) |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the updated test: public function testPartialOverlaps()
{
$entity1 = new Availability([
'id' => '1',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '08:00:00',
'endTime' => '12:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test overlap at start
$entity2 = new Availability([
'id' => '2',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '07:00:00',
'endTime' => '09:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test overlap at end
$entity3 = new Availability([
'id' => '3',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '11:00:00',
'endTime' => '13:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Test completely contained
$entity4 = new Availability([
'id' => '4',
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'startTime' => '09:00:00',
'endTime' => '11:00:00',
'type' => 'appointment',
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$entity1, $entity2, $entity3, $entity4]);
$conflicts = $list->checkAllVsExistingConflicts(
new \DateTimeImmutable('2024-01-15 00:00:00'),
new \DateTimeImmutable('2024-01-15 23:59:59')
);
$this->assertEquals(6, $conflicts->count(), "Should detect all overlaps bidirectionally");
}✏️ Learnings added
|
|
@coderabbitai what other tests can we add to cover every method in AvailabilityList |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are the additional test cases needed for full coverage: public function testGetMaxWorkstationCount()
{
$list = new AvailabilityList([
new Availability([
'workstationCount' => ['intern' => 3, 'public' => 2],
'scope' => ['id' => '141', 'dayoff' => []]
]),
new Availability([
'workstationCount' => ['intern' => 5, 'public' => 4],
'scope' => ['id' => '141', 'dayoff' => []]
]),
new Availability([
'workstationCount' => ['intern' => 2, 'public' => 1],
'scope' => ['id' => '141', 'dayoff' => []]
])
]);
$this->assertEquals(5, $list->getMaxWorkstationCount(), "Should return highest intern workstation count");
}
public function testWithOutDoubles()
{
$availability1 = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'weekday' => ['monday' => '2'],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$availability2 = new Availability([
'id' => '2',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'weekday' => ['monday' => '2'],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability1, $availability1, $availability2]);
$uniqueList = $list->withOutDoubles();
$this->assertEquals(2, $uniqueList->count(), "Should remove duplicate availabilities");
}
public function testWithDateTimeInRange()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'weekday' => ['monday' => '2', 'wednesday' => '4'],
'startDate' => strtotime('2024-01-01'),
'endDate' => strtotime('2024-01-31'),
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability]);
$rangeList = $list->withDateTimeInRange(
new \DateTimeImmutable('2024-01-01'),
new \DateTimeImmutable('2024-01-07')
);
$this->assertEquals(2, $rangeList->count(), "Should include availabilities for Monday and Wednesday");
}
public function testGetSlotListByType()
{
$appointment = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'scope' => ['id' => '141', 'dayoff' => []]
]);
$openinghours = new Availability([
'id' => '2',
'type' => 'openinghours',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$appointment, $openinghours]);
$appointmentSlots = $list->getSlotListByType('appointment');
$this->assertEquals(6, $appointmentSlots->count(), "Should return slots only for appointment type");
}
public function testGetSummerizedSlotCount()
{
$availability1 = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'scope' => ['id' => '141', 'dayoff' => []]
]);
$availability2 = new Availability([
'id' => '2',
'type' => 'appointment',
'startTime' => '10:00:00',
'endTime' => '11:00:00',
'slotTimeInMinutes' => 10,
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability1, $availability2]);
$slotCounts = $list->getSummerizedSlotCount();
$this->assertEquals(6, $slotCounts['1'], "First availability should have 6 slots");
$this->assertEquals(6, $slotCounts['2'], "Second availability should have 6 slots");
}
public function testGetCalculatedSlotCount()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'scope' => ['id' => '141', 'dayoff' => []]
]);
$process = new \BO\Zmsentities\Process();
$appointment = new \BO\Zmsentities\Appointment();
$appointment->availability = $availability;
$process->addAppointment($appointment);
$processList = new \BO\Zmsentities\Collection\ProcessList([$process]);
$list = new AvailabilityList([$availability]);
$slotCounts = $list->getCalculatedSlotCount($processList);
$this->assertEquals(1, $slotCounts['1'], "Should count one busy slot");
}
public function testWithLessData()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'description' => 'Test description',
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability]);
$reducedList = $list->withLessData(['id', 'type']);
$reducedAvailability = $reducedList->getFirst();
$this->assertEquals('1', $reducedAvailability->id, "Should keep id");
$this->assertEquals('appointment', $reducedAvailability->type, "Should keep type");
$this->assertNull($reducedAvailability->description, "Should remove description");
}These tests cover:
Would you like me to create an issue to track adding these tests? ✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 18247 |
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are the corrected test cases: public function testWithLessData()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 3, 'public' => 2],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability]);
$reducedList = $list->withLessData(['id', 'type']);
$reducedAvailability = $reducedList->getFirst();
$this->assertEquals('1', $reducedAvailability->id, "Should keep id");
$this->assertEquals('appointment', $reducedAvailability->type, "Should keep type");
$this->assertArrayNotHasKey('workstationCount', $reducedAvailability, "Should remove workstationCount");
}
public function testGetSummerizedSlotCount()
{
$availability1 = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 1, 'public' => 1],
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'scope' => ['id' => '141', 'dayoff' => []]
]);
$availability2 = new Availability([
'id' => '2',
'type' => 'appointment',
'startTime' => '10:00:00',
'endTime' => '11:00:00',
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 1, 'public' => 1],
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability1, $availability2]);
$slotCounts = $list->getSummerizedSlotCount();
$this->assertEquals(6, $slotCounts['1'], "First availability should have 6 slots");
$this->assertEquals(6, $slotCounts['2'], "Second availability should have 6 slots");
}
public function testGetCalculatedSlotCount()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 1, 'public' => 1],
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'scope' => ['id' => '141', 'dayoff' => []]
]);
$process = new \BO\Zmsentities\Process();
$appointment = new \BO\Zmsentities\Appointment();
$appointment->availability = $availability;
$appointment->date = strtotime('2024-01-15 09:00:00');
$process->addAppointment($appointment);
$processList = new \BO\Zmsentities\Collection\ProcessList([$process]);
$list = new AvailabilityList([$availability]);
$slotCounts = $list->getCalculatedSlotCount($processList);
$this->assertEquals(1, $slotCounts['1'], "Should count one busy slot");
}
public function testWithDateTimeInRange()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'weekday' => ['monday' => '2', 'wednesday' => '4'],
'startDate' => strtotime('2024-01-01'),
'endDate' => strtotime('2024-01-31'),
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 1, 'public' => 1],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability]);
$rangeList = $list->withDateTimeInRange(
new \DateTimeImmutable('2024-01-01'),
new \DateTimeImmutable('2024-01-07')
);
$this->assertEquals(2, $rangeList->count(), "Should include availabilities for Monday and Wednesday");
}Key changes:
Would you like me to create an issue to track these test improvements? ✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 53614 |
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are the corrected test cases: public function testWithLessData()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 3, 'public' => 2],
'repeat' => ['afterWeeks' => 1],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability]);
$reducedList = $list->withLessData(['id', 'type']);
$reducedAvailability = $reducedList->getFirst();
$this->assertEquals('1', $reducedAvailability->id, "Should keep id");
$this->assertEquals('appointment', $reducedAvailability->type, "Should keep type");
$this->assertArrayNotHasKey('workstationCount', $reducedAvailability->getArrayCopy(), "Should remove workstationCount");
}
public function testGetCalculatedSlotCount()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '10:00:00',
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 1, 'public' => 1],
'weekday' => ['monday' => '2'],
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'repeat' => ['afterWeeks' => 1],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$process = new \BO\Zmsentities\Process();
$appointment = new \BO\Zmsentities\Appointment();
$appointment->availability = clone $availability;
$appointment->date = strtotime('2024-01-15');
$appointment->time = '09:00:00';
$process->addAppointment($appointment);
$processList = new \BO\Zmsentities\Collection\ProcessList([$process]);
$list = new AvailabilityList([$availability]);
$slotCounts = $list->getCalculatedSlotCount($processList);
$this->assertEquals(1, $slotCounts['1'], "Should count one busy slot");
}
public function testWithDateTimeInRange()
{
$availability = new Availability([
'id' => '1',
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'weekday' => ['monday' => '2', 'wednesday' => '4'],
'startDate' => strtotime('2024-01-01'),
'endDate' => strtotime('2024-01-31'),
'slotTimeInMinutes' => 10,
'workstationCount' => ['intern' => 1, 'public' => 1],
'repeat' => ['afterWeeks' => 1],
'scope' => ['id' => '141', 'dayoff' => []]
]);
$list = new AvailabilityList([$availability]);
$rangeList = $list->withDateTimeInRange(
new \DateTimeImmutable('2024-01-01'),
new \DateTimeImmutable('2024-01-07')
);
$this->assertEquals(2, $rangeList->count(), "Should include availabilities for Monday and Wednesday");
}Key changes:
Would you like me to create an issue to track these test improvements? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 2977 ✏️ Learnings added
|
|
@coderabbitai series jede Woche don't seem to recognize conflicting einmaliger Termin lying in the future. only after saving. why is that? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
zmsadmin/js/page/availabilityDay/form/datepicker.js (2)
127-136: Consider optimizing maintenance window time generation.The current implementation generates individual Date objects for each minute in the maintenance window, which is memory-intensive and could be optimized.
Consider using a more efficient approach:
- // Add maintenance window times (22:00-01:00) - const selectedDate = moment(this.state.selectedDate); - for (let minute = 1; minute < 60; minute++) { - times.push(selectedDate.clone().hour(22).minute(minute).toDate()); - } - for (let minute = 0; minute < 59; minute++) { - times.push(selectedDate.clone().hour(0).minute(minute).toDate()); - } + // Add maintenance window times (22:00-01:00) + const selectedDate = moment(this.state.selectedDate); + const slotTime = this.state.availability.slotTimeInMinutes; + + // Generate times for 22:00-23:59 + for (let minute = 0; minute < 60; minute += slotTime) { + times.push(selectedDate.clone().hour(22).minute(minute).toDate()); + } + + // Generate times for 00:00-00:59 + for (let minute = 0; minute < 60; minute += slotTime) { + times.push(selectedDate.clone().hour(0).minute(minute).toDate()); + }
298-311: Consider making the buffer time configurable.The 60-minute buffer is a good default, but different services might need different buffer times.
Consider making it configurable:
- const BUFFER_MINUTES = 60; // Minimum minutes needed between current time and next slot + const BUFFER_MINUTES = this.props.attributes.bufferTime || 60; // Configurable buffer time with defaultThen update the PropTypes to include the new attribute:
AvailabilityDatePicker.propTypes = { onChange: PropTypes.func, attributes: PropTypes.shape({ bufferTime: PropTypes.number, // Minutes of buffer time for same-day appointments // ... other existing attributes }) }zmsentities/tests/Zmsentities/AvailabilityTest.php (4)
Line range hint
870-956: Improve test readability and maintainability.The conflict test method is complex and could benefit from the following improvements:
- Extract the conflict message strings into constants to avoid duplication
- Consider breaking down the test into smaller, focused test methods
- Add descriptive comments for each test scenario
Example refactor for the conflict messages:
+ private const OVERLAP_CONFLICT_MESSAGE = "Konflikt: Zwei Öffnungszeiten überschneiden sich.\n" . + "Bestehende Öffnungszeit:  [%s]\n" . + "Neue Öffnungszeit:                 [%s]"; + private const EQUAL_CONFLICT_MESSAGE = "Konflikt: Zwei Öffnungszeiten sind gleich.\n" . + "Bestehende Öffnungszeit:  [%s]\n" . + "Neue Öffnungszeit:                 [%s]";
959-1109: Well-structured overlap test cases!The new test methods thoroughly cover various overlap scenarios including:
- Partial overlaps (start/end)
- Edge cases (back-to-back times, midnight crossing)
- Weekday-specific overlaps
The test cases are well-organized and provide good coverage.
Consider adding data providers to make the test cases more maintainable:
+ /** + * @dataProvider provideOverlapTestCases + */ + public function testPartialOverlaps(array $entity1Data, array $entity2Data, int $expectedConflicts) + { + $entity1 = new Availability($entity1Data); + $entity2 = new Availability($entity2Data); + // ... rest of the test + } + + public function provideOverlapTestCases(): array + { + return [ + 'overlap_at_start' => [/* ... */], + 'overlap_at_end' => [/* ... */], + 'completely_contained' => [/* ... */], + ]; + }
1150-1428: Add more edge cases to validation tests.The validation test methods provide good coverage but could benefit from additional edge cases:
In
testValidateStartTime:
- Test with millisecond precision
- Test with timezone edge cases
In
testValidateSlotTime:
- Test with maximum allowed slot time
- Test with negative slot times
In
testValidateBookableDayRange:
- Test with maximum allowed range
- Test with zero values
Example additional test cases:
// In testValidateStartTime $startDate = new \DateTimeImmutable('2024-01-15 12:00:00.500'); $errors = $entity->validateStartTime($today, $tomorrow, $startDate, $endDate, $selectedDate, 'current'); $this->assertCount(1, $errors, "Should reject millisecond precision"); // In testValidateSlotTime $entity['slotTimeInMinutes'] = -30; $errors = $entity->validateSlotTime($startDate, $endDate); $this->assertCount(1, $errors, "Should reject negative slot times");
Line range hint
1-1429: Consider reorganizing test classes for better maintainability.Given the growing number of test methods and different testing concerns, consider splitting this test class into more focused test classes:
AvailabilityValidationTest- For all validation-related testsAvailabilityConflictTest- For overlap and conflict-related testsAvailabilityCalculationTest- For slot and time calculation testsThis would improve maintainability and make the test suite easier to navigate.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsadmin/js/page/availabilityDay/form/datepicker.js(4 hunks)zmsentities/tests/Zmsentities/AvailabilityTest.php(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
🔇 Additional comments (3)
zmsadmin/js/page/availabilityDay/form/datepicker.js (2)
25-25: LGTM! Appropriate time limit for scheduling.The maxTime adjustment to 22:59 aligns well with the maintenance window requirements, preventing appointments from being scheduled during maintenance hours (22:00-01:00).
138-191: LGTM! Well-structured availability management.The implementation effectively:
- Filters and sorts availabilities by type and weekday
- Handles time slots with proper consideration of appointment duration
- Manages boundary cases between adjacent availabilities
zmsentities/tests/Zmsentities/AvailabilityTest.php (1)
182-182: Verify the updated assertion values.Several test assertions have been updated with new expected values. Please ensure these changes align with the current implementation and business requirements:
- Line 182: Changed expected available seconds per day
- Line 199: Changed expected available seconds on date time
- Line 501: Changed expected workstation count
- Lines 510-512: Changed expected slot count and times
- Lines 544,553: Changed expected collection counts
Also applies to: 199-199, 501-501, 510-510, 512-512, 544-544, 553-553
✅ Verification successful
Assertion values are consistent with implementation ✓
The updated assertion values in the test file are verified to be consistent with the implementation:
- The value of
6534000seconds matches the calculation ingetAvailableSecondsPerDay- The workstation count of
99is used consistently throughout the codebase- Other slot-related assertions align with the slot calculation logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changes in assertion values by checking the implementation # Check for related changes in the Availability class implementation rg -A 5 "getAvailableSecondsPerDay|getAvailableSecondsOnDateTime|withCalculatedSlots|getSlotList" --type phpLength of output: 14074
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink The issue is in how conflicts are checked differently before and after saving:
To fix this, hasNewVsNewConflicts should use isOpenedOnDate instead of just hasSharedWeekdayWith. Would you like me to create an issue to track this fix? ✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 8051 Script: Length of output: 8266 |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink The issue is in how conflicts are checked differently before and after saving:
To fix this, we should modify hasNewVsNewConflicts to use isOpenedOnDate instead of just hasSharedWeekdayWith. Would you like me to create an issue to track this fix? ✏️ Learnings added
|
@coderabbitai fix so it checks both before saving checkAllVsExistingConflicts hasNewVsNewConflicts. it should already though in AvailabilityConflicts. It does find a series in the future but not einmalige. |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the corrected implementation: public function hasNewVsNewConflicts(\DateTimeImmutable $selectedDate): \BO\Zmsentities\Collection\ProcessList
{
$conflicts = new \BO\Zmsentities\Collection\ProcessList();
foreach ($this as $availability1) {
foreach ($this 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->isOpenedOnDate($selectedDate) &&
$availability2->isOpenedOnDate($selectedDate)) {
$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) {
$process = new \BO\Zmsentities\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 = $start1->format('H:i') . ' - ' . $end1->format('H:i');
$timeRange2 = $start2->format('H:i') . ' - ' . $end2->format('H:i');
$process->amendment = "Konflikt: Zwei Öffnungszeiten überschneiden sich.\n"
. "Neue Ö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:
(＾▽＾) I've added a learning to help remember this important change. ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Looking at the AvailabilityConflicts implementation, it's already checking both types of conflicts before saving:
$overlapConflicts = $availabilityList->hasNewVsNewConflicts($selectedDateTime);
$conflictList->addList($overlapConflicts);
$existingConflicts = $filteredAvailabilityList->checkAllVsExistingConflicts($earliestStartDateTime, $latestEndDateTime);
$conflictList->addList($existingConflicts);Both sets of conflicts are combined into the same conflictList. The previous fix we made to hasNewVsNewConflicts (using isOpenedOnDate) should make it work correctly with weekly series and one-time appointments. Would you like me to add more test cases to verify this behavior? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 7969 ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink I found the issue. The problem is in the hasSharedWeekdayWith method's repeat pattern comparison: public function hasSharedWeekdayWith(Availability $availability)
{
return ($this->type == $availability->type
&& $this->repeat['afterWeeks'] != $availability->repeat['afterWeeks'] // This is wrong!
&& $this->repeat['weekOfMonth'] != $availability->repeat['weekOfMonth'] // This is wrong!
&& (bool)$this->weekday['monday'] != (bool)$availability->weekday['monday'] // This is wrong!
// ... other weekdays
) ? false : true;
}The issue is that it's using
Here's how it should be: public function hasSharedWeekdayWith(Availability $availability)
{
return $this->type == $availability->type
&& ((bool)$this->weekday['monday'] == (bool)$availability->weekday['monday']
&& (bool)$this->weekday['tuesday'] == (bool)$availability->weekday['tuesday']
&& (bool)$this->weekday['wednesday'] == (bool)$availability->weekday['wednesday']
&& (bool)$this->weekday['thursday'] == (bool)$availability->weekday['thursday']
&& (bool)$this->weekday['friday'] == (bool)$availability->weekday['friday']
&& (bool)$this->weekday['saturday'] == (bool)$availability->weekday['saturday']
&& (bool)$this->weekday['sunday'] == (bool)$availability->weekday['sunday']);
}Key changes:
Would you like me to create an issue to track this fix? 🤔 Let me know if you'd like me to explain any part in more detail. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 12108 ✏️ Learnings added
🧠 Learnings used |
@coderabbitai actually the opposite is correct and testDifferentWeekdaysNoConflict should have conflict fix the test to expect a conflict |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the corrected test: public function testDifferentWeekdaysConflict()
{
$this->setApiCalls([
[
'function' => 'readGetResult',
'url' => '/scope/141/availability/',
'parameters' => [
'resolveReferences' => 0,
'startDate' => '2016-04-04'
],
'response' => $this->readFixture("GET_availability_68985.json")
]
]);
$response = $this->render([], [
'__body' => '{
"availabilityList": [
{
"id": "81871",
"weekday": {"monday": "2"},
"startDate": 1452553200,
"endDate": 1463868000,
"startTime": "08:00:00",
"endTime": "15:50:00",
"type": "appointment",
"scope": {
"id": "141",
"dayoff": [
{
"id": "302",
"date": 1458860400,
"lastChange": 1566566540,
"name": "Karfreitag"
},
{
"id": "303",
"date": 1459116000,
"lastChange": 1566566540,
"name": "Ostermontag"
}
]
}
},
{
"id": null,
"weekday": {"tuesday": "4"},
"startDate": 1452553200,
"endDate": 1463868000,
"startTime": "08:00:00",
"endTime": "15:50:00",
"type": "appointment",
"scope": {
"id": "141",
"dayoff": [
{
"id": "302",
"date": 1458860400,
"lastChange": 1566566540,
"name": "Karfreitag"
},
{
"id": "303",
"date": 1459116000,
"lastChange": 1566566540,
"name": "Ostermontag"
}
]
},
"tempId": "__temp__0"
}
],
"selectedDate": "2016-04-04"
}'
], [], 'POST');
$this->assertStringContainsString('Konflikt', (string)$response->getBody());
$this->assertStringContainsString('"conflictIdList":["81871","__temp__0"]', (string)$response->getBody());
$this->assertEquals(200, $response->getStatusCode());
}Key changes:
✏️ Learnings added
|
|
@coderabbitai what tests are still missing in \BO\Zmsentities\Availability\AvailabilityTest? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are the additional test cases needed for full coverage: public function testGetDefaults()
{
$entity = new Availability();
$defaults = $entity->getDefaults();
$this->assertEquals(0, $defaults['id']);
$this->assertEquals(1, $defaults['repeat']['afterWeeks']);
$this->assertEquals(0, $defaults['repeat']['weekOfMonth']);
$this->assertEquals(1, $defaults['bookable']['startInDays']);
$this->assertEquals(60, $defaults['bookable']['endInDays']);
$this->assertEquals(10, $defaults['slotTimeInMinutes']);
$this->assertEquals('appointment', $defaults['type']);
// Check all weekdays are initialized to 0
foreach (['sunday', 'monday', 'tuesday', 'wednesday', 'thursday', 'friday', 'saturday'] as $day) {
$this->assertEquals(0, $defaults['weekday'][$day]);
}
}
public function testIsMatchOf()
{
$entity1 = new Availability([
'type' => 'appointment',
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15'),
'weekday' => ['monday' => '2'],
'repeat' => ['afterWeeks' => 1, 'weekOfMonth' => 0],
'scope' => ['id' => '141', 'dayoff' => []]
]);
// Exact match
$entity2 = clone $entity1;
$this->assertTrue($entity1->isMatchOf($entity2));
// Different type
$entity2['type'] = 'openinghours';
$this->assertFalse($entity1->isMatchOf($entity2));
// Different time
$entity2 = clone $entity1;
$entity2['startTime'] = '10:00:00';
$this->assertFalse($entity1->isMatchOf($entity2));
// Different weekday
$entity2 = clone $entity1;
$entity2['weekday']['tuesday'] = '2';
$entity2['weekday']['monday'] = '0';
$this->assertFalse($entity1->isMatchOf($entity2));
}
public function testCacheClearing()
{
$entity = new Availability([
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15')
]);
// Access times to populate cache
$startTime1 = $entity->getStartDateTime();
$endTime1 = $entity->getEndDateTime();
// Modify times
$entity['startTime'] = '10:00:00';
$entity['endTime'] = '18:00:00';
// Get new times
$startTime2 = $entity->getStartDateTime();
$endTime2 = $entity->getEndDateTime();
// Verify cache was cleared
$this->assertNotEquals($startTime1->format('H:i'), $startTime2->format('H:i'));
$this->assertNotEquals($endTime1->format('H:i'), $endTime2->format('H:i'));
}
public function testHasTimeEdgeCases()
{
$entity = new Availability([
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15')
]);
// Exactly at start time
$this->assertTrue($entity->hasTime(new \DateTimeImmutable('2024-01-15 09:00:00')));
// Just before start time
$this->assertFalse($entity->hasTime(new \DateTimeImmutable('2024-01-15 08:59:59')));
// Just before end time
$this->assertTrue($entity->hasTime(new \DateTimeImmutable('2024-01-15 16:59:59')));
// Exactly at end time
$this->assertFalse($entity->hasTime(new \DateTimeImmutable('2024-01-15 17:00:00')));
}
public function testGetAvailableSecondsPerDayWithTypes()
{
$entity = new Availability([
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'workstationCount' => [
'intern' => 3,
'public' => 2,
'callcenter' => 1
]
]);
// 8 hours = 28800 seconds
$this->assertEquals(86400, $entity->getAvailableSecondsPerDay('intern')); // 28800 * 3
$this->assertEquals(57600, $entity->getAvailableSecondsPerDay('public')); // 28800 * 2
$this->assertEquals(28800, $entity->getAvailableSecondsPerDay('callcenter')); // 28800 * 1
}
public function testBookableTimeErrors()
{
$entity = new Availability([
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-15')
]);
// Missing bookable start days
$this->expectException(\BO\Zmsentities\Exception\ProcessBookableFailed::class);
$this->expectExceptionMessage('Undefined start time for booking, try to set the scope properly');
$entity->getBookableStart(new \DateTimeImmutable());
// Missing bookable end days
$this->expectException(\BO\Zmsentities\Exception\ProcessBookableFailed::class);
$this->expectExceptionMessage('Undefined end time for booking, try to set the scope properly');
$entity->getBookableEnd(new \DateTimeImmutable());
}
public function testIsBookableScenarios()
{
$now = new \DateTimeImmutable('2024-01-15 12:00:00');
$entity = new Availability([
'startTime' => '09:00:00',
'endTime' => '17:00:00',
'startDate' => strtotime('2024-01-15'),
'endDate' => strtotime('2024-01-17'),
'bookable' => [
'startInDays' => 0,
'endInDays' => 2
]
]);
// Current day within hours
$this->assertTrue($entity->isBookable($now, $now));
// Current day before hours
$this->assertFalse($entity->isBookable($now->modify('08:00'), $now));
// Current day after hours
$this->assertFalse($entity->isBookable($now->modify('18:00'), $now));
// Next day
$this->assertTrue($entity->isBookable($now->modify('+1 day 12:00'), $now));
// Last bookable day
$this->assertTrue($entity->isBookable($now->modify('+2 days 12:00'), $now));
// Beyond bookable range
$this->assertFalse($entity->isBookable($now->modify('+3 days 12:00'), $now));
}These tests cover:
Would you like me to add more test cases or modify the existing ones? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 96464 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (3)
Line range hint
13-1957: Add test description and improve assertions.The test method would benefit from:
- A PHPDoc comment explaining the test scenario
- More specific assertions about the conflict detection logic
- Constants for repeated test data like dates and IDs
Apply this diff to improve the test:
+ /** + * @test + * @group availability + * @description Verifies that overlapping availabilities of the same type are detected as conflicts + */ public function testRendering() { + const TEST_SCOPE_ID = '141'; + const TEST_START_DATE = '2016-04-04'; + const AVAILABILITY_ID = '81871'; + const TEMP_ID = '__temp__0'; + $this->setApiCalls( [ [ 'function' => 'readGetResult', - 'url' => '/scope/141/availability/', + 'url' => '/scope/' . self::TEST_SCOPE_ID . '/availability/', 'parameters' => [ 'resolveReferences' => 0, - 'startDate' => '2016-04-04' + 'startDate' => self::TEST_START_DATE ], 'response' => $this->readFixture("GET_availability_68985.json") ] ] ); // ... rest of the test + // Assert specific conflict detection + $responseBody = (string) $response->getBody(); + $this->assertStringContainsString( + '"conflictIdList":["' . self::AVAILABILITY_ID . '","' . self::TEMP_ID . '"]', + $responseBody, + 'Expected conflict between original and temporary availability not detected' + );
2119-2138: Enhance error handling test coverage.The test only verifies one invalid scenario (missing scope). Consider adding more test cases for comprehensive error handling coverage.
Add data provider to test multiple invalid scenarios:
/** * @return array */ public function invalidInputProvider() { return [ 'missing_scope' => [ 'input' => ['id' => '81871', 'weekday' => ['monday' => '2'], 'type' => 'appointment'], 'expectedMessage' => 'Missing required scope' ], 'invalid_weekday' => [ 'input' => ['id' => '81871', 'weekday' => ['invalid' => '2'], 'type' => 'appointment', 'scope' => ['id' => '141']], 'expectedMessage' => 'Invalid weekday' ], 'invalid_time_format' => [ 'input' => ['id' => '81871', 'weekday' => ['monday' => '2'], 'startTime' => 'invalid', 'type' => 'appointment', 'scope' => ['id' => '141']], 'expectedMessage' => 'Invalid time format' ] ]; } /** * @dataProvider invalidInputProvider */ public function testInvalidInput($input, $expectedMessage) { $this->expectException(\BO\Zmsadmin\Exception\BadRequest::class); $this->expectExceptionMessage($expectedMessage); $this->render([], ['__body' => json_encode(['availabilityList' => [$input]])], [], 'POST'); }
2140-2245: Add more specific assertions for multiple conflicts.While the test verifies that multiple conflicts are detected, it could benefit from more specific assertions about the nature of each conflict.
Add assertions to verify specific conflict details:
public function testMultipleConflicts() { // ... test setup ... $responseBody = (string) $response->getBody(); $this->assertStringContainsString('Konflikt', $responseBody); $this->assertStringContainsString('"conflictIdList":["81871","__temp__0","__temp__1"]', $responseBody); + + // Verify specific conflict details + $this->assertStringContainsString('08:00:00-15:50:00', $responseBody, 'First conflict time range not found'); + $this->assertStringContainsString('09:00:00-16:50:00', $responseBody, 'Second conflict time range not found'); + $this->assertStringContainsString('monday', $responseBody, 'Conflict weekday not found'); $this->assertEquals(200, $response->getStatusCode()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php(6 hunks)zmsentities/src/Zmsentities/Availability.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsentities/src/Zmsentities/Availability.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
🔇 Additional comments (2)
zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (2)
2039-2117: LGTM! Well-structured test for type-based conflict exclusion.The test correctly verifies that availabilities of different types (appointment vs. openinghours) can share the same time slot without causing conflicts.
1959-2037: Verify the business logic for weekday conflict detection.The test expects availabilities on different weekdays (Monday and Tuesday) to be marked as conflicts. This seems counterintuitive as appointments on different days should not overlap.
Please verify if this is the intended behavior. If not, consider updating the test to expect no conflicts:
public function testDifferentWeekdaysConflict() { // ... test setup ... - $this->assertStringContainsString('Konflikt', (string)$response->getBody()); - $this->assertStringContainsString('"conflictIdList":["81871","__temp__0"]', (string)$response->getBody()); + $this->assertStringNotContainsString('Konflikt', (string)$response->getBody()); + $this->assertStringContainsString('"conflictIdList":[]', (string)$response->getBody()); $this->assertEquals(200, $response->getStatusCode()); }
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
New Features
Bug Fixes
Improvements
Performance
These changes focus on making the availability management system more reliable, user-friendly, and precise in handling time-based scheduling.