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 #726
Conversation
…orm error messaging
This reverts commit ce5d795.
…the-same-appointment-type-must-not-overlap
…ation-opening-hours-of-the-same-appointment-type-must-not-overlap
…the-same-appointment-type-must-not-overlap
WalkthroughThe changes in this pull request encompass various modifications across multiple JavaScript and PHP files, primarily focusing on improving data handling, validation, and rendering logic related to availability management. Key updates include enhancements to conflict handling, error rendering, and form input management in the JavaScript components, while the PHP classes have seen improvements in input validation and error handling for availability-related operations. New templates for error handling have also been introduced, along with updates to existing tests to ensure comprehensive coverage of the new functionalities. Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (23)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (3)
9-9: Remove Unused Use StatementThe import statement
use BO\Zmsapi\Exception\BadRequest as BadRequestException;might be redundant ifBadRequestExceptionis not used elsewhere in the code. Please verify and remove it if it's unnecessary to keep the code clean.
94-100: Add Return Type Declarations in DocblockIn the docblock for
getAvailabilityList, consider specifying the return type using PHP 7+ syntax for better clarity and type safety./** - * @return AvailabilityList + * @return \BO\Zmsentities\Collection\AvailabilityList */ protected static function getAvailabilityList($scope, $dateTime)
Line range hint
109-121: Handle Exceptions More SpecificallyIn the
try-catchblock, the code catches a generic\BO\Zmsclient\Exception, but only checks forAvailabilityNotFound. If other exceptions occur, they are rethrown without additional handling. Consider handling other potential exceptions or logging them appropriately to aid in debugging.zmsapi/tests/Zmsapi/AvailabilityAddTest.php (2)
16-43: Use Data Providers for Test CasesThe
testRenderingmethod could be enhanced by using data providers to supply multiple sets of input data, improving test coverage and maintainability.
84-116: Consolidate Duplicate Code in Test MethodsThe test methods
testOverlappingAvailabilityandtestDuplicateOverlappingAvailabilityshare similar structures. Consider refactoring the common code into a separate method or using a data provider to reduce duplication.zmsapi/src/Zmsapi/AvailabilityAdd.php (1)
49-55: Enhance Input Validation MessagesThe validation checks for
availabilityList,scope, andselectedDatethrow genericBadRequestExceptions with messages. To improve client understanding, consider providing more detailed messages or using specific exception subclasses.zmsapi/src/Zmsapi/AvailabilityUpdate.php (2)
4-4: Fix Incomplete DocblockThe docblock on line 4 seems incomplete ("@copyright BerlinOnline Stadtportal GmbH & Co."). Ensure that the company name is fully specified.
Apply this change:
/** * @package ZMS API - * @copyright BerlinOnline Stadtportal GmbH & Co. + * @copyright BerlinOnline Stadtportal GmbH & Co. KG **/
49-55: Enhance Input Validation ConsistencyThe input validation logic is similar to that in
AvailabilityAdd.php. Consider refactoring this validation into a shared method or trait to maintain consistency and reduce code duplication.zmsadmin/js/page/availabilityDay/form/validate.js (1)
29-30: Simplify boolean assignment ofvalidYou can simplify the assignment of
validby directly using the boolean expression without the ternary operator.Apply this diff to simplify the code:
-let valid = (0 < errorList.itemList.length) ? false : true; +let valid = errorList.itemList.length === 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (1)
83-177: Enhance exception assertions in test methodsConsider adding assertions to check the exception message or code to ensure the tests verify the specific failure reasons. This will make your tests more robust and precise.
Example:
$this->expectException(AvailabilityUpdateFailed::class); +$this->expectExceptionMessage('Specific error message related to the failure');zmsadmin/js/page/availabilityDay/index.js (1)
117-118: Simplify null check with optional chainingYou can simplify the check
availability.tempId && availability.tempId.includes('__temp__')using optional chaining for better readability.Apply this diff to simplify the code:
return ( (availability.__modified || - (availability.tempId && availability.tempId.includes('__temp__'))) && + (availability.tempId?.includes('__temp__'))) && !this.hasErrors(availability) );🧰 Tools
🪛 Biome (1.9.4)
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
zmsentities/src/Zmsentities/Availability.php (4)
115-115: Ensure type hint consistency for parameter$type.The docblock for the
$typeparameter in theisOpenedOnDatemethod specifies the type asstring. Ensure that the method signature also includes the type hintstring $type = falsefor consistency and to enhance type safety.
139-139: Ensure type hint consistency for parameter$type.Similarly, in the
isOpenedmethod, verify that the method signature includes the type hintstring $type = falseto match the docblock and improve type safety.
453-483: Validate input parameters thoroughly invalidateStartTimemethod.The
validateStartTimemethod effectively checks for future dates, midnight start times, and adds appropriate error messages. Ensure that all potential edge cases are covered, and consider adding unit tests to verify the validation logic.
484-510: Review conditions for potential redundancy invalidateEndTimemethod.The conditions at lines 497~ and 502~ both aim to ensure that the end time is after the start time. Verify if both checks are necessary or if they can be consolidated to simplify the logic.
zmsadmin/js/page/availabilityDay/form/errors.js (1)
Line range hint
22-26: Consider enhancing error message accessibility.While the error message already uses
role="alert", consider adding more descriptive ARIA labels for screen readers.- <div className="message message--error" role="alert" aria-live="polite"> + <div + className="message message--error" + role="alert" + aria-live="polite" + aria-label="Formular Fehlermeldungen" + >zmsadmin/js/page/availabilityDay/form/footerButtons.js (1)
7-11: LGTM: Enhanced button state management.The updated disable conditions properly prevent actions when conflicts exist, aligning with the PR's goal of improving validation.
Consider moving inline styles to CSS.
For better maintainability, consider moving the inline styles to a CSS class.
- <div className="form-actions" style={{ "marginTop": "0", "padding": "0.75em" }}> + <div className="form-actions form-actions--compact">Add to your CSS:
.form-actions--compact { margin-top: 0; padding: 0.75em; }zmsadmin/js/page/availabilityDay/saveBar.js (1)
13-19: Consider optimizing useEffect dependenciesThe empty dependency array suggests this effect should only run once on mount, but it actually depends on the component's lifecycle. Consider adding
isVisibleto the dependency array to make the dependencies explicit.- }, []) + }, [isVisible])zmsadmin/js/page/availabilityDay/form/conflicts.js (1)
11-27: Consider using functional programming patternsThe nested map operations with side effects could be refactored to be more functional and readable.
- let conflictDatesByMessage = []; - conflictList.map(collection => { - collection.conflicts.map((conflict) => { - const existingConflict = conflictDatesByMessage.find( - item => item.message === conflict.message - ); - - if (existingConflict) { - existingConflict.dates.push(formatDate(collection.date)); - } else { - conflictDatesByMessage.push({ - message: conflict.message, - dates: [formatDate(collection.date)] - }); - } - }); - }); + const conflictDatesByMessage = conflictList.reduce((acc, collection) => { + return collection.conflicts.reduce((innerAcc, conflict) => { + const existingIndex = innerAcc.findIndex(item => item.message === conflict.message); + if (existingIndex >= 0) { + return [ + ...innerAcc.slice(0, existingIndex), + { + ...innerAcc[existingIndex], + dates: [...innerAcc[existingIndex].dates, formatDate(collection.date)] + }, + ...innerAcc.slice(existingIndex + 1) + ]; + } + return [...innerAcc, { message: conflict.message, dates: [formatDate(collection.date)] }]; + }, acc); + }, []);zmsadmin/js/page/availabilityDay/form/index.js (2)
6-6: Consider using absolute imports for better maintainabilityThe relative import path
../form/validatecould become problematic if the file structure changes. Consider using absolute imports.-import { hasSlotCountError } from '../form/validate'; +import { hasSlotCountError } from '@/page/availabilityDay/form/validate';
68-68: Add error handling for hasSlotCountError validationThe validation function is called directly with props without any error handling. Consider wrapping it in a try-catch block to handle potential validation errors gracefully.
-hasSlotCountError={hasSlotCountError(this.props)} +hasSlotCountError={(() => { + try { + return hasSlotCountError(this.props); + } catch (error) { + console.error('Slot count validation failed:', error); + return true; // Fail safe: disable buttons on validation error + } +})()}zmsentities/schema/availability.json (1)
122-125: Improve documentation for thekindproperty.The description "format" is too vague. Consider enhancing the documentation to specify:
- The purpose of this property
- Expected values/formats
- Example usage
"kind": { "type": "string", - "description": "format" + "description": "Specifies the format or type of the availability entry. Example values: [TODO: list expected values]" },zmsdb/tests/Zmsdb/ProcessConflictTest.php (1)
103-105: Consider using constants for error messages.The hardcoded error messages with HTML entities could be moved to constants or a translation file for better maintainability.
+ private const CONFLICT_EQUAL_MESSAGE = "Konflikt: Zwei Öffnungszeiten sind gleich.\n" . + "Bestehende Öffnungszeit:  [%s]\n" . + "Neue Öffnungszeit:                 [%s]"; public function testEqual() { // ... - $this->assertEquals("Konflikt: Zwei Öffnungszeiten sind gleich.\n" . - "Bestehende Öffnungszeit:  [30.01.2016 - 22.05.2016, 08:00 - 13:50]\n" . - "Neue Öffnungszeit:                 [30.01.2016 - 22.05.2016, 08:00 - 13:50]", + $this->assertEquals(sprintf( + self::CONFLICT_EQUAL_MESSAGE, + "30.01.2016 - 22.05.2016, 08:00 - 13:50", + "30.01.2016 - 22.05.2016, 08:00 - 13:50" + ), $conflictList->getFirst()->getAmendment() );Also applies to: 120-122
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
zmscalldisplay/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
zmsadmin/js/page/availabilityDay/form/conflicts.js(1 hunks)zmsadmin/js/page/availabilityDay/form/content.js(2 hunks)zmsadmin/js/page/availabilityDay/form/errors.js(2 hunks)zmsadmin/js/page/availabilityDay/form/footerButtons.js(1 hunks)zmsadmin/js/page/availabilityDay/form/formButtons.js(2 hunks)zmsadmin/js/page/availabilityDay/form/index.js(2 hunks)zmsadmin/js/page/availabilityDay/form/validate.js(3 hunks)zmsadmin/js/page/availabilityDay/helpers.js(0 hunks)zmsadmin/js/page/availabilityDay/index.js(7 hunks)zmsadmin/js/page/availabilityDay/layouts/page.js(2 hunks)zmsadmin/js/page/availabilityDay/saveBar.js(2 hunks)zmsadmin/js/page/availabilityDay/timetable/graphview.js(0 hunks)zmsadmin/js/page/availabilityDay/timetable/index.js(1 hunks)zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(4 hunks)zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityaddfailed.twig(1 hunks)zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityupdatefailed.twig(1 hunks)zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php(1 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(2 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(3 hunks)zmsapi/src/Zmsapi/Exception/Availability/AvailabilityAddFailed.php(1 hunks)zmsapi/src/Zmsapi/Exception/Availability/AvailabilityUpdateFailed.php(1 hunks)zmsapi/tests/Zmsapi/AvailabilityAddTest.php(2 hunks)zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php(3 hunks)zmsdb/tests/Zmsdb/AvailabilityTest.php(1 hunks)zmsdb/tests/Zmsdb/ProcessConflictTest.php(3 hunks)zmsentities/schema/availability.json(2 hunks)zmsentities/src/Zmsentities/Availability.php(5 hunks)zmsentities/src/Zmsentities/Collection/AvailabilityList.php(2 hunks)zmsentities/tests/Zmsentities/AvailabilityTest.php(7 hunks)zmsentities/tests/Zmsentities/ProcessTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- zmsadmin/js/page/availabilityDay/timetable/graphview.js
- zmsadmin/js/page/availabilityDay/helpers.js
✅ Files skipped from review due to trivial changes (1)
- zmsapi/src/Zmsapi/Exception/Availability/AvailabilityUpdateFailed.php
👮 Files not reviewed due to content moderation or server errors (1)
- zmsentities/tests/Zmsentities/ProcessTest.php
🧰 Additional context used
📓 Learnings (1)
zmsadmin/js/page/availabilityDay/layouts/page.js (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#685
File: zmsadmin/js/page/availabilityDay/layouts/page.js:14-14
Timestamp: 2024-12-04T12:18:56.123Z
Learning: In `zmsadmin/js/page/availabilityDay/layouts/page.js`, it's acceptable to render the `saveBar` component twice, once after `props.timeTable` and once after `props.accordion`, to display the save bar at both the top and bottom of the layout.
🪛 Biome (1.9.4)
zmsadmin/js/page/availabilityDay/form/conflicts.js
[error] 33-33: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
zmsadmin/js/page/availabilityDay/form/validate.js
[error] 29-29: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
zmsadmin/js/page/availabilityDay/index.js
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 203-204: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (28)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (3)
36-41: Ensure Input Validation for 'availabilityList', 'scope', and 'selectedDate'
The validation checks for availabilityList, scope, and selectedDate are crucial. Ensure that these checks cover all edge cases, such as empty arrays or incorrect data types, to prevent potential exceptions further down the code execution.
48-54: Check Date Formatting Consistency
The selectedDateTime is set using modify(\App::$now->format('H:i:s')), which might lead to unexpected results if \App::$now is not properly initialized or in a different timezone. Consider explicitly setting the time or ensuring that \App::$now is consistent throughout the application.
74-77: Validate Availability List Date Range Calculation
When calculating $earliestStartDateTime and $latestEndDateTime using getDateTimeRangeFromList, ensure that the method correctly handles cases where the availability list is empty or contains entries with invalid dates. This prevents potential null references or incorrect conflict assessments.
zmsapi/tests/Zmsapi/AvailabilityAddTest.php (2)
50-82: Check for Consistency in Exception Handling
In testOverlappingAvailability, the test expects an AvailabilityAddFailed exception. Ensure that this exception is appropriately thrown in all cases of overlapping availability within the application logic.
118-139: Validate Time Logic in Tests
In testInvalidEndTime, the test checks for an end time before the start time. Ensure that the application correctly validates time inputs across different time zones and handles cases where daylight saving changes might affect the time logic.
zmsapi/src/Zmsapi/AvailabilityAdd.php (2)
23-24: Ensure AvailabilityAddFailed Exception is Properly Defined
The AvailabilityAddFailed exception is being used. Confirm that this exception class is appropriately defined and includes meaningful messages to aid in debugging when the exception is thrown.
66-68: Validate Date Range Correctness
The startDate and endDate are hardcoded to now and one month ahead. Ensure this aligns with business requirements and consider making these configurable or calculated based on input data.
zmsapi/src/Zmsapi/AvailabilityUpdate.php (2)
22-24: Validate Exception Classes
The newly used exceptions BadRequestException and NotFoundException should be correctly defined and namespaced. Ensure their definitions align with PSR standards and that they provide meaningful error messages.
147-165:
Ensure Correct Handling of Entity Updates
The writeEntityUpdate method should handle cases where $updatedEntity remains null after attempting to update or write the entity. Confirm that the repository methods updateEntity and writeEntity return the expected values or throw exceptions when they fail.
Apply this diff to check for null and handle exceptions:
$updatedEntity = null;
try {
if ($entity->id) {
$oldEntity = $repository->readEntity($entity->id);
if ($oldEntity && $oldEntity->hasId()) {
$this->writeSpontaneousEntity($oldEntity);
$updatedEntity = $repository->updateEntity($entity->id, $entity, $resolveReferences);
} else {
throw new NotFoundException("Entity with ID {$entity->id} not found.");
}
} else {
$updatedEntity = $repository->writeEntity($entity, 2);
}
} catch (\Exception $e) {
throw new AvailabilityUpdateFailed($e->getMessage());
}
if (!$updatedEntity) {
throw new AvailabilityUpdateFailed("Failed to update or write the entity.");
}zmsentities/src/Zmsentities/Collection/AvailabilityList.php (1)
203-234: Method getDateTimeRangeFromList implemented correctly
The getDateTimeRangeFromList method correctly computes the earliest start and latest end date-times from the availability list, adjusting for the selected date when necessary.
zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (1)
301-366: Test method testInvalidEndTime correctly implemented
The testInvalidEndTime method appropriately tests the scenario where the end time is before the start time, ensuring that the AvailabilityUpdateFailed exception is thrown.
zmsadmin/js/page/availabilityDay/index.js (1)
492-524: Validation logic refactored correctly
The getValidationList method has been successfully refactored to return a Promise, allowing for asynchronous validation of the availability list. This enhancement improves the handling of validation results and error reporting.
zmsentities/src/Zmsentities/Availability.php (2)
437-437: Good practice: Added explicit return type bool.
Adding the return type : bool to the hasDateBetween method enhances code clarity and type safety.
768-801: Verify overlap detection logic in getTimeOverlaps method.
The overlap detection logic compares the start and end times of availabilities. Ensure that date ranges are also considered, as overlaps might occur across different dates.
Run the following script to check for potential overlaps spanning multiple days:
✅ Verification successful
The overlap detection logic is correctly implemented
The code correctly handles both time and date overlaps:
- The
hasDateBetweenmethod checks if a given date falls within the availability's date range by comparing full timestamps. - The overlap detection in the provided snippet uses both date ranges (displayed in error messages) and time ranges:
- It first checks for exact time matches using
getSecondsOfDay() - Then checks for overlapping time ranges while considering the date context through
modify($currentDate->format("Y-m-d"))
- It first checks for exact time matches using
The implementation ensures that overlaps are detected within the same day context, which is the intended behavior as availabilities are day-specific and repeatable patterns rather than continuous spans across multiple days.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for availabilities with overlapping date ranges.
# Search for availabilities with overlapping date ranges
ast-grep 'public function getTimeOverlaps($$) {
$$$
}' --lang php
# Verify if date ranges are correctly handled in the overlap detection logic
Length of output: 226
Script:
#!/bin/bash
# Search for the getTimeOverlaps method implementation
ast-grep --pattern 'function getTimeOverlaps'
# Also search for any references to date comparisons in the Availability class
rg -A 5 "getStartDateTime|getEndDateTime" zmsentities/src/Zmsentities/Availability.php
Length of output: 8041
zmsadmin/js/page/availabilityDay/layouts/page.js (1)
14-14: LGTM: Duplicate saveBar rendering is intentional.
The duplicate rendering of saveBar is an intentional UX improvement to display save controls at both the top and bottom of the layout. This pattern was previously established and approved in PR #685.
Also applies to: 23-23
zmsadmin/js/page/availabilityDay/form/errors.js (1)
8-15: LGTM: Improved error handling with nested arrays.
The enhanced error rendering logic now properly handles nested error arrays while maintaining proper React key generation.
zmsadmin/js/page/availabilityDay/timetable/index.js (1)
37-37: LGTM: OpeningTimebar rendering logic maintained
The mapping of opening hours to the OpeningTimebar component is correctly preserved.
zmsadmin/js/page/availabilityDay/saveBar.js (1)
11-21: LGTM: Well-implemented auto-hide functionality
The implementation includes:
- Proper state management with useState
- Cleanup on unmount with clearTimeout
- Correct visibility control
zmsadmin/js/page/availabilityDay/form/conflicts.js (1)
14-24: LGTM: Improved conflict aggregation logic
The new data structure using objects with message and dates arrays is more maintainable and efficient than the previous implementation.
zmsadmin/js/page/availabilityDay/form/formButtons.js (1)
24-24: LGTM: Button disable logic enhanced with slot count validation
The update to include hasSlotCountError in the button's disabled state aligns well with the PR's objective of improving validation.
zmsdb/tests/Zmsdb/AvailabilityTest.php (1)
77-77: Verify the impact of changing slotTimeInMinutes from 12 to 10
This change in the expected slot time could have broader implications across the system. Please ensure this change is intentional and properly documented.
Let's check for other occurrences of slot time values:
✅ Verification successful
Change from 12 to 10 minutes appears to be intentional and aligned with system defaults
Based on the search results, the change from 12 to 10 minutes in the test appears to be correct and aligned with the system's actual behavior:
- The default slot time is defined as 10 minutes in
zmsentities/src/Zmsentities/Availability.php - Most test fixtures and JSON configurations use 10 minutes as the slot time
- Only a few legacy test fixtures still reference 12 minutes
- The database query in
zmsdb/src/Zmsdb/Query/Availability.phpcalculates slot time in minutes from seconds
The test change appears to be fixing an incorrect expectation to match the actual system default of 10 minutes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to slot time values
echo "Searching for slot time references in PHP files..."
rg -t php "slotTimeInMinutes.*=.*\d+" -A 2
echo "Searching for slot time references in JS files..."
rg -t js "slotTimeInMinutes.*=.*\d+" -A 2
echo "Searching for slot time references in JSON schema..."
fd -e json | xargs rg "slotTimeInMinutes"
Length of output: 47447
zmsentities/schema/availability.json (1)
21-21: Verify the impact of reducing the default slot time.
Changing the default slot time from 12 to 10 minutes could affect existing appointments and scheduling logic.
zmsdb/tests/Zmsdb/ProcessConflictTest.php (1)
56-57: LGTM!
The comment formatting change improves readability while maintaining the documentation.
zmsadmin/js/page/availabilityDay/form/content.js (1)
144-144: Verify browser compatibility for nullish coalescing operator.
The use of the nullish coalescing operator (??) improves null handling, but ensure it's compatible with your supported browsers.
Also applies to: 153-153
✅ Verification successful
Let me check the webpack configuration as it might contain browser compatibility settings.
Let me check if there's any transpilation or build setup in the repository that might handle browser compatibility.
Based on the findings, I can now provide a final response:
The nullish coalescing operator is safe to use in this codebase
The project uses Parcel bundler (v2) for building, which automatically includes necessary polyfills and transformations for modern JavaScript features. Looking at the package.json files, we can see:
- The project uses modern dependencies and build tools
- Parcel v2 handles browser compatibility by default
- Core-js is included as a dependency in some packages, which provides polyfills for modern JavaScript features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for babel configuration and browser targets
# Check for babel config
fd -g "*babel*" -t f
# Check for browserslist config
fd -g "*browserslist*" -t f
# Check package.json for babel/browserslist config
fd "package.json" -x cat {} | jq -r '.browserslist, .babel'
Length of output: 174
Script:
#!/bin/bash
# Check webpack configuration for browser targets and babel settings
fd -g "*webpack*" -t f -x cat {}
# Check for any other build configuration files
fd -g "*.config.js" -t f -x cat {}
Length of output: 75
Script:
#!/bin/bash
# Look for build-related files and dependencies
fd -g "package.json" -x grep -l "babel\|browserslist\|webpack\|build\|transpil" {} \; -x cat {}
# Check for any build or dist directories
fd -t d "build|dist"
# Look for any JavaScript configuration files
fd -e js -x grep -l "babel\|browserslist\|webpack\|build\|transpil" {}
Length of output: 8521
zmsentities/tests/Zmsentities/AvailabilityTest.php (3)
182-182: LGTM: Updated available seconds calculations
The available seconds calculations have been updated to reflect the correct business hours:
- 6534000 seconds = 7.56 hours (previously 6.19 hours)
Also applies to: 199-199
501-501: LGTM: Slot timing and capacity adjustments
The following test expectations have been updated to reflect new business requirements:
- Increased public workstation count from 81 to 99
- Increased total slot count from 28 to 33
- Adjusted second slot time from '10:12' to '10:10' for more consistent intervals
Also applies to: 510-510, 512-512
881-957: Enhanced conflict detection messages
The conflict test assertions have been restructured with more descriptive messages to better identify:
- Overlapping availability times
- Exact matches between availabilities
- Slot size conflicts
Run the following script to verify consistent error messaging across the codebase:
✅ Verification successful
Error messages are consistently used across the codebase
The verification shows that the conflict error messages are consistently used across the codebase in both test files (AvailabilityTest.php, ProcessConflictTest.php) and the main implementation (Availability.php). The messages maintain the same format and wording for:
- Overlapping availability conflicts
- Exact match conflicts
- Slot size validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent conflict error messages across the codebase
# Search for conflict-related error messages
rg -A 2 'Konflikt: Eine neue Öffnungszeit überschneidet sich'
rg -A 2 'Konflikt: Zwei Öffnungszeiten sind gleich'
rg -A 2 'Der eingestellte Zeitschlitz.*sollte in die eingestellte Uhrzeit passen'
Length of output: 6235
zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (1)
13-26: LGTM: Added API call setup for availability data
The API call configuration has been added to properly test the availability conflicts endpoint.
Run the following script to verify the API endpoint configuration:
✅ Verification successful
API endpoint configuration and fixture are properly set up
The verification confirms:
- The
/scope/141/availability/endpoint is consistently used across multiple test files with similar parameter structure - The fixture file
GET_availability_68985.jsonexists at the correct location inzmsadmin/tests/Zmsadmin/fixtures/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API endpoint configuration and fixture usage
# Search for similar API endpoint usage
rg -A 2 "'/scope/\d+/availability/'"
# Check if the fixture file exists
fd -g "GET_availability_68985.json"
Length of output: 5431
zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityaddfailed.twig
Show resolved
Hide resolved
zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityupdatefailed.twig
Show resolved
Hide resolved
| const { data, onCopy, onExclusion, onEditInFuture, onUpdateSingle, onDelete, selectedDate, hasConflicts, hasSlotCountError } = props | ||
| const disabled = ((data && (! data.id || data.__modified === true)) || hasConflicts || hasSlotCountError); |
There was a problem hiding this comment.
Add PropTypes declaration for hasSlotCountError prop
The new hasSlotCountError prop is used in the component but missing from the PropTypes declaration. This should be added to maintain proper type checking.
Add the following to the PropTypes declaration:
FormButtons.propTypes = {
data: PropTypes.object,
hasConflicts: PropTypes.bool,
+ hasSlotCountError: PropTypes.bool,
onCopy: PropTypes.func,
onExclusion: PropTypes.func,
onEditInFuture: PropTypes.func,
onDelete: PropTypes.func,
onUpdateSingle: PropTypes.func,
selectedDate: PropTypes.number,
isCreatingExclusion: PropTypes.bool
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@ThomasAFink, adding hasSlotCountError to FormButtons.propTypes ensures proper runtime type checking during development. Since the hasSlotCountError prop is used within the component, declaring it in PropTypes helps React validate that the prop is passed correctly and is of the expected type. This can aid in catching potential bugs related to prop usage early in the development process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
zmsapi/tests/Zmsapi/AvailabilityAddTest.php (3)
21-22: Consider extracting date calculations into constantsThe magic numbers in date calculations (1, 2, 4, 5 days) could be extracted into named constants for better readability and maintenance.
+ private const ONE_DAY_IN_SECONDS = 24 * 60 * 60; + private const TWO_DAYS_FUTURE = 2 * self::ONE_DAY_IN_SECONDS; + private const FOUR_DAYS_FUTURE = 4 * self::ONE_DAY_IN_SECONDS; + private const FIVE_DAYS_FUTURE = 5 * self::ONE_DAY_IN_SECONDS; "startDate" => time() + self::TWO_DAYS_FUTURE, "endDate" => time() + self::FIVE_DAYS_FUTURE,Also applies to: 32-33
50-82: Add documentation to clarify the overlap scenarioWhile the test correctly validates overlapping times, adding a comment to explain the specific overlap scenario (10:00-18:00 overlapping with 09:00-17:00) would improve test clarity.
+ /** + * Test that overlapping availability times on the same day are rejected + * Scenario: Two entries for the same scope on the same day + * Entry 1: 09:00-17:00 + * Entry 2: 10:00-18:00 (overlaps with Entry 1) + */ public function testOverlappingAvailability()
118-139: Consider adding cross-day invalid time testWhile the current test validates invalid times within the same day, consider adding a test case for invalid times across different days to ensure comprehensive validation.
/** * Example additional test case: */ public function testInvalidCrossDayEndTime() { $this->setWorkstation(); $this->expectException(AvailabilityAddFailed::class); $this->render([], [ '__body' => json_encode([ 'availabilityList' => [ [ "description" => "End Date Before Start Date", "startDate" => time() + (3 * 24 * 60 * 60), "endDate" => time() + (2 * 24 * 60 * 60), // End date before start date "startTime" => "09:00:00", "endTime" => "17:00:00", "kind" => "default", "scope" => ["id" => 312] ] ], 'selectedDate' => date('Y-m-d') ]) ], []); }zmsentities/src/Zmsentities/Collection/AvailabilityList.php (1)
195-234: Consider adding validation for edge cases.The method handles the basic datetime calculations well, but consider adding validation for:
- Empty availability list
- Null datetime values
- Invalid datetime strings
public function getDateTimeRangeFromList(\DateTimeImmutable $selectedDate): array { + if (count($this) === 0) { + return [null, null]; + } + $earliestStartDateTime = null; $latestEndDateTime = null; foreach ($this as $availability) { // Convert Unix timestamp to date strings + if (!$availability->startDate || !$availability->endDate) { + continue; + } + $startDate = (new \DateTimeImmutable())->setTimestamp($availability->startDate)->format('Y-m-d'); $endDate = (new \DateTimeImmutable())->setTimestamp($availability->endDate)->format('Y-m-d');zmsentities/src/Zmsentities/Availability.php (5)
453-483: Consider extracting magic numbers to constants.The method logic is sound, but would benefit from defining constants for magic numbers:
class Availability extends Schema\Entity { + private const MIDNIGHT_HOUR = 0; + private const MIDNIGHT_MINUTE = 0; + public function validateStartTime(\DateTimeInterface $today, \DateTimeInterface $tomorrow, \DateTimeInterface $startDate, \DateTimeInterface $endDate, \DateTimeInterface $selectedDate, string $kind): array { // ... - if (($startHour == 0 && $startMinute == 0) || ($endHour == 0 && $endMinute == 0)) { + if (($startHour == self::MIDNIGHT_HOUR && $startMinute == self::MIDNIGHT_MINUTE) || + ($endHour == self::MIDNIGHT_HOUR && $endMinute == self::MIDNIGHT_MINUTE)) {
484-510: Consider extracting time calculations to a helper method.The time calculations could be encapsulated in a helper method for better reusability:
+ private function calculateDayMinutes(int $hours, int $minutes): int + { + return ($hours * 60) + $minutes; + } + public function validateEndTime(\DateTimeInterface $startDate, \DateTimeInterface $endDate): array { $errorList = []; $startHour = (int)$startDate->format('H'); $endHour = (int)$endDate->format('H'); $startMinute = (int)$startDate->format('i'); $endMinute = (int)$endDate->format('i'); - $dayMinutesStart = ($startHour * 60) + $startMinute; - $dayMinutesEnd = ($endHour * 60) + $endMinute; + $dayMinutesStart = $this->calculateDayMinutes($startHour, $startMinute); + $dayMinutesEnd = $this->calculateDayMinutes($endHour, $endMinute);
512-538: Consider extracting kind values to constants.The method would benefit from defining constants for kind values:
class Availability extends Schema\Entity { + private const KIND_ORIGIN = 'origin'; + private const KIND_FUTURE = 'future'; + public function validateOriginEndTime(\DateTimeInterface $today, \DateTimeInterface $yesterday, \DateTimeInterface $startDate, \DateTimeInterface $endDate, \DateTimeInterface $selectedDate, string $kind): array { // ... - $isOrigin = ($kind && $kind === 'origin'); + $isOrigin = ($kind && $kind === self::KIND_ORIGIN);
540-550: Consider validating against allowed types.The method could be enhanced to validate against a predefined list of allowed types:
class Availability extends Schema\Entity { + private const ALLOWED_TYPES = ['appointment', 'openinghours']; + public function validateType(string $kind): array { $errorList = []; - if (empty($kind)) { + if (empty($kind) || !in_array($kind, self::ALLOWED_TYPES)) { $errorList[] = [ 'type' => 'type', - 'message' => 'Typ erforderlich' + 'message' => empty($kind) ? 'Typ erforderlich' : 'Ungültiger Typ' ]; } return $errorList; }
577-588: Consider adding validation for negative values.The method could be enhanced to validate against negative day values:
public function validateBookableDayRange(int $startInDays, int $endInDays): array { $errorList = []; + if ($startInDays < 0 || $endInDays < 0) { + $errorList[] = [ + 'type' => 'bookableDayRange', + 'message' => 'Die Tagesangaben dürfen nicht negativ sein.' + ]; + return $errorList; + } + if ($startInDays > $endInDays) { $errorList[] = [ 'type' => 'bookableDayRange',zmsapi/src/Zmsapi/AvailabilityUpdate.php (4)
43-55: Consider refactoring input validation for better maintainabilityWhile the validation logic is thorough, consider extracting it into a dedicated validation method for better organization and reusability.
+ protected function validateInput($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.'); + } + + if(!isset($input['availabilityList'][0]['scope'])) { + throw new BadRequestException('Missing or invalid scope.'); + } + + if (!isset($input['selectedDate'])) { + throw new BadRequestException("'selectedDate' is required."); + } + } public function readResponse( RequestInterface $request, ResponseInterface $response, array $args ): ResponseInterface { // ... - if (!$input || count($input) === 0) { - throw new BadRequestException(); - } - - 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."); - } + $this->validateInput($input);
56-69: Enhance error handling for entity validationThe entity validation could benefit from more detailed error messages when validation fails.
foreach ($input['availabilityList'] as $item) { $entity = new Entity($item); - $entity->testValid(); + try { + $entity->testValid(); + } catch (\Exception $e) { + throw new BadRequestException(sprintf( + 'Invalid entity data at index %d: %s', + array_search($item, $input['availabilityList']), + $e->getMessage() + )); + }
108-122: Add documentation for origin and exclusion handling logicThe complex logic for handling origins and exclusions would benefit from clear documentation explaining the business rules and edge cases.
+ /** + * Process origin IDs and handle exclusions + * + * This logic: + * 1. Identifies the origin availability entry + * 2. Creates a new collection excluding: + * - Entries marked as 'exclusion' + * - The identified origin entry + * + * @param Collection $mergedCollection Input collection to process + * @return Collection Filtered collection without exclusions + */ $originId = null; foreach ($mergedCollection as $availability) { if (isset($availability->kind) && $availability->kind === 'origin' && isset($availability->id)) {
Line range hint
171-182: Add documentation for writeSpontaneousEntity methodThe purpose and behavior of this method should be documented for better maintainability.
+ /** + * Writes a spontaneous entity by modifying workstation counts and type + * + * This method: + * 1. Reads double types for the given entity + * 2. Resets workstation counts to zero + * 3. Updates description and type + * 4. Writes the modified entity back to storage + * + * @param Entity $entity The entity to process + * @return void + */ protected function writeSpontaneousEntity(Entity $entity): void { $doubleTypesEntity = (new AvailabilityRepository())->readEntityDoubleTypes($entity->id);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
zmsadmin/js/page/availabilityDay/form/errors.js(2 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(2 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(3 hunks)zmsapi/tests/Zmsapi/AvailabilityAddTest.php(2 hunks)zmsentities/src/Zmsentities/Availability.php(5 hunks)zmsentities/src/Zmsentities/Collection/AvailabilityList.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsadmin/js/page/availabilityDay/form/errors.js
- zmsapi/src/Zmsapi/AvailabilityAdd.php
🔇 Additional comments (11)
zmsapi/tests/Zmsapi/AvailabilityAddTest.php (3)
5-6: LGTM: Appropriate imports added
The imports for AvailabilityAddFailed and DateTime are correctly added and used throughout the test cases.
84-116: LGTM: Well-structured duplicate availability test
The test case effectively validates the rejection of duplicate availability entries with identical time slots.
158-174: LGTM: Method name and exception type now aligned
The changes correctly implement the previous review suggestion to align the method name with its purpose and expected exception.
zmsentities/src/Zmsentities/Collection/AvailabilityList.php (3)
21-21: LGTM!
The formatting change doesn't affect functionality.
164-193: LGTM! Well-structured validation implementation.
The method effectively orchestrates multiple validation checks while maintaining single responsibility principle. The error accumulation has been properly implemented using array_merge with $errorList.
Line range hint 775-808: LGTM! Improved conflict message clarity.
The enhanced conflict messages now provide better context with date ranges and time ranges, making it easier for users to understand and resolve conflicts.
zmsentities/src/Zmsentities/Availability.php (3)
115-115: LGTM! Improved type safety.
The changes to use lowercase string type hint and addition of bool return type hint follow PHP 7+ best practices.
Also applies to: 139-139, 437-437
552-575: LGTM! Well-implemented slot time validation.
The method correctly handles slot time validation with proper division by zero check and error messages.
776-808: LGTM! Improved conflict message clarity.
The enhanced conflict messages now provide better context with date and time ranges.
zmsapi/src/Zmsapi/AvailabilityUpdate.php (2)
103-106: Include validation details in error response
The commented error logging should be utilized to provide meaningful error information.
if (count($validations) > 0) {
- //error_log(json_encode($validations));
- throw new AvailabilityUpdateFailed();
+ throw new AvailabilityUpdateFailed(json_encode($validations));
}133-138: LGTM! Clean and well-structured update logic
The entity update logic is well-organized with proper error handling in the writeEntityUpdate method.
| $startDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->startDate)->format('Y-m-d'); | ||
| $endDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->endDate)->format('Y-m-d'); | ||
|
|
||
| $selectedDate = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $input['selectedDate'] . ' 00:00:00'); | ||
|
|
||
| $startDateTime = new \DateTimeImmutable("{$startDate} {$newAvailability->startTime}"); | ||
| $endDateTime = new \DateTimeImmutable("{$endDate} {$newAvailability->endTime}"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve date handling robustness
The date/time handling could be more robust by validating the format and handling potential failures.
- $startDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->startDate)->format('Y-m-d');
- $endDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->endDate)->format('Y-m-d');
-
- $selectedDate = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $input['selectedDate'] . ' 00:00:00');
-
- $startDateTime = new \DateTimeImmutable("{$startDate} {$newAvailability->startTime}");
- $endDateTime = new \DateTimeImmutable("{$endDate} {$newAvailability->endTime}");
+ try {
+ $startDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->startDate);
+ if ($startDate === false) {
+ throw new \Exception('Invalid start date timestamp');
+ }
+ $startDate = $startDate->format('Y-m-d');
+
+ $endDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->endDate);
+ if ($endDate === false) {
+ throw new \Exception('Invalid end date timestamp');
+ }
+ $endDate = $endDate->format('Y-m-d');
+
+ $selectedDate = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $input['selectedDate'] . ' 00:00:00');
+ if ($selectedDate === false) {
+ throw new \Exception('Invalid selected date format');
+ }
+
+ $startDateTime = \DateTimeImmutable::createFromFormat('Y-m-d H:i', "{$startDate} {$newAvailability->startTime}");
+ if ($startDateTime === false) {
+ throw new \Exception('Invalid start time format');
+ }
+
+ $endDateTime = \DateTimeImmutable::createFromFormat('Y-m-d H:i', "{$endDate} {$newAvailability->endTime}");
+ if ($endDateTime === false) {
+ throw new \Exception('Invalid end time format');
+ }
+ } catch (\Exception $e) {
+ throw new BadRequestException('Date/time validation failed: ' . $e->getMessage());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $startDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->startDate)->format('Y-m-d'); | |
| $endDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->endDate)->format('Y-m-d'); | |
| $selectedDate = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $input['selectedDate'] . ' 00:00:00'); | |
| $startDateTime = new \DateTimeImmutable("{$startDate} {$newAvailability->startTime}"); | |
| $endDateTime = new \DateTimeImmutable("{$endDate} {$newAvailability->endTime}"); | |
| try { | |
| $startDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->startDate); | |
| if ($startDate === false) { | |
| throw new \Exception('Invalid start date timestamp'); | |
| } | |
| $startDate = $startDate->format('Y-m-d'); | |
| $endDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->endDate); | |
| if ($endDate === false) { | |
| throw new \Exception('Invalid end date timestamp'); | |
| } | |
| $endDate = $endDate->format('Y-m-d'); | |
| $selectedDate = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $input['selectedDate'] . ' 00:00:00'); | |
| if ($selectedDate === false) { | |
| throw new \Exception('Invalid selected date format'); | |
| } | |
| $startDateTime = \DateTimeImmutable::createFromFormat('Y-m-d H:i', "{$startDate} {$newAvailability->startTime}"); | |
| if ($startDateTime === false) { | |
| throw new \Exception('Invalid start time format'); | |
| } | |
| $endDateTime = \DateTimeImmutable::createFromFormat('Y-m-d H:i', "{$endDate} {$newAvailability->endTime}"); | |
| if ($endDateTime === false) { | |
| throw new \Exception('Invalid end time format'); | |
| } | |
| } catch (\Exception $e) { | |
| throw new BadRequestException('Date/time validation failed: ' . $e->getMessage()); | |
| } |
Description
Improves form validation to improve data integrity
Avoids conflicting opening hours both on the current date and for future dates
Improves UI form error handling and messaging
Reference
Issues ZMS-3253 ZMS-3466 ZMS-3415 ZMS-1891
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Documentation