fix(ZMS-3253 ZMS-1891 ZMS-3415): Replicate frontend validation in the backend for the availability opening hours to improve data integrity and frontend validation messaging #685
Conversation
…orm error messaging
This reverts commit ce5d795.
ThomasAFink
changed the title
ThomasAFink
changed the title
ThomasAFink
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request includes extensive modifications across multiple JavaScript files and PHP classes related to availability management. Key changes involve enhancements to conflict handling and error rendering logic, improved validation for availability entries, and updates to the rendering of components. New properties and methods have been introduced to facilitate better state management and user feedback. Additionally, new Twig templates for error handling have been added, and existing tests have been updated to reflect these changes. Changes
Possibly related PRs
Suggested reviewers
Poem
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: 28
🧹 Outside diff range and nitpick comments (53)
zmsapi/src/Zmsapi/Exception/Availability/AvailabilityUpdateFailed.php (1)
Line range hint
5-7: Improve class documentationThe current docblock is a placeholder. Consider adding more descriptive documentation explaining when this exception is thrown and what it indicates.
/** - * example class to generate an exception + * Exception thrown when an availability update operation fails. + * + * This exception is thrown when the system encounters an error while + * attempting to update an existing availability record. This could be + * due to conflicts, validation failures, or database errors. */zmsapi/src/Zmsapi/Exception/Availability/AvailabilityAddFailed.php (1)
5-7: Enhance exception class documentationThe current documentation is too generic. Consider adding more specific details about when this exception is thrown and what it represents.
-/** - * example class to generate an exception - */ +/** + * Exception thrown when an attempt to create a new availability entry fails. + * + * This exception is typically thrown when: + * - There are conflicts with existing availability entries + * - The input data is invalid + * - The availability creation process encounters an error + */zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityaddfailed.twig (3)
3-4: Consider implementing internationalization support.The error message and title are currently hard-coded in German. Consider using translation keys to support multiple languages and maintain consistency across the application.
- "message":"Zu den angegebenen Daten konnte keine Öffnungszeit erstellt werden. Bitte überprüfen Sie Ihre Eingaben und korrigieren Sie diese gegebenenfalls.", - "title":"Konflikt: Öffnungszeit überschneidet sich mit einer bestehenden Zeit", + "message": "{{ 'availability.add.failed.message'|trans }}", + "title": "{{ 'availability.add.failed.title'|trans }}",
5-5: Use boolean value for reload parameter.Consider using a boolean value instead of string "1" for the reload parameter to maintain type consistency.
- "reload":"1" + "reload": true
1-7: Consider enhancing error feedback with specific conflict details.The current template provides a generic error message. Consider passing additional parameters to show specific details about the conflicting time slots to help users quickly identify and resolve the issue.
Example enhancement:
{% include "exception/bo/layout.twig" with { "message": "{{ 'availability.add.failed.message'|trans }}", "title": "{{ 'availability.add.failed.title'|trans }}", + "details": conflict_details, + "conflicting_slots": existing_slots, "reload": true } %}zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityupdatefailed.twig (2)
1-7: LGTM with minor formatting suggestions!The error handling implementation aligns well with the PR objectives of improving frontend validation messaging. The message and title provide clear guidance to users when availability conflicts occur.
Consider maintaining consistent indentation (currently mixing tabs and spaces).
{% include "exception/bo/layout.twig" with { - "message":"Zu den angegebenen Daten konnte keine Öffnungszeit geändert werden. Bitte überprüfen Sie Ihre Eingaben und korrigieren Sie diese gegebenenfalls.", - "title":"Konflikt: Öffnungszeit überschneidet sich mit einer bestehenden Zeit", + "message":"Zu den angegebenen Daten konnte keine Öffnungszeit geändert werden. Bitte überprüfen Sie Ihre Eingaben und korrigieren Sie diese gegebenenfalls.", + "title":"Konflikt: Öffnungszeit überschneidet sich mit einer bestehenden Zeit", "reload":"1" } %}
5-5: Consider using a boolean value for the reload flagInstead of using a string "1", consider using a boolean value for better type safety and clarity.
- "reload":"1" + "reload": truezmsadmin/js/page/availabilityDay/form/errors.js (3)
4-17: Consider improving key uniqueness and type safetyWhile the implementation handles both flat and nested error structures well, there are two areas for improvement:
- Key Generation: The current composite key
${index}-${nestedIndex}might not guarantee uniqueness in all cases. Consider using a more unique identifier:-<div key={`${index}-${nestedIndex}`}>{nestedItem.message}</div> +<div key={`${errors[key].id}-${index}-${nestedIndex}`}>{nestedItem.message}</div>
- Type Safety: Consider adding type checking for the message property:
-return <div key={index}>{item.message}</div>; +return <div key={index}>{item?.message || 'Unknown error'}</div>;
Line range hint
19-26: LGTM: Well-structured error component with good accessibilityThe component implementation is solid with proper semantic structure and accessibility attributes.
Consider adding an error boundary to gracefully handle any rendering failures:
class ErrorBoundary extends React.Component { state = { hasError: false }; static getDerivedStateFromError(error) { return { hasError: true }; } render() { if (this.state.hasError) { return <div className="message message--error">Error displaying messages</div>; } return this.props.children; } }Then wrap the error rendering:
-return ( +return ( + <ErrorBoundary> Object.keys(props.errorList).length > 0 ? <div className="message message--error" role="alert" aria-live="polite"> <h3>Folgende Fehler sind bei der Prüfung Ihrer Eingaben aufgetreten:</h3> {renderErrors(props.errorList)} </div> : null + </ErrorBoundary> );
29-35: Consider using more specific PropTypesWhile the current PropTypes definition works, it could be more specific to better document the expected shape of the errorList.
-errorList: PropTypes.object +errorList: PropTypes.objectOf(PropTypes.shape({ + id: PropTypes.string.isRequired, + itemList: PropTypes.arrayOf( + PropTypes.oneOfType([ + PropTypes.shape({ + message: PropTypes.string.isRequired + }), + PropTypes.arrayOf( + PropTypes.shape({ + message: PropTypes.string.isRequired + }) + ) + ]) + ).isRequired +}))This would provide better type checking and documentation of the expected data structure.
zmsadmin/js/page/availabilityDay/form/footerButtons.js (2)
7-7: Consider moving inline styles to CSSMoving the inline styles to a CSS file would improve maintainability and follow React best practices.
-<div className="form-actions" style={{ "marginTop": "0", "padding": "0.75em" }}> +<div className="form-actions form-actions--compact">Then add to your CSS:
.form-actions--compact { margin-top: 0; padding: 0.75em; }
Line range hint
15-22: Add missing PropType for hasSlotCountErrorThe
hasSlotCountErrorprop is used in the component but not defined in PropTypes.FooterButtons.propTypes = { data: PropTypes.object, hasConflicts: PropTypes.bool, stateChanged: PropTypes.bool, onNew: PropTypes.func, onPublish: PropTypes.func, - onAbort: PropTypes.func + onAbort: PropTypes.func, + hasSlotCountError: PropTypes.bool }zmsadmin/js/page/availabilityDay/saveBar.js (2)
1-1: Consider memoizing the formatDate function and handling localization.The utility function works correctly but could benefit from two improvements:
- Memoize the function to prevent unnecessary recalculations on re-renders
- Consider extracting the German date format strings to a localization configuration
-const formatDate = date => { +const formatDate = React.useMemo(() => (date) => { const momentDate = moment(date) return `${momentDate.format('DD.MM.YYYY')} um ${momentDate.format('HH:mm')} Uhr` -} +}, [])Also applies to: 5-8
11-19: Optimize the effect and consider making the timeout configurable.The effect implementation is correct but could be improved:
- The timeout duration should be a prop or constant for better maintainability
- The effect could be memoized since it doesn't depend on any props
+const HIDE_TIMEOUT = 5000 + const SaveBar = (props) => { const [isVisible, setIsVisible] = useState(true) - useEffect(() => { + useEffect(() => { const timer = setTimeout(() => { setIsVisible(false) - }, 5000) + }, HIDE_TIMEOUT) return () => clearTimeout(timer) - }, []) + }, []) // Consider adding props.success to dependencies if you want to reset visibility on status changezmsadmin/js/page/availabilityDay/form/conflicts.js (1)
14-27: Consider optimizing conflict aggregation performanceThe current implementation uses
Array.find()which has O(n) complexity for each conflict. For better performance with large datasets, consider using a Map for O(1) lookups.const renderConflictList = (conflictList) => { - let conflictDatesByMessage = []; + const conflictMap = new Map(); conflictList.map(collection => { collection.conflicts.map((conflict) => { - const existingConflict = conflictDatesByMessage.find( - item => item.message === conflict.message - ); - - if (existingConflict) { - existingConflict.dates.push(formatDate(collection.date)); + if (conflictMap.has(conflict.message)) { + conflictMap.get(conflict.message).dates.push(formatDate(collection.date)); } else { - conflictDatesByMessage.push({ + conflictMap.set(conflict.message, { message: conflict.message, dates: [formatDate(collection.date)] }); } }); }); + return Array.from(conflictMap.values());zmsadmin/js/page/availabilityDay/form/formButtons.js (1)
Line range hint
30-39: Add PropType declaration for hasSlotCountErrorThe new
hasSlotCountErrorprop is missing from the PropTypes declaration. This should be added to maintain proper type checking and component documentation.Apply this diff to add the missing PropType:
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 }zmsadmin/js/page/availabilityDay/form/index.js (3)
Line range hint
39-47: Simplify conflict detection logicThe nested conditional can be simplified for better readability.
Consider refactoring to:
- const hasConflicts = (( - this.props.conflictList.itemList && - Object.keys(this.props.conflictList.itemList).length - ) || - ( - this.props.errorList && - Object.keys(this.props.errorList).length - )) ? true : false + const hasConflicts = Boolean( + (this.props.conflictList.itemList && + Object.keys(this.props.conflictList.itemList).length) || + (this.props.errorList && + Object.keys(this.props.errorList).length) + )
Line range hint
84-98: Add missing PropType for hasSlotCountErrorThe
hasSlotCountErrorprop is being used in FormButtons but isn't defined in the component's PropTypes.Add the prop type definition:
AvailabilityForm.propTypes = { availabilityList: PropTypes.array, errorList: PropTypes.object, conflictList: PropTypes.object, data: PropTypes.object, today: PropTypes.number, selectedDate: PropTypes.number, handleChange: PropTypes.func, onCopy: PropTypes.func, onExclusion: PropTypes.func, onEditInFuture: PropTypes.func, setErrorRef: PropTypes.func, onDelete: PropTypes.func, onUpdateSingle: PropTypes.func, - isCreatingExclusion: PropTypes.bool + isCreatingExclusion: PropTypes.bool, + hasSlotCountError: PropTypes.bool }
Line range hint
8-73: Consider breaking down component responsibilitiesThe
AvailabilityFormcomponent is handling multiple concerns:
- Form state management
- Data transformation
- Validation logic
- Multiple child component coordination
Consider splitting this into smaller, more focused components or using hooks for better maintainability. This could involve:
- Moving form state to a custom hook
- Creating a separate validation container
- Using composition for the button logic
zmsdb/tests/Zmsdb/ProcessConflictTest.php (1)
103-105: Consider using CSS for spacing instead of HTML entities.While the message content is clear and well-structured, using
 HTML entities for spacing might not render consistently across all contexts. Consider using CSS for spacing or simple spaces with monospace font to achieve the desired alignment.- $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]", $conflictList->getFirst()->getAmendment()); + $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]", $conflictList->getFirst()->getAmendment());zmsentities/src/Zmsentities/Collection/AvailabilityList.php (1)
164-220: Consider architectural improvements for validation and time handlingThe new methods enhance the validation and time range calculation capabilities of the
AvailabilityListclass. However, consider the following architectural improvements:
- Extract validation logic into a dedicated validator service to maintain single responsibility principle
- Consider using a Value Object pattern for time ranges to encapsulate the logic and validation
- Implement a proper error handling strategy across the application for consistency
Would you like assistance in designing these architectural improvements?
zmsadmin/js/page/availabilityDay/form/validate.js (5)
28-28: Simplify the boolean conversion.The boolean conversion can be simplified for better readability.
-let valid = (0 < errorList.itemList.length) ? false : true; +let valid = errorList.itemList.length === 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: 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)
36-129: Consider type checking and error message constants.The validation functions are well-structured with comprehensive checks. Consider these improvements:
- Add type checking for the data parameter
- Extract error messages to constants for reusability and maintenance
Example implementation:
const ERROR_MESSAGES = { START_DATE_NULL: 'Das Startdatum darf nicht leer sein.', END_DATE_NULL: 'Das Enddatum darf nicht leer sein.', // ... other messages }; function validateNullValues(data) { if (typeof data !== 'object' || data === null) { throw new TypeError('Data parameter must be an object'); } // ... rest of the function }
Line range hint
183-204: Simplify time comparison logic using parseTimestampAndTime.The time comparison logic can be simplified by utilizing the parseTimestampAndTime function you've already implemented.
function validateEndTime(today, yesterday, selectedDate, data) { var errorList = [] - const startTime = moment(data.startDate, 'X').startOf('day'); - const endTime = moment(data.endDate, 'X').startOf('day'); - const startHour = data.startTime.split(':')[0] - const endHour = data.endTime.split(':')[0] - const startMinute = data.startTime.split(':')[1] - const endMinute = data.endTime.split(':')[1] - const dayMinutesStart = (parseInt(startHour) * 60) + parseInt(startMinute); - const dayMinutesEnd = (parseInt(endHour) * 60) + parseInt(endMinute); - const startTimestamp = startTime.set({ h: startHour, m: startMinute }).unix(); - const endTimestamp = endTime.clone().set({ h: endHour, m: endMinute }).unix(); + const startDateTime = parseTimestampAndTime(data.startDate, data.startTime); + const endDateTime = parseTimestampAndTime(data.endDate, data.endTime); + + if (!startDateTime || !endDateTime) return errorList; + + const startTimestamp = startDateTime.unix(); + const endTimestamp = endDateTime.unix(); - if (dayMinutesEnd <= dayMinutesStart) { + if (endDateTime.isSameOrBefore(startDateTime)) { errorList.push({ type: 'endTime', message: 'Die Endzeit darf nicht vor der Startzeit liegen.' }) } - if (startTimestamp >= endTimestamp) { + if (startDateTime.isSameOrAfter(endDateTime)) { errorList.push({ type: 'endTime', message: 'Das Enddatum darf nicht vor dem Startdatum liegen.' }) } return errorList; }
Line range hint
241-283: Optimize slot validation and error checking.The slot validation and error checking functions can be simplified for better performance and maintainability.
- Simplify slot validation:
function validateSlotTime(data) { let errorList = [] - const startTime = moment(data.startDate, 'X'); - const startHour = data.startTime.split(':')[0] - const endHour = data.endTime.split(':')[0] - const startMinute = data.startTime.split(':')[1] - const endMinute = data.endTime.split(':')[1] - const startTimestamp = startTime.set({ h: startHour, m: startMinute }).unix(); - const endTimestamp = startTime.set({ h: endHour, m: endMinute }).unix(); + const startDateTime = parseTimestampAndTime(data.startDate, data.startTime); + const endDateTime = parseTimestampAndTime(data.endDate, data.endTime); + + if (!startDateTime || !endDateTime) return errorList; + + const durationInMinutes = endDateTime.diff(startDateTime, 'minutes'); const slotTime = data.slotTimeInMinutes - let slotAmount = (endTimestamp - startTimestamp) / 60 % slotTime; + if (durationInMinutes % slotTime !== 0) { errorList.push({ type: 'slotCount', message: 'Zeitschlitze müssen sich gleichmäßig in der Öffnungszeit aufteilen lassen.' }) } return errorList; }
- Optimize error checking:
export function hasSlotCountError(dataObject) { - const errorList = dataObject?.errorList; - for (let key in errorList) { - if (errorList.hasOwnProperty(key)) { - const error = errorList[key]; - if (error && Array.isArray(error.itemList)) { - for (let sublist of error.itemList) { - if (Array.isArray(sublist)) { - for (let item of sublist) { - if (item && item.type === 'slotCount') { - return true; - } - } - } - } - } - } - } - return false; + return dataObject?.errorList?.some(error => + error?.itemList?.some(sublist => + Array.isArray(sublist) && + sublist.some(item => item?.type === 'slotCount') + ) + ) ?? false; }🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: 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)
[error] 132-132: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
1-1: Add JSDoc documentation for exported functions.Consider adding JSDoc documentation for the exported functions to improve maintainability and IDE support.
Example:
/** * Validates the availability form data * @param {Object} data - The form data to validate * @param {Object} props - Additional properties for validation * @returns {Object} Validation result with valid flag and error list */ const validate = (data, props) => { // ... existing implementation }; /** * Checks if the data object contains any slot count errors * @param {Object} dataObject - The data object containing error list * @returns {boolean} True if slot count errors exist, false otherwise */ export function hasSlotCountError(dataObject) { // ... existing implementation }zmsentities/tests/Zmsentities/ProcessTest.php (1)
Line range hint
475-496: Consider enhancing test coverage for conflict detection.The test method
testToConflictListByDaycould benefit from additional test cases:
- Edge cases for slot calculations
- Overlapping appointments
- Appointments spanning multiple slots
Consider adding these test cases:
public function testToConflictListByDay() { $collection = new $this->collectionclass(); $entity = $this->getExample(); $entity2 = $this->getExample(); $availability = (new \BO\Zmsentities\Availability())->getExample(); $availability->id = "1"; $availability2 = (new \BO\Zmsentities\Availability())->getExample(); $availability2->id = "2"; $entity->getFirstAppointment()->availability = $availability; $entity2->getFirstAppointment()->availability = $availability2; $collection->addEntity($entity); $collection->addEntity($entity2); $list = $collection->toConflictListByDay(); $this->assertArrayHasKey('2015-11-18', $list); $this->assertEquals('18:52', $list['2015-11-18'][0]['appointments'][0]['startTime']); $this->assertEquals('19:12', $list['2015-11-18'][0]['appointments'][0]['endTime']); + + // Test overlapping appointments + $entity3 = $this->getExample(); + $entity3->getFirstAppointment()->availability = $availability; + $entity3->getFirstAppointment()->setTime('19:00'); + $collection->addEntity($entity3); + $list = $collection->toConflictListByDay(); + $this->assertCount(2, $list['2015-11-18'][0]['appointments'], 'Should detect overlapping appointments'); + + // Test appointments spanning multiple slots + $entity4 = $this->getExample(); + $entity4->getFirstAppointment()->availability = $availability; + $entity4->getFirstAppointment()->slotCount = 3; + $collection->addEntity($entity4); + $list = $collection->toConflictListByDay(); + $this->assertEquals('19:24', $list['2015-11-18'][0]['appointments'][3]['endTime'], 'Should handle multiple slots'); }zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (3)
13-26: Add test method documentation to improve maintainability.The test setup is well-structured, but adding PHPDoc comments explaining the test's purpose, the fixture data's structure, and expected behavior would improve maintainability.
Add documentation like this:
+ /** + * Tests the rendering of availability conflicts + * + * This test verifies that: + * 1. Overlapping availability entries are detected + * 2. Correct dates are included in the response + * 3. Conflict IDs are properly reported + * + * Fixture: GET_availability_68985.json contains sample availability data + */ public function testRendering() {
Line range hint
27-1000: Improve test maintainability by extracting test data.The test contains a large inline JSON payload which makes it hard to maintain. Consider extracting it to a separate fixture file.
Create a new fixture file and modify the test:
+ private function getTestAvailabilityData(): string + { + return file_get_contents(__DIR__ . '/fixtures/availability_conflicts_test_data.json'); + } public function testRendering() { $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' => '{...}'], [], 'POST'); + $response = $this->render([], ['__body' => $this->getTestAvailabilityData()], [], 'POST');
Line range hint
27-1000: Enhance test coverage with additional assertions.While the test covers basic conflict detection, it could benefit from additional assertions to verify the complete response structure.
Add these assertions:
$this->assertStringContainsString('"conflictIdList":["81871","__temp__0"]', (string)$response->getBody()); + + // Verify response structure + $responseData = json_decode((string)$response->getBody(), true); + $this->assertArrayHasKey('conflicts', $responseData); + $this->assertArrayHasKey('dates', $responseData); + + // Verify workstation counts + $this->assertEquals(3, $responseData['conflicts'][0]['workstationCount']['public']); + + // Verify time ranges + $this->assertStringContainsString('08:00:00', (string)$response->getBody()); + $this->assertStringContainsString('15:50:00', (string)$response->getBody()); $this->assertEquals(200, $response->getStatusCode());zmsapi/src/Zmsapi/AvailabilityAdd.php (4)
59-60: Remove unused variables to improve code clarityThe variables
$startDateand$endDateare assigned but not used in the subsequent code. Removing these unused variables can improve code readability.Apply this diff to remove the unused variables:
-$startDate = new \DateTimeImmutable('now'); -$endDate = (new \DateTimeImmutable('now'))->modify('+1 month');
88-89: Consider removing or handling commented-outerror_logstatementsThere are commented-out
error_logstatements in the code. If these logs are no longer needed, consider removing them. If they are helpful for debugging, consider implementing a proper logging mechanism or wrapping them in a condition that checks for a debug mode.Apply this diff if you decide to remove them:
-//error_log(json_encode($validation));-//error_log(json_encode($conflicts));Also applies to: 95-96
113-126: Inconsistent usage of$resolveReferencesinwriteEntityUpdatemethodIn the
writeEntityUpdatemethod, the$resolveReferencesparameter is used when callingupdateEntity, but a hardcoded value of2is used when callingwriteEntity. For consistency and to respect the parameter's purpose, consider passing$resolveReferencestowriteEntityas well.Apply this diff to fix the inconsistency:
} else { - $updatedEntity = $repository->writeEntity($entity, 2); + $updatedEntity = $repository->writeEntity($entity, $resolveReferences); }
127-127: Provide more informative exception messagesWhen throwing
AvailabilityAddFailed, consider providing a detailed message to aid in debugging and user feedback. This can help identify the exact reason for the failure.Apply this diff to include an exception message:
if (!$updatedEntity) { - throw new AvailabilityAddFailed(); + throw new AvailabilityAddFailed('Failed to update or write the availability entity.'); }zmsapi/src/Zmsapi/AvailabilityUpdate.php (3)
95-96: Avoid usingerror_logfor logging validation and conflict errorsUsing
error_logto log validation errors and conflicts may not be appropriate for production systems, as it can clutter the error logs and may not provide structured logging. Consider using a proper logging mechanism or including the error details in the exception message to provide better feedback to the API consumer.Apply this diff to include error details in the exceptions:
- error_log(json_encode($allValidations)); - throw new AvailabilityUpdateFailed(); + $errorDetails = json_encode($allValidations); + throw new AvailabilityUpdateFailed("Validation failed: $errorDetails"); ... - error_log(json_encode($conflicts)); - throw new AvailabilityUpdateFailed(); + $conflictDetails = json_encode($conflicts); + throw new AvailabilityUpdateFailed("Conflicts detected: $conflictDetails");Also applies to: 102-103
128-130: Check if updateEntity operation succeeds and handle potential failuresIn the
writeEntityUpdatemethod, after calling$repository->updateEntity, you should verify whether the update was successful. Currently, if the update fails,$updatedEntitymay benull, which is handled later, but more specific error handling could improve debugging.Consider adding error handling immediately after the update operation:
$updatedEntity = $repository->updateEntity($entity->id, $entity, $resolveReferences); + if (!$updatedEntity) { + throw new AvailabilityUpdateFailed("Failed to update entity with ID {$entity->id}."); + }
Line range hint
141-150: Clarify the purpose and impact ofwriteSpontaneousEntityThe
writeSpontaneousEntitymethod modifies several properties of$doubleTypesEntityand writes it back. It's not immediately clear what the purpose is and how this affects the overall system. Adding comments or documentation would improve maintainability and understanding for future developers.Consider adding comments explaining the intention behind this method.
zmsapi/tests/Zmsapi/AvailabilityAddTest.php (2)
161-161: Use class constant for exception expectation for consistencyIn line 161, you're using a string to specify the expected exception:
$this->expectException('\BO\Zmsapi\Exception\Availability\AvailabilityAddFailed');For consistency and to leverage IDE support and static analysis tools, it's better to use the class constant
AvailabilityAddFailed::class, as you've done elsewhere in the code.Apply this diff to improve consistency:
-$this->expectException('\BO\Zmsapi\Exception\Availability\AvailabilityAddFailed'); +$this->expectException(AvailabilityAddFailed::class);
42-42: Use fixed 'selectedDate' for consistent test outcomesThe
selectedDatefield currently usesdate('Y-m-d'), which depends on the current date. This can cause tests to behave differently depending on when they are run. To ensure consistent and reliable tests, use a fixed date value.Suggested change:
-'selectedDate' => date('Y-m-d') +'selectedDate' => '2023-12-01'Apply this change to all occurrences of
selectedDatein your tests.Also applies to: 79-79, 113-113, 136-136, 173-173
zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (3)
148-152: Clarify the usage of IDs intestDuplicateOverlappingAvailabilityIn the test data, the availability entries have IDs set using
$entity->getId()and$secondEntity->getId(), but the comment indicates a "Duplicate ID". Unless both entities share the same ID, they are not duplicates. Please ensure that if the test aims to simulate duplicate IDs, the IDs are explicitly set to be the same. Otherwise, update the comment to accurately reflect the test scenario.
270-274: Clarify the usage of IDs intestOverlappingAvailabilitySimilar to the previous test, the availability entries use
$entity->getId()and$secondEntity->getId()with a comment suggesting a "Duplicate ID". Verify whether the IDs are intended to be duplicates. If not, consider correcting the comment to prevent confusion.
86-110: Refactor common test setup code into a helper methodThe methods
testDuplicateOverlappingAvailability(),testOverlappingAvailability(), andtestInvalidEndTime()share similar code for initializing$inputdata. Extracting this code into a private helper method, such ascreateAvailabilityInput(), would reduce duplication and improve maintainability.Also applies to: 182-206, 304-328
zmsadmin/js/page/availabilityDay/index.js (6)
Line range hint
79-82: Incorrect lifecycle method: UsecomponentWillUnmountThe method
componentDidUnMountis not a valid React lifecycle method. It should becomponentWillUnmountto ensure proper cleanup when the component is unmounted.Apply this diff to correct the method name:
-componentDidUnMount() { +componentWillUnmount() { window.removeEventListener('beforeunload', this.unloadHandler) }
114-138: Enhance data preparation by removing unnecessary propertiesConsider removing unnecessary properties like
tempIdfromsendAvailabilitybefore sending the data. Also, ensure that logging statements are removed or adjusted for production.Apply this diff to improve data preparation:
const sendData = this.state.availabilitylist .filter((availability) => { return ( (availability.__modified || - (availability.tempId && availability.tempId.includes('__temp__'))) && + (availability.tempId?.includes('__temp__'))) && !this.hasErrors(availability) ); }) .map(availability => { const sendAvailability = Object.assign({}, availability); - if (availability.tempId) { - delete sendAvailability.tempId; - } - console.log(availability.kind); + delete sendAvailability.tempId; return { ...sendAvailability, kind: availability.kind || 'default', }; }) .map(cleanupAvailabilityForSave);🧰 Tools
🪛 Biome (1.9.4)
[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)
127-127: Remove unnecessary console.log statementsThe
console.logstatement can clutter the console and should be removed before deploying to production.Apply this diff to remove the debug log:
- console.log(availability.kind);
125-126: Avoid using thedeleteoperator for better performanceUsing the
deleteoperator can impact performance negatively. Instead, set the property toundefinedornull.First occurrence:
- delete sendAvailability.tempId; + sendAvailability.tempId = undefined;Second occurrence:
- delete sendAvailability.tempId; + sendAvailability.tempId = undefined;Also applies to: 203-204
🧰 Tools
🪛 Biome (1.9.4)
[error] 125-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
515-516: Avoid potential null reference errors by checkingerrorElementEnsure that
this.errorElementis not null or undefined before callingscrollIntoView()to prevent runtime errors.Apply this diff:
if (this.errorElement) { this.errorElement.scrollIntoView(); }
621-662: Handle potential errors inreadCalculatedAvailabilityListEnsure that error handling provides meaningful feedback to the user, and avoid silent failures. Also, consider checking the response status before proceeding.
Review the error handling logic and adjust as necessary to improve robustness.
zmsentities/src/Zmsentities/Availability.php (4)
458-459: Ensure consistent data types for$startHourand$endHourOn line 458,
$startHouris assigned without casting to an integer, whereas$endHouron line 459 is cast using(int). To maintain consistency and avoid potential type issues, consider casting$startHourto an integer as well.Apply this diff:
-$startHour = ($startDate->format('H')); +$startHour = (int)$startDate->format('H');
484-510: Refactor redundant conditions invalidateEndTimeThe validation checks in lines 497-507 have overlapping conditions:
- Line 497 checks if
$dayMinutesEnd <= $dayMinutesStart.- Line 502 checks if
$startTimestamp >= $endTimestamp.These conditions can be consolidated to simplify the logic and avoid redundancy.
Consider refactoring as follows:
if ($startTimestamp >= $endTimestamp) { $errorList[] = [ 'type' => 'endTime', 'message' => 'Das Enddatum darf nicht vor dem Startdatum liegen.' ]; } elseif ($dayMinutesEnd <= $dayMinutesStart) { $errorList[] = [ 'type' => 'endTime', 'message' => 'Die Endzeit darf nicht vor der Startzeit liegen.' ]; }
530-535: Improve readability of the error messageThe error message constructed in lines 533-535 is complex due to string concatenation. Consider simplifying the message or using string interpolation for better readability.
Example:
$errorList[] = [ 'type' => 'endTimePast', 'message' => sprintf( 'Öffnungszeiten in der Vergangenheit lassen sich nicht bearbeiten (Die aktuelle Zeit "%s Uhr" liegt nach dem Terminende am "%s Uhr").', $today->format('d.m.Y H:i'), $endDateTime->format('d.m.Y H:i') ) ];
580-581: Handle empty error lists invalidateAllIn the
validateAllmethod, the merged$errorListmay be empty if all validations pass. Ensure that the caller of this method appropriately handles an empty error list, or consider returningnullwhen there are no errors.zmsentities/tests/Zmsentities/AvailabilityTest.php (1)
881-954: Avoid hardcoding assertion messages intestConflictsThe test assertions use hardcoded conflict messages in German. Hardcoding messages can make tests brittle and difficult to maintain, especially if messages change or localization is needed. Consider refactoring the tests to compare conflict identifiers or use constants for the messages.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (29)
zmsadmin/js/page/availabilityDay/form/conflicts.js(1 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(5 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(2 hunks)zmsentities/src/Zmsentities/Collection/AvailabilityList.php(1 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/helpers.js
- zmsadmin/js/page/availabilityDay/timetable/graphview.js
🧰 Additional context used
🪛 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] 28-28: 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)
[error] 132-132: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
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)
[error] 529-529: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (35)
zmsapi/src/Zmsapi/Exception/Availability/AvailabilityAddFailed.php (2)
8-11: LGTM: Exception class structure
The class extends the base Exception class correctly and uses an appropriate HTTP status code (400) for bad request scenarios.
8-13: Verify exception usage and handling
Let's ensure this exception is properly integrated with the rest of the codebase.
✅ Verification successful
Exception is properly integrated and tested
The exception is well-integrated into the codebase:
- Thrown in
AvailabilityAdd.phpfor validation failures, conflicts, and write failures - Comprehensive test coverage in
AvailabilityAddTest.phpwith multiple test cases:- Overlapping availability scenarios
- Duplicate entries
- Invalid time entries
- Update failures
- HTTP status code (400) and message are appropriate for the use case
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage and handling of AvailabilityAddFailed exception
# Find where the exception is thrown
echo "=== Locations where AvailabilityAddFailed is used ==="
rg -l "AvailabilityAddFailed"
# Find exception handling patterns
echo -e "\n=== Exception handling patterns ==="
rg "catch.*AvailabilityAddFailed"
# Find similar availability-related exceptions for consistency
echo -e "\n=== Similar availability exceptions ==="
fd -e php . | rg -l "class.*Availability.*Failed"
Length of output: 565
Script:
#!/bin/bash
# Let's examine the actual usage context in the files
echo "=== Content of AvailabilityAdd.php where exception might be thrown ==="
rg -A 5 -B 5 "throw.*AvailabilityAddFailed" zmsapi/src/Zmsapi/AvailabilityAdd.php
echo -e "\n=== Test file content to understand exception handling ==="
cat zmsapi/tests/Zmsapi/AvailabilityAddTest.php
echo -e "\n=== Check for any try-catch blocks handling availability exceptions ==="
rg -A 5 -B 5 "try.*{.*availability" -i
echo -e "\n=== Looking for similar exception classes in the codebase ==="
ast-grep --pattern 'class $_ extends \Exception
{
$$$
}'
Length of output: 8392
zmsadmin/js/page/availabilityDay/layouts/page.js (1)
23-23: LGTM: PropTypes declaration is correct
The PropTypes declaration for saveBar is properly defined as a node type, which is appropriate for React component props.
zmsadmin/js/page/availabilityDay/form/errors.js (1)
1-2: LGTM: Imports are clean and necessary
The imports are appropriate for a React component using PropTypes.
zmsadmin/js/page/availabilityDay/form/footerButtons.js (3)
8-8: LGTM! Good validation enhancement
Adding hasConflicts to the disable condition helps prevent users from creating new availability entries while conflicts exist, improving data integrity.
9-9: LGTM! Logical abort button behavior
The abort button's disable condition correctly allows users to cancel when there are changes, conflicts, or data to be discarded.
10-11: LGTM! Enhanced validation for save action
Adding hasConflicts to the save button's disable condition prevents users from saving invalid data, which aligns well with the PR's data integrity objectives.
zmsadmin/js/page/availabilityDay/saveBar.js (1)
Line range hint 34-41: LGTM! PropTypes are well-defined.
The PropTypes declaration is comprehensive and correctly validates all props, including the new success boolean prop.
zmsadmin/js/page/availabilityDay/timetable/index.js (1)
37-37: LGTM! Verify integration with validation logic.
The implementation follows React best practices for rendering lists of components. The change aligns well with the PR's objective of improving validation.
Let's verify the integration with the new validation logic:
zmsadmin/js/page/availabilityDay/form/formButtons.js (3)
5-6: LGTM: Proper integration of slot count validation
The addition of hasSlotCountError to the disabled state calculation enhances form validation by preventing actions when slot count validation fails.
24-24: LGTM: Consistent validation handling
The "Aktualisieren" button's disable condition properly includes the slot count validation, maintaining consistency with other form controls.
5-5: Verify FormButtons usage across the codebase
Let's ensure all parent components properly pass the new hasSlotCountError prop to maintain consistent validation behavior.
✅ Verification successful
Let me try a different approach to find the component usage. The previous pattern might have failed due to syntax issues or the way the component is imported/used in the codebase.
I see the FormButtons usage in the parent component, but I need to verify how the hasSlotCountError prop is being handled and passed. Let me check the complete parent component implementation.
I see that the parent component properly passes the hasSlotCountError prop by calling the imported validation function. Let me check the validation implementation to ensure it's properly defined and exported.
Verification completed: FormButtons component is properly integrated
The hasSlotCountError prop is correctly implemented and used:
- Parent component
AvailabilityFormproperly passes the prop using the importedhasSlotCountErrorvalidation function - The validation function is properly defined and exported in
validate.js - The prop is used correctly in
FormButtonsto disable buttons when slot count validation fails - No other usages of
FormButtonsin the codebase require this prop as it's specific to the availability day form
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FormButtons component usage
ast-grep --pattern 'FormButtons $$$' || rg -A 5 '<FormButtons'
Length of output: 38
Script:
#!/bin/bash
# Search for FormButtons component usage with ripgrep
rg -i "formbuttons" --type js -A 5
Length of output: 3446
Script:
#!/bin/bash
# Let's check the content of the parent component to see how hasSlotCountError is passed
cat zmsadmin/js/page/availabilityDay/form/index.js
Length of output: 3330
Script:
#!/bin/bash
# Let's check the validate.js file to understand the hasSlotCountError implementation
cat zmsadmin/js/page/availabilityDay/form/validate.js
Length of output: 9808
zmsadmin/js/page/availabilityDay/form/index.js (2)
68-68: Consider destructuring props for validation
The validation function might not need all props. Consider destructuring specific props needed for validation.
6-6: Verify the imported validation function
Let's ensure the hasSlotCountError function is properly exported from the validate module.
✅ Verification successful
Import verification successful
The hasSlotCountError function is properly exported and defined in zmsadmin/js/page/availabilityDay/form/validate.js. The function implementation is complete and matches the expected usage in the form component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and export of hasSlotCountError function
# Expected: Find the function definition and its export statement
# Search for the function definition
ast-grep --pattern 'export function hasSlotCountError($_) { $$$ }'
# Alternative pattern if it's a const/let/var
ast-grep --pattern 'export const hasSlotCountError = $_'
Length of output: 18706
zmsdb/tests/Zmsdb/AvailabilityTest.php (1)
77-77: LGTM! Test updated to reflect new slot time duration.
The assertion update from 12 to 10 minutes correctly aligns with the schema changes in zmsentities/schema/availability.json.
Let's verify the consistency of this breaking change across the codebase:
✅ Verification successful
Based on the search results, I can now provide a final response:
✓ Test update is safe - slot time change is consistently applied across the codebase
The change from 12 to 10 minutes is aligned with the schema and implementation:
- Default value in schema files is already set to 10 minutes
- All test fixtures and sample data use 10 minutes as the standard slot duration
- Time calculations in the codebase are dynamic, using the slotTimeInMinutes property rather than hardcoded values:
- Availability.php:
$duration = $this->slotTimeInMinutes * $appointment->slotCount - SlotList.php: Uses dynamic slot time for interval calculations
- Availability.php:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to slot time duration have been updated consistently
# Search for any hardcoded references to 12-minute slots that might have been missed
rg -A 2 "slotTimeInMinutes.*12|12.*minutes?"
# Search for time slot calculations that might be affected
ast-grep --pattern 'function $_($$) {
$$$
slotTimeInMinutes
$$$
}'
# Look for frontend components that might need updates for time slot display
fd -e js -e vue -e php --exec rg -A 2 "slotTime|timeSlot"
Length of output: 125459
Script:
#!/bin/bash
# Search for slotTimeInMinutes in PHP files
rg -t php "slotTimeInMinutes" -A 2
# Search for slot time configurations in schema files
fd -e json --exec rg -A 2 "slotTimeInMinutes|slot_time"
# Search for slot time calculations in PHP files
ast-grep --pattern 'function $name($params) {
$$$
$var = $_ * slotTimeInMinutes
$$$
}'
Length of output: 57515
zmsentities/schema/availability.json (1)
21-21:
Verify impact of reduced slot time duration
Changing slotTimeInMinutes from 12 to 10 minutes is a breaking change that could affect:
- Existing appointments and their durations
- Frontend scheduling logic
- Backend validation rules
Let's verify the impact:
zmsdb/tests/Zmsdb/ProcessConflictTest.php (2)
56-57: LGTM!
The comment formatting is clean and clearly describes the test's purpose.
120-122: Maintain consistent message formatting and consider using CSS for spacing.
The message content is clear, but has the same HTML entity spacing concerns. Additionally, let's verify that these messages are consistently used across the codebase.
Suggested improvement:
- $this->assertEquals("Konflikt: Eine neue Öffnungszeit überschneidet sich mit einer bestehenden Öffnungszeit.\n" .
- "Bestehende Öffnungszeit:  [30.01.2016 - 22.05.2016, 08:00 - 15:50]\n" .
- "Neue Öffnungszeit:                 [30.01.2016 - 22.05.2016, 08:00 - 13:50]", $conflictList->getFirst()->getAmendment());
+ $this->assertEquals("Konflikt: Eine neue Öffnungszeit überschneidet sich mit einer bestehenden Öffnungszeit.\n" .
+ "Bestehende Öffnungszeit: [30.01.2016 - 22.05.2016, 08:00 - 15:50]\n" .
+ "Neue Öffnungszeit: [30.01.2016 - 22.05.2016, 08:00 - 13:50]", $conflictList->getFirst()->getAmendment());zmsentities/src/Zmsentities/Collection/AvailabilityList.php (1)
189-220: Consider optimization and error handling improvements
While the method is well-documented and functionally correct, consider the following improvements:
- Add error handling for empty lists:
public function getDateTimeRangeFromList(\DateTimeImmutable $selectedDate): array
{
$earliestStartDateTime = null;
$latestEndDateTime = null;
+ if ($this->count() === 0) {
+ throw new \RuntimeException('Cannot calculate date range from empty availability list');
+ }- Consider returning named values for better clarity:
- return [$earliestStartDateTime, $latestEndDateTime];
+ return [
+ 'earliestStart' => $earliestStartDateTime,
+ 'latestEnd' => $latestEndDateTime
+ ];- Optimize DateTime object creation:
- $startDate = (new \DateTimeImmutable())->setTimestamp($availability->startDate)->format('Y-m-d');
- $endDate = (new \DateTimeImmutable())->setTimestamp($availability->endDate)->format('Y-m-d');
+ $startDateTime = \DateTimeImmutable::createFromFormat('U H:i', $availability->startDate . ' ' . $availability->startTime);
+ $endDateTime = \DateTimeImmutable::createFromFormat('U H:i', $availability->endDate . ' ' . $availability->endTime);Let's verify the timezone handling in the codebase:
zmsentities/tests/Zmsentities/ProcessTest.php (1)
494-494: Verify the end time calculation logic.
The end time calculation appears to be based on slotTimeInMinutes * slotCount. Let's verify this change:
- Start time is '18:52'
- The comment indicates "12X2 = 24" minutes
- Adding 24 minutes to 18:52 should indeed result in 19:12
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient context about the test method and the calculation logic.
The end time calculation is correct
The test verifies that:
- Start time is '18:52'
- The comment explicitly states "endTime = slotTimeInMinutes * slotCount 12X2 = 24"
- Adding 24 minutes (12 minutes × 2 slots) to 18:52 correctly results in 19:12
The change from '19:16' to '19:12' aligns with the documented calculation logic in the test comment, making this a valid correction to match the actual business logic of slot-based time calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the slot time calculation logic in the codebase
# Search for slot time definitions and calculations
rg -A 3 "slotTimeInMinutes.*=.*12"
# Search for related availability configurations
ast-grep --pattern 'class Availability {
$$$
slotTimeInMinutes = $_;
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search to understand the slot time and appointment calculation logic
# Search for the test method and its context
rg -B 5 -A 10 "testToConflictListByDay"
# Search for any slot time related configurations
rg -l "slotTime"
# Search for appointment time calculations
rg "endTime.*=.*"
Length of output: 14920
Script:
#!/bin/bash
# Let's examine the specific test method and its context more closely
# Get the full test method implementation
rg -B 10 -A 20 "public function testToConflictListByDay"
# Look for slot time configuration in the test
rg -B 5 -A 5 "slotTimeInMinutes.*=.*12" zmsentities/tests/Zmsentities/ProcessTest.php
Length of output: 3051
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (5)
29-29: Code change improves readability
The return statement is now more concise, enhancing code readability without altering functionality.
44-46: Approved: Loop correctly integrates future availabilities
The loop effectively adds future availabilities to the availabilityList, ensuring they are considered in conflict detection.
68-74: Well-documented method enhances maintainability
Adding the docblock to getAvailabilityList improves code documentation, aiding future developers in understanding the method's purpose and parameters.
48-48: Ensure 'getDateTimeRangeFromList' returns expected values
Verify that getDateTimeRangeFromList reliably returns an array with two \DateTimeImmutable objects. If it returns unexpected values, it could lead to errors in conflict detection.
Run the following script to test the method's output:
✅ Verification successful
Method implementation is safe and returns expected types
The implementation of getDateTimeRangeFromList is correct and safe. It:
- Takes a
\DateTimeImmutableparameter - Always returns an array with two
\DateTimeImmutableobjects - Properly initializes both return values
- Handles edge cases like dates before the selected date
- Uses type-safe DateTime operations throughout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that getDateTimeRangeFromList returns the expected date range
# Navigate to the directory containing the PHP code
cd /path/to/project
# Create a PHP script to test the method
php -r '
require_once "vendor/autoload.php";
$availabilityList = new \BO\Zmsentities\Collection\AvailabilityList();
// Add test data to $availabilityList as needed
list($earliestStartDateTime, $latestEndDateTime) = $availabilityList->getDateTimeRangeFromList(new \DateTimeImmutable());
if ($earliestStartDateTime instanceof \DateTimeImmutable && $latestEndDateTime instanceof \DateTimeImmutable) {
echo "Method returns expected values.\n";
} else {
echo "Method does not return expected values.\n";
}
'
Length of output: 747
Script:
#!/bin/bash
# Let's examine the implementation of getDateTimeRangeFromList method
ast-grep --pattern 'function getDateTimeRangeFromList' -A 20
Length of output: 62
Script:
#!/bin/bash
# Search for any test files that might verify this method
rg -l "getDateTimeRangeFromList" --type-add 'test:*{Test,test}.php' -t test
Length of output: 75
Script:
#!/bin/bash
# Look for the actual implementation and its return type hints
rg "getDateTimeRangeFromList" -A 5
Length of output: 2597
Script:
#!/bin/bash
# Let's examine the full implementation of getDateTimeRangeFromList
rg -p "public function getDateTimeRangeFromList" -A 30 zmsentities/src/Zmsentities/Collection/AvailabilityList.php
Length of output: 1645
83-83: Verify 'startDate' parameter compatibility with the API endpoint
Ensure that the API endpoint accepts the 'startDate' parameter and correctly filters availabilities from the specified date onward.
Run the following script to confirm the API support:
✅ Verification successful
Based on the search results, I can verify that the 'startDate' parameter is well-supported in the API. Here's what I found:
- The API endpoint
/scope/{id}/availability/accepts the 'startDate' parameter as shown in the routing file and multiple API controller implementations. - The parameter is used consistently throughout the codebase to filter availabilities:
- In
zmsapi/src/Zmsapi/AvailabilityListByScope.php, the parameter is validated and processed - In
zmsdb/src/Zmsdb/Query/Availability.php, there's a dedicated methodaddConditionTimeframe()that handles the filtering - Multiple test files demonstrate the parameter's usage and expected behavior
- In
The 'startDate' parameter is properly implemented and supported by the API endpoint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test if the 'startDate' parameter is supported by the availability API endpoint
# Replace <scope_id> with a valid scope ID and ensure authentication if required
SCOPE_ID=<scope_id>
START_DATE=$(date +"%Y-%m-%d")
# Make an API request with 'startDate' parameter
curl -s -X GET "https://api.example.com/scope/$SCOPE_ID/availability/?resolveReferences=0&startDate=$START_DATE" -H "Authorization: Bearer <token>" -o response.json -w "%{http_code}"
# Check if the response contains availabilities starting from the specified date
grep -q "$START_DATE" response.json && echo "startDate parameter is supported." || echo "startDate parameter is not supported or not functioning as expected."
Length of output: 208
Script:
#!/bin/bash
# Search for API endpoint definitions or configurations related to availability
rg -l "availability" --type php
# Search for startDate parameter usage in API-related files
rg "startDate" --type php -C 2
# Look for any API endpoint configurations or route definitions
ast-grep --pattern 'function $FUNC($$$) {
$$$
startDate
$$$
}'
Length of output: 107167
zmsapi/src/Zmsapi/AvailabilityAdd.php (1)
118-121: Handle potential null $oldEntity when updating
When updating an entity, if $oldEntity is null, the code currently does not handle this scenario, which could lead to unexpected behavior. Ensure that $oldEntity is valid before proceeding with the update.
Run the following script to verify if $oldEntity could be null and how often this scenario occurs:
zmsapi/src/Zmsapi/AvailabilityUpdate.php (2)
125-133: Handle cases when $entity->id is not set correctly
In writeEntityUpdate, when $entity->id is not set, the code proceeds to write a new entity. If the intent is to only update existing entities, allowing entities without an id to be written may lead to unintended data being added. Please verify whether creating new entities in this method is appropriate.
Please confirm if writing new entities when $entity->id is not set is the desired behavior. If not, consider throwing an exception or handling it accordingly.
4-4: LGTM on the added copyright
The addition of the @copyright annotation is appropriate and follows the documentation standards.
zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (1)
301-366: Well-implemented test case in testInvalidEndTime
The testInvalidEndTime() method correctly tests the scenario where the end time is before the start time and expects an AvailabilityUpdateFailed exception. This improves the robustness of the availability validation.
zmsadmin/js/page/availabilityDay/index.js (2)
54-56: Approved: Implementation of handleScrollToBottom is correct
The handleScrollToBottom method is implemented correctly to scroll to the bottom of the page.
842-848:
Conditional rendering of SaveBar may not return a value
The renderSaveBar method may not return anything if this.state.lastSave is falsy, which can cause issues in rendering.
Apply this diff to ensure the method returns null when there is nothing to render:
if (this.state.lastSave) {
return (
<SaveBar
lastSave={this.state.lastSave}
success={this.state.saveSuccess}
setSuccessRef={this.setSuccessRef}
/>
)
+ } else {
+ return null;
}Likely invalid or redundant comment.
zmsentities/tests/Zmsentities/AvailabilityTest.php (4)
182-182: Verify the updated expected value in testGetAvailableSecondsPerDay
The expected value in the assertion has changed to 6534000. Please ensure that this new value accurately reflects the underlying calculations in getAvailableSecondsPerDay() after recent changes.
199-199: Confirm the updated expected value in testGetAvailableSecondsOnDateTime
The assertion now expects 6534000. Verify that this updated value correctly corresponds with the changes in getAvailableSecondsOnDateTime().
501-501: Ensure correctness of the updated workstation count
In testWithCalculatedSlots, the expected public workstation count has been updated to 99. Please confirm that this change aligns with the modifications in the withCalculatedSlots() method and reflects the correct calculation.
510-512: Confirm the updated slot count and timing in testGetSlotList
The expected slot count has changed to 33, and the expected time for the second slot is now '10:10'. Please verify that these updates are consistent with the logic changes in getSlotList().
| @@ -9,5 +9,5 @@ class AvailabilityUpdateFailed extends \Exception | |||
| { | |||
| protected $code = 400; | |||
|
|
|||
| protected $message = 'Could not insert or update availablity'; | |||
| protected $message = 'Could not update availablity.'; | |||
There was a problem hiding this comment.
Fix typo in error message
There's a typo in the error message: "availablity" should be "availability".
- protected $message = 'Could not update availablity.';
+ protected $message = 'Could not update availability.';📝 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.
| protected $message = 'Could not update availablity.'; | |
| protected $message = 'Could not update availability.'; |
| { | ||
| protected $code = 400; | ||
|
|
||
| protected $message = 'Could not create availablity.'; |
There was a problem hiding this comment.
Fix typo in error message and make it more descriptive
There's a typo in "availablity" and the message could be more informative about the failure.
- protected $message = 'Could not create availablity.';
+ protected $message = 'Failed to create availability. Please ensure there are no conflicts with existing entries.';📝 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.
| protected $message = 'Could not create availablity.'; | |
| protected $message = 'Failed to create availability. Please ensure there are no conflicts with existing entries.'; |
| @@ -11,6 +11,7 @@ const PageLayout = (props) => { | |||
| {props.timeTable} | |||
| {props.saveBar} | |||
| {props.accordion} | |||
| {props.saveBar} | |||
There was a problem hiding this comment.
Remove duplicate saveBar rendering
The saveBar component is being rendered twice in the layout, which would cause duplicate UI elements to appear. This appears to be unintentional and should be removed.
Apply this diff to fix the issue:
{props.accordion}
- {props.saveBar}📝 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.
| {props.saveBar} | |
| {props.accordion} |
| if (!isVisible) return null | ||
|
|
||
| return ( | ||
| <div ref={props.setSuccessRef} className="message message--success"> | ||
| Öffnungszeiten gespeichert, {formatDate(props.lastSave)} | ||
| <div | ||
| ref={props.setSuccessRef} | ||
| className={`message ${props.success ? 'message--success' : 'message--error'}`} | ||
| > | ||
| {props.success | ||
| ? <b><i class="fas fa-check-circle" aria-hidden="true"></i> Öffnungszeiten gespeichert, {formatDate(props.lastSave)}</b> | ||
| : <b><i class="fas fa-times-circle" aria-hidden="true"></i> Fehler beim Speichern der Öffnungszeiten. Bitte versuchen Sie es erneut.</b>} | ||
| </div> |
There was a problem hiding this comment.
Fix JSX syntax and enhance accessibility.
Several improvements needed in the render logic:
- Use
classNameinstead ofclassfor the Font Awesome icon - Add proper aria-labels to icons
- Consider adding action guidance to the error message
- {props.success
- ? <b><i class="fas fa-check-circle" aria-hidden="true"></i> Öffnungszeiten gespeichert, {formatDate(props.lastSave)}</b>
- : <b><i class="fas fa-times-circle" aria-hidden="true"></i> Fehler beim Speichern der Öffnungszeiten. Bitte versuchen Sie es erneut.</b>}
+ {props.success
+ ? <b><i className="fas fa-check-circle" aria-hidden="true" aria-label="Erfolg"></i> Öffnungszeiten gespeichert, {formatDate(props.lastSave)}</b>
+ : <b>
+ <i className="fas fa-times-circle" aria-hidden="true" aria-label="Fehler"></i>
+ Fehler beim Speichern der Öffnungszeiten. Bitte überprüfen Sie Ihre Eingaben und versuchen Sie es erneut.
+ </b>}📝 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.
| if (!isVisible) return null | |
| return ( | |
| <div ref={props.setSuccessRef} className="message message--success"> | |
| Öffnungszeiten gespeichert, {formatDate(props.lastSave)} | |
| <div | |
| ref={props.setSuccessRef} | |
| className={`message ${props.success ? 'message--success' : 'message--error'}`} | |
| > | |
| {props.success | |
| ? <b><i class="fas fa-check-circle" aria-hidden="true"></i> Öffnungszeiten gespeichert, {formatDate(props.lastSave)}</b> | |
| : <b><i class="fas fa-times-circle" aria-hidden="true"></i> Fehler beim Speichern der Öffnungszeiten. Bitte versuchen Sie es erneut.</b>} | |
| </div> | |
| if (!isVisible) return null | |
| return ( | |
| <div | |
| ref={props.setSuccessRef} | |
| className={`message ${props.success ? 'message--success' : 'message--error'}`} | |
| > | |
| {props.success | |
| ? <b><i className="fas fa-check-circle" aria-hidden="true" aria-label="Erfolg"></i> Öffnungszeiten gespeichert, {formatDate(props.lastSave)}</b> | |
| : <b> | |
| <i className="fas fa-times-circle" aria-hidden="true" aria-label="Fehler"></i> | |
| Fehler beim Speichern der Öffnungszeiten. Bitte überprüfen Sie Ihre Eingaben und versuchen Sie es erneut. | |
| </b>} | |
| </div> |
| conflictDatesByMessage.map((item, index) => ( | ||
| <div key={index} style={{ marginBottom: '1rem' }}> | ||
| {/* Convert newlines in the message to <br /> tags */} | ||
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> | ||
| </div> | ||
| )) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Security: Replace dangerouslySetInnerHTML with safe message rendering
The current implementation uses dangerouslySetInnerHTML to convert newlines to <br /> tags, which poses a potential XSS security risk. Since we're only handling newlines, we can achieve the same result safely using array mapping.
- <div key={index} style={{ marginBottom: '1rem' }}>
- {/* Convert newlines in the message to <br /> tags */}
- <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} />
- </div>
+ <div key={index} style={{ marginBottom: '1rem' }}>
+ {item.message.split('\n').map((line, i) => (
+ <React.Fragment key={i}>
+ {line}
+ {i < item.message.split('\n').length - 1 && <br />}
+ </React.Fragment>
+ ))}
+ </div>This approach:
- Safely splits the message on newlines
- Renders each line with proper
<br />tags - Eliminates the XSS vulnerability
- Maintains the same visual output
📝 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.
| conflictDatesByMessage.map((item, index) => ( | |
| <div key={index} style={{ marginBottom: '1rem' }}> | |
| {/* Convert newlines in the message to <br /> tags */} | |
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> | |
| </div> | |
| )) | |
| ); | |
| }; | |
| conflictDatesByMessage.map((item, index) => ( | |
| <div key={index} style={{ marginBottom: '1rem' }}> | |
| {item.message.split('\n').map((line, i) => ( | |
| <React.Fragment key={i}> | |
| {line} | |
| {i < item.message.split('\n').length - 1 && <br />} | |
| </React.Fragment> | |
| ))} | |
| </div> | |
| )) | |
| ); | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[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)
| $startTimestamp = $startDate->getTimestamp(); | ||
| $endTimestamp = $endDate->getTimestamp(); | ||
|
|
||
| $slotAmount = ($endTimestamp - $startTimestamp) / 60 % $slotTime; |
There was a problem hiding this comment.
Ensure correct operator precedence in slot amount calculation
In line 562, the calculation:
$slotAmount = ($endTimestamp - $startTimestamp) / 60 % $slotTime;Due to operator precedence, the modulo operation % is performed before division /. To ensure the intended calculation, add parentheses around the division operation.
Apply this diff:
-$slotAmount = ($endTimestamp - $startTimestamp) / 60 % $slotTime;
+$slotAmount = (($endTimestamp - $startTimestamp) / 60) % $slotTime;|
|
||
| $existingDateRange = $this->getStartDateTime()->format('d.m.Y') . ' - ' . $this->getEndDateTime()->format('d.m.Y'); | ||
| $newDateRange = $availability->getStartDateTime()->format('d.m.Y') . ' - ' . $availability->getEndDateTime()->format('d.m.Y'); | ||
|
|
||
| $existingTimeRange = $this->getStartDateTime()->format('H:i') . ' - ' . $this->getEndDateTime()->format('H:i'); | ||
| $newTimeRange = $availability->getStartDateTime()->format('H:i') . ' - ' . $availability->getEndDateTime()->format('H:i'); | ||
|
|
||
| $isEqual = ($this->getStartDateTime()->getSecondsOfDay() == $availability->getStartDateTime()->getSecondsOfDay() && | ||
| $this->getEndDateTime()->getSecondsOfDay() == $availability->getEndDateTime()->getSecondsOfDay()); | ||
| if ($isEqual) { | ||
| $process = clone $processTemplate; | ||
| $process->amendment = "Konflikt: Zwei Öffnungszeiten sind gleich.\n" | ||
| . "Bestehende Öffnungszeit:  [$newDateRange, $newTimeRange]\n" | ||
| . "Neue Öffnungszeit:                 [$existingDateRange, $existingTimeRange]"; | ||
| $process->getFirstAppointment()->date = $availability | ||
| ->getStartDateTime() | ||
| ->modify($currentDate->format("Y-m-d")) | ||
| ->getTimestamp(); | ||
| $processList->addEntity($process); | ||
| } elseif ($isEqual) { | ||
| } | ||
| elseif ($availability->getStartDateTime()->getSecondsOfDay() < $this->getEndDateTime()->getSecondsOfDay() && | ||
| $this->getStartDateTime()->getSecondsOfDay() < $availability->getEndDateTime()->getSecondsOfDay()) { | ||
| $process = clone $processTemplate; | ||
| $process->amendment = "Zwei Öffnungszeiten sind gleich."; | ||
| $process->amendment = "Konflikt: Eine neue Öffnungszeit überschneidet sich mit einer bestehenden Öffnungszeit.\n" | ||
| . "Bestehende Öffnungszeit:  [$newDateRange, $newTimeRange]\n" | ||
| . "Neue Öffnungszeit:                 [$existingDateRange, $existingTimeRange]"; | ||
| $process->getFirstAppointment()->date = $availability | ||
| ->getStartDateTime() | ||
| ->modify($currentDate->format("Y-m-d")) | ||
| ->getTimestamp(); | ||
| $processList->addEntity($process); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor getTimeOverlaps for clarity and efficiency
The getTimeOverlaps method contains complex logic for detecting time overlaps. Consider simplifying the overlap checks by using a dedicated function or utilizing existing date/time comparison functions to make the code more readable and maintainable.
Example:
if ($this->timeIntervalsOverlap(
$this->getStartDateTime()->getSecondsOfDay(),
$this->getEndDateTime()->getSecondsOfDay(),
$availability->getStartDateTime()->getSecondsOfDay(),
$availability->getEndDateTime()->getSecondsOfDay()
)) {
// Handle overlap
}
private function timeIntervalsOverlap($start1, $end1, $start2, $end2)
{
return $start1 < $end2 && $start2 < $end1;
}| $isOrigin = ($kind && $kind === 'origin'); | ||
|
|
||
| // Validate that end date is after the selected date | ||
| if (!$isOrigin && $selectedDate->getTimestamp() > $today->getTimestamp() && $endDate < $selectedDate->setTime(0, 0)) { |
There was a problem hiding this comment.
Avoid mutating $selectedDate when setting time
In line 522, $selectedDate->setTime(0, 0) modifies $selectedDate. Clone the object before modification to prevent unintended side effects.
Apply this diff:
-if (!$isOrigin && $selectedDate->getTimestamp() > $today->getTimestamp() && $endDate < $selectedDate->setTime(0, 0)) {
+if (!$isOrigin && $selectedDate->getTimestamp() > $today->getTimestamp() && $endDate < (clone $selectedDate)->setTime(0, 0)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!$isOrigin && $selectedDate->getTimestamp() > $today->getTimestamp() && $endDate < $selectedDate->setTime(0, 0)) { | |
| if (!$isOrigin && $selectedDate->getTimestamp() > $today->getTimestamp() && $endDate < (clone $selectedDate)->setTime(0, 0)) { |
| !$isFuture && | ||
| $selectedDate->getTimestamp() > $today->getTimestamp() && | ||
| $startTime->getTimestamp() > $selectedDate->setTime(0, 0)->getTimestamp() |
There was a problem hiding this comment.
Avoid mutating $selectedDate when setting time
In line 467, $selectedDate->setTime(0, 0) modifies the original $selectedDate object. This can cause unexpected behavior if $selectedDate is used later in the code. Clone the DateTime object before setting the time to avoid side effects.
Apply this diff to clone $selectedDate:
-if (
!$isFuture &&
$selectedDate->getTimestamp() > $today->getTimestamp() &&
$startTime->getTimestamp() > $selectedDate->setTime(0, 0)->getTimestamp()
) {
+if (
!$isFuture &&
$selectedDate->getTimestamp() > $today->getTimestamp() &&
$startTime->getTimestamp() > (clone $selectedDate)->setTime(0, 0)->getTimestamp()
) {📝 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.
| !$isFuture && | |
| $selectedDate->getTimestamp() > $today->getTimestamp() && | |
| $startTime->getTimestamp() > $selectedDate->setTime(0, 0)->getTimestamp() | |
| !$isFuture && | |
| $selectedDate->getTimestamp() > $today->getTimestamp() && | |
| $startTime->getTimestamp() > (clone $selectedDate)->setTime(0, 0)->getTimestamp() |
| $startTime = $startDate->setTime(0, 0); | ||
| $startHour = ($startDate->format('H')); |
There was a problem hiding this comment.
Avoid mutating original DateTime objects by cloning before modification
In line 457, $startTime = $startDate->setTime(0, 0); modifies the original $startDate object. This could lead to unintended side effects if $startDate is used elsewhere in the code. To prevent this, clone the DateTime object before modifying it.
Apply this diff to clone the DateTime object before modification:
-$startTime = $startDate->setTime(0, 0);
+$startTime = (clone $startDate)->setTime(0, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $startTime = $startDate->setTime(0, 0); | |
| $startHour = ($startDate->format('H')); | |
| $startTime = (clone $startDate)->setTime(0, 0); | |
| $startHour = ($startDate->format('H')); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
zmsadmin/js/page/availabilityDay/index.js (1)
77-77: Clean up debug console.logsRemove or replace debug console.logs with meaningful logging messages.
Apply this diff:
- console.log("here5") + console.log("Initiating conflict list update after publish") - console.log("here4") + console.log("Updating conflict list after deletion") - console.log("here3") + console.log("Updating conflict list after exclusion creation") - console.log("here2") + console.log("Updating conflict list after future edit") - console.log("here") + console.log("Updating conflict list after data change")Also applies to: 270-270, 403-403, 445-445, 692-692
zmsapi/src/Zmsapi/AvailabilityAdd.php (1)
117-134: Add parameter validation and improve error handlingThe method should validate the resolveReferences parameter and provide more specific error messages.
Apply this diff:
protected function writeEntityUpdate($entity, $resolveReferences): Entity { + if (!is_numeric($resolveReferences)) { + throw new BadRequestException('Invalid resolveReferences parameter'); + } $repository = new AvailabilityRepository(); $updatedEntity = null; if ($entity->id) { $oldEntity = $repository->readEntity($entity->id); if ($oldEntity && $oldEntity->hasId()) { $this->writeSpontaneousEntity($oldEntity); $updatedEntity = $repository->updateEntity($entity->id, $entity, $resolveReferences); } } else { $updatedEntity = $repository->writeEntity($entity, 2); } if (!$updatedEntity) { - throw new AvailabilityAddFailed(); + throw new AvailabilityAddFailed('Failed to update or create availability entity'); } return $updatedEntity; }
📜 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 (3)
zmsadmin/js/page/availabilityDay/form/formButtons.js(2 hunks)zmsadmin/js/page/availabilityDay/index.js(12 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsadmin/js/page/availabilityDay/form/formButtons.js
🧰 Additional context used
🪛 Biome (1.9.4)
zmsadmin/js/page/availabilityDay/index.js
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 126-127: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 204-205: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 533-533: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (11)
zmsadmin/js/page/availabilityDay/index.js (5)
54-56: LGTM: Utility method for consistent scroll behavior
The handleScrollToBottom method provides a centralized way to handle scrolling behavior, which is good for maintainability.
856-862: LGTM: SaveBar component enhancement
The addition of the success prop to SaveBar improves user feedback for save operations.
533-533:
Replace isNaN with Number.isNaN
The global isNaN function performs type coercion which can lead to unexpected results.
Apply this diff:
- const isValidTimestamp = (timestamp) => !isNaN(timestamp) && moment.unix(timestamp).isValid();
+ const isValidTimestamp = (timestamp) => !Number.isNaN(Number(timestamp)) && moment.unix(timestamp).isValid();🧰 Tools
🪛 Biome (1.9.4)
[error] 533-533: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
496-527:
Uncomment the Promise rejection
The commented out reject prevents proper error handling in the Promise chain. This could lead to silent failures.
Apply this diff:
if (list.length > 0) {
console.warn("Validation failed with errors:", list);
this.errorElement?.scrollIntoView();
- //reject(new Error("Validation failed")); // Reject with an error object
+ reject(list); // Reject with the actual validation errors
} else {Likely invalid or redundant comment.
568-569:
Avoid direct state mutations
Directly modifying the selectedAvailability object violates React's immutability principles.
Apply this diff:
- selectedAvailability.startTime = moment(selectedAvailability.startTime, ['HH:mm:ss', 'HH:mm']).format('HH:mm');
- selectedAvailability.endTime = moment(selectedAvailability.endTime, ['HH:mm:ss', 'HH:mm']).format('HH:mm');
+ const formattedAvailability = {
+ ...selectedAvailability,
+ startTime: moment(selectedAvailability.startTime, ['HH:mm:ss', 'HH:mm']).format('HH:mm'),
+ endTime: moment(selectedAvailability.endTime, ['HH:mm:ss', 'HH:mm']).format('HH:mm'),
+ };Likely invalid or redundant comment.
zmsapi/src/Zmsapi/AvailabilityAdd.php (6)
23-24: LGTM: Exception import aligns with new error handling
The addition of the AvailabilityAddFailed exception import is appropriate for the enhanced error handling implemented in this class.
Line range hint 136-149: LGTM: No changes in this method
The method remains unchanged and appears to be functioning as intended.
60-62:
Validate scope data before usage
The scope data access needs validation to prevent undefined index errors.
Apply this diff:
-$scopeData = $input['availabilityList'][0]['scope'];
+$scopeData = $input['availabilityList'][0]['scope'] ?? null;
+if ($scopeData === null) {
+ throw new BadRequestException('Missing scope data in availabilityList');
+}
$scope = new \BO\Zmsentities\Scope($scopeData);73-89:
Validation results should be accumulated
The validation results should be collected across all iterations to catch all potential conflicts.
Apply this diff:
+$allValidations = [];
foreach ($newCollection as $newAvailability) {
$startDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->startDate);
$endDate = (new \DateTimeImmutable())->setTimestamp($newAvailability->endDate);
+ if (!isset($input['selectedDate'])) {
+ throw new BadRequestException('Missing selectedDate in input');
+ }
$selectedDate = \DateTimeImmutable::createFromFormat('Y-m-d H:i:s', $input['selectedDate'] . ' 00:00:00');
$startDateTime = new \DateTimeImmutable("{$startDate->format('Y-m-d')} {$newAvailability->startTime}");
$endDateTime = new \DateTimeImmutable("{$endDate->format('Y-m-d')} {$newAvailability->endTime}");
$validation = $mergedCollection->validateInputs(
$startDateTime,
$endDateTime,
$selectedDate,
$newAvailability->kind ?? 'default'
);
+ if (!empty($validation)) {
+ $allValidations = array_merge($allValidations, $validation);
+ }
$mergedCollection->addEntity($newAvailability);
}91-94:
Update validation check to use accumulated results
The validation check should use the accumulated validation results.
Apply this diff:
-if (count($validation) > 0) {
+if (!empty($allValidations)) {
//error_log(json_encode($validation));
throw new AvailabilityAddFailed();
}41-46:
Input validation needs improvement
The input validation is incomplete and could lead to runtime errors.
Apply this diff to add proper validation:
$resolveReferences = Validator::param('resolveReferences')->isNumber()->setDefault(2)->getValue();
$input = Validator::input()->isJson()->assertValid()->getValue();
-if (!$input || count($input) === 0) {
+if (!$input || !isset($input['availabilityList']) || !is_array($input['availabilityList'])) {
throw new BadRequestException();
}
+if (empty($input['availabilityList'])) {
+ throw new BadRequestException('availabilityList cannot be empty');
+}Likely invalid or redundant comment.
| foreach ($input['availabilityList'] as $item) { | ||
| error_log(json_encode($item['kind'])); | ||
| $entity = new Entity($item); | ||
| error_log(json_encode($entity->kind)); | ||
| $entity->testValid(); | ||
| $updatedEntity = $this->writeEntityUpdate($entity); | ||
| AvailabilitySlotsUpdate::writeCalculatedSlots($updatedEntity, true); | ||
| $collection->addEntity($updatedEntity); | ||
| $newCollection->addEntity($entity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug logging and add kind validation
The debug logging statements should be removed, and kind validation should be added.
Apply this diff:
- error_log(json_encode($item['kind']));
$entity = new Entity($item);
- error_log(json_encode($entity->kind));
+ if (!isset($item['kind'])) {
+ throw new BadRequestException('Missing kind in availability item');
+ }
$entity->testValid();
$newCollection->addEntity($entity);📝 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.
| foreach ($input['availabilityList'] as $item) { | |
| error_log(json_encode($item['kind'])); | |
| $entity = new Entity($item); | |
| error_log(json_encode($entity->kind)); | |
| $entity->testValid(); | |
| $updatedEntity = $this->writeEntityUpdate($entity); | |
| AvailabilitySlotsUpdate::writeCalculatedSlots($updatedEntity, true); | |
| $collection->addEntity($updatedEntity); | |
| $newCollection->addEntity($entity); | |
| foreach ($input['availabilityList'] as $item) { | |
| $entity = new Entity($item); | |
| if (!isset($item['kind'])) { | |
| throw new BadRequestException('Missing kind in availability item'); | |
| } | |
| $entity->testValid(); | |
| $newCollection->addEntity($entity); |
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-1891 ZMS-3415
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores