Bugfix zms 3253 3466 3415 1891 validation opening hours of the same appointment type must not overlap #807
Conversation
…orm error messaging
This reverts commit ce5d795.
WalkthroughThis pull request introduces comprehensive changes across multiple files in the zmsadmin and zmsapi projects, focusing on improving availability management, error handling, and validation processes. The modifications span frontend JavaScript components, backend PHP classes, and test suites, with a primary emphasis on enhancing conflict detection, input validation, and user feedback mechanisms for availability-related operations. Changes
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 after adding new availabilities saving with Alle Änderungen aktivieren and then adding more and saving a second time with Alle Änderungen aktivieren we get |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink I've found the root cause of the issue. The // In the AJAX success callback, change this:
this.setState({
lastSave: new Date().getTime(),
saveSuccess: true,
}, () => {
this.successElement.scrollIntoView();
});
// To this:
this.setState({
lastSave: new Date().getTime(),
saveSuccess: true,
}, () => {
// Attempt to scroll immediately after state update, before the 6-second timer expires
if (this.successElement) {
this.successElement.scrollIntoView();
} else {
// Fallback to bottom scroll if the element isn't available
this.handleScrollToBottom();
}
});This fix:
🔗 Analysis chain@ThomasAFink The error occurs because // In the success callback, change this:
this.setState({
lastSave: new Date().getTime(),
saveSuccess: true,
}, () => {
this.successElement.scrollIntoView(); // Can throw error if null
});
// To this:
this.setState({
lastSave: new Date().getTime(),
saveSuccess: true,
}, () => {
if (this.successElement) {
this.successElement.scrollIntoView();
}
});This will prevent the TypeError by checking if the element exists before trying to scroll to it. @ThomasAFink I've found that this issue occurs in multiple places in the code. The // 1. First AJAX success callback
this.setState({
lastSave: new Date().getTime(),
saveSuccess: true,
}, () => {
if (this.successElement) {
this.successElement.scrollIntoView();
}
});
// 2. Second AJAX success callback (same fix needed here)
this.setState({
lastSave: new Date().getTime(),
saveSuccess: true,
}, () => {
if (this.successElement) {
this.successElement.scrollIntoView();
}
});This will prevent the TypeError in both cases. The error occurs because the ref might become null during state updates or component re-renders. Let me check how the ref is attached to see if there might be additional improvements needed. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 3105 Script: Length of output: 5334 ✏️ Learnings added
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (34)
zmsapi/tests/Zmsapi/AvailabilityAddTest.php (1)
16-45: Caution with time-based tests
Usingtime() + (X * 24 * 60 * 60)can lead to date-based flakiness if tests run near midnight or during DST transitions. Consider using a more robust date/time approach (likeDateTimeImmutablewith fixed offsets).zmsapi/src/Zmsapi/AvailabilityAdd.php (4)
42-54: Input Validation
Good checks foravailabilityList,scope, andselectedDate. Consider centralizing validation logic in a helper to avoid duplication if used elsewhere.
96-98: Exception with minimal context
When multiple validations fail, the exception provides no detail about the conflicts. Offering more descriptive error information could improve debugging.
126-130: Entity Update & Slot Calculation
Writing entities one by one followed by recalculation is functional. If usage scales, a batched approach may improve performance.
174-208: Nested Overlap Checks
Comparing every pair in a nested loop is fine for small sets but can be costly for large input. Consider interval trees or segment-based checks for higher performance.zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (1)
98-163: Detailed Conflict Logging
While error logging is helpful, ensure that production logs only capture necessary info. Consider gatingerror_logcalls behind a debug flag.zmsapi/src/Zmsapi/AvailabilityUpdate.php (9)
43-44: Consider adding a descriptive exception message.
It might help debugging to provide more context (e.g., “Empty request input”).- throw new BadRequestException(); + throw new BadRequestException("Empty or invalid request body.");
71-73: Scope creation from a single item may be limiting if multiple scopes are mixed.
If mixing is not allowed, ensure a test or additional checks are in place.
77-77: Magic number '1' for readAvailabilityListByScope.
Clarify the meaning of this argument or replace with a named constant for clarity.
103-105: Throwing an exception without a descriptive message can hamper troubleshooting.- throw new AvailabilityUpdateFailed(); + throw new AvailabilityUpdateFailed("Validation failure: see 'validations' details.");
148-149: Initializing $updatedEntity as null is harmless but could be omitted.- $updatedEntity = null;
150-158: Reading and updating existing entities is handled properly, but consider consistency with $resolveReferences.
When reading old entity, you might want to pass the same $resolveReferences for uniform behavior.- $oldEntity = $repository->readEntity($entity->id); + $oldEntity = $repository->readEntity($entity->id, $resolveReferences);
159-160: Using the hard-coded “2” might ignore $resolveReferences for new entities.
Consider passing the user-specified $resolveReferences.- $updatedEntity = $repository->writeEntity($entity, 2); + $updatedEntity = $repository->writeEntity($entity, $resolveReferences);
162-165: Exception thrown without a descriptive message.
191-215: Nested O(n^2) loops for conflict checks could become expensive.
Consider an interval-based approach or sorting for more efficiency if typical usage scales.zmsadmin/js/page/availabilityDay/index.js (8)
54-56: Ensure Consistent Naming VerbosityThe new
handleScrollToBottom()method is straightforward. However, consider clarifying the method name to better describe its usage context, such asscrollToPageBottom(), especially if future scrolling logic might include offset handling or scrolling to specific elements.
108-111: Confirm Dialog Localization and AccessibilityUsing a
confirm()dialog is acceptable, though it might not be ideal for larger apps. Consider internationalization or a custom modal for improved user experience, especially for accessibility and multi-language support.
114-134: Address Safe-Delete Concern & Optional Chaining
- The usage of
delete sendAvailability.tempId;(lines 125-126) can degrade performance in some engines:-delete sendAvailability.tempId; +sendAvailability.tempId = undefined;- On line 118, static analysis suggests using optional chaining on
availability.tempId.includes(...). For a small performance or readability win, you can do:though functionally both approaches are similar here.(availability.tempId?.includes('__temp__'))🧰 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)
144-145: Consider Abstracting AJAX LogicLines 144-145 set up JSON data for a POST request. If multiple methods in this file or other files do the same, consider extracting into a helper function for maintainability and consistency.
194-195: Dialog Confirmation ConsiderationAnother
confirm()usage. The same recommendation applies regarding potential localization and a custom modal as in lines 108-111.
198-214: Repeated Temporary ID DeletionSimilar to the earlier mention, removing
tempIdusingdeleteat lines 203-204 can be replaced with setting it toundefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 203-204: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Line range hint
717-796: Segmented Logic for Future/Exclusion/OriginThe specialized logic in
handleExclusionChanges(),handleFutureChanges(), andhandleOriginChanges()is quite extensive. Consider extracting these into helper functions or a dedicated class/file to keep the main component more readable and maintainable.
Line range hint
831-855: Leverage Collapsible Error DetailsProviding both
errorListandconflictListto theAccordionLayoutis great for user feedback. If the lists grow large, consider collapsible or paginated error details, so the user isn’t overwhelmed.zmsapi/src/Zmsapi/Exception/Availability/AvailabilityAddFailed.php (1)
5-7: Enhance class documentation.The current documentation is a placeholder. Consider adding more detailed information about when this exception is thrown and what it indicates.
/** - * example class to generate an exception + * Exception thrown when an attempt to create availability fails due to conflicts + * with existing entries. This typically occurs when there are overlapping + * time slots for the same appointment type. */zmsadmin/js/page/availabilityDay/saveBar.js (1)
24-30: Enhance accessibility for screen readers.While the implementation is good, we could improve the accessibility by adding more ARIA attributes.
<div ref={props.setSuccessRef} - className={`message ${props.success ? 'message--success' : 'message--error'}`} + className={`message ${props.success ? 'message--success' : 'message--error'}`} + role="status" + aria-live="polite" >zmsadmin/js/page/availabilityDay/form/index.js (2)
42-44: Simplify the hasConflicts expression.The boolean expression can be simplified for better readability.
- const hasConflicts = (( - this.props.conflictList.itemList && - Object.keys(this.props.conflictList.itemList).length - )) ? true : false + const hasConflicts = !!( + this.props.conflictList.itemList && + Object.keys(this.props.conflictList.itemList).length + )🧰 Tools
🪛 Biome (1.9.4)
[error] 41-44: 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)
46-50: Simplify the hasErrors expression.Similar to hasConflicts, this boolean expression can be simplified.
- const hasErrors = ( - ( - this.props.errorList && - Object.keys(this.props.errorList).length - )) ? true : false + const hasErrors = !!( + this.props.errorList && + Object.keys(this.props.errorList).length + )🧰 Tools
🪛 Biome (1.9.4)
[error] 46-50: 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/layouts/accordion.js (1)
160-165: Simplify boolean expressions and improve error handling.The error handling logic looks good, but the boolean expressions can be simplified.
Apply this diff to simplify the expressions:
- hasConflicts={Object.keys(this.props.conflictList.itemList).length ? true : false} + hasConflicts={Boolean(Object.keys(this.props.conflictList.itemList).length)} hasErrors={Object.values(this.props.errorList).some(error => { const hasPastTimeError = error.itemList?.flat(2) .some(item => item?.type === 'endTimePast'); return !hasPastTimeError; })}🧰 Tools
🪛 Biome (1.9.4)
[error] 160-160: 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/form/validate.js (2)
29-29: Simplify boolean expression.The boolean expression can be simplified.
Apply this diff:
- let valid = (0 < errorList.itemList.length) ? false : true; + let valid = !errorList.itemList.length;🧰 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)
212-219: Improve error message clarity.The error messages for time validation could be more specific.
Apply this diff to enhance the error messages:
- message: 'Die Endzeit darf nicht vor der Startzeit liegen.' + message: 'Die Endzeit muss nach der Startzeit liegen.' - message: 'Das Enddatum darf nicht vor dem Startdatum liegen.' + message: 'Das Enddatum muss nach dem Startdatum liegen.'zmsentities/src/Zmsentities/Availability.php (3)
453-483: Consider extracting hardcoded German error messagesThe error messages are hardcoded in German. Consider extracting them to a translation/localization system for better maintainability and internationalization support.
475-480: Simplify start time validationThe condition
($startHour == 0 && $startMinute == 0) || ($endHour == 0 && $endMinute == 0)could be simplified by extracting a helper method to check for midnight time.+private function isMidnight(int $hour, int $minute): bool { + return $hour === 0 && $minute === 0; +} + if (($startHour == 0 && $startMinute == 0) || ($endHour == 0 && $endMinute == 0)) { - $errorList[] = [ - 'type' => 'startOfDay', - 'message' => 'Die Uhrzeit darf nicht "00:00" sein.' - ]; + $errorList[] = $this->createError('startOfDay', 'Die Uhrzeit darf nicht "00:00" sein.'); }
559-563: Fix inconsistent error message formattingThe error message formatting in validateSlotTime() is inconsistent with other validation methods. The closing bracket should be on a new line.
- $errorList[] = [ - 'type' => 'slotTime', - 'message' => 'Die Slot-Zeit darf nicht 0 sein.']; + $errorList[] = [ + 'type' => 'slotTime', + 'message' => 'Die Slot-Zeit darf nicht 0 sein.' + ];zmsadmin/js/page/availabilityDay/form/formButtons.js (1)
33-33: Add JSDoc comments for the new props.While the PropType is correctly added, documentation would help explain the validation scenarios these props handle.
Add JSDoc comments:
FormButtons.propTypes = { + /** Indicates if form has validation errors */ hasErrors: PropTypes.bool, + /** Indicates if slot count validation failed */ + hasSlotCountError: PropTypes.bool, // ... rest of props }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
zmsadmin/js/page/availabilityDay/form/conflicts.js(1 hunks)zmsadmin/js/page/availabilityDay/form/content.js(5 hunks)zmsadmin/js/page/availabilityDay/form/datepicker.js(3 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(3 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(4 hunks)zmsadmin/js/page/availabilityDay/index.js(22 hunks)zmsadmin/js/page/availabilityDay/layouts/accordion.js(1 hunks)zmsadmin/js/page/availabilityDay/layouts/page.js(2 hunks)zmsadmin/js/page/availabilityDay/saveBar.js(1 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(3 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(4 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/src/Zmsdb/Process.php(1 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(8 hunks)zmsentities/tests/Zmsentities/ProcessTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- zmsadmin/js/page/availabilityDay/timetable/graphview.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 (4)
- zmsadmin/js/page/availabilityDay/helpers.js
- zmsadmin/js/page/availabilityDay/form/datepicker.js
- zmsadmin/js/page/availabilityDay/form/content.js
- zmsentities/tests/Zmsentities/ProcessTest.php
🧰 Additional context used
📓 Learnings (2)
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.
zmsadmin/js/page/availabilityDay/index.js (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-01-27T16:54:44.665Z
Learning: In the eappointment system's getConflictList method, create a copy of selectedAvailability before modifying it to avoid mutating state directly, handle null selectedAvailability gracefully, clear stale conflicts on errors, and maintain a consistent promise chain to ensure proper button state management.
🪛 Biome (1.9.4)
zmsadmin/js/page/availabilityDay/form/index.js
[error] 46-50: 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/layouts/accordion.js
[error] 160-160: 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/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/content.js
[error] 23-23: 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] 123-123: This property value named disabled is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named disabled
(lint/suspicious/noDuplicateObjectKeys)
[error] 168-168: This property value named disabled is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named disabled
(lint/suspicious/noDuplicateObjectKeys)
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)
[error] 326-326: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 327-327: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (85)
zmsapi/tests/Zmsapi/AvailabilityAddTest.php (6)
5-6: Consistent Import Statements
Good addition of the exception namespace. The\DateTimeimport appears consistent with other references.
14-14: Empty line
No action needed.
50-82: Overlap Testing
These additions properly test for overlapping availabilities. The arrangement is clear and ensures the exception is thrown as expected.
84-116: Duplicate Overlap Testing
The test is well-structured to detect duplicated overlapping availabilities. Nicely done.
118-139: Invalid End Time Coverage
This test effectively verifies that an end time earlier than the start time triggers an appropriate exception.
158-174: Add Failed Scenario
The testing logic verifies that an invalid ID triggers anAvailabilityAddFailedexception. This ensures robust error coverage.zmsapi/src/Zmsapi/AvailabilityAdd.php (5)
23-23: New Exception Namespace
Replacing the older exception withAvailabilityAddFailedis consistent with the new error handling approach.
40-40: Verify default value for references
Validator::param('resolveReferences')->isNumber()->setDefault(2)sets a default of 2. Confirm if this matches the intended behavior for reference resolution.
56-61: Collection Instantiation
Creating a new collection and validating each entity is straightforward. Nicely done.
66-68: Conflict Checking
checkNewVsNewConflictsensures new availabilities do not overlap among themselves. This is a robust early check.
140-156: Handling Missing Entity
ThrowingAvailabilityAddFailedwith a descriptive message is helpful. This aligns with the new approach of specialized exceptions.zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (5)
9-9: BadRequest Usage
Importing and usingBadRequestExceptionensures consistent error handling.
11-11: Process Class Usage
IncludingProcessfor conflict tracking is a well-structured approach.
35-41: Comprehensive Input Validation
VerifyingavailabilityList,scope, andselectedDateis correct. Ifkindis also mandatory in some scenarios, consider validating that as well.
49-51: Performing In-Memory Overlap Check
Good practice to check new vs. new conflicts before merging with existing data.
77-82: Conflict Discovery with checkAllVsExistingConflicts
Incorporating existing conflicts after sorting the list makes the approach more systematic.zmsapi/src/Zmsapi/AvailabilityUpdate.php (20)
4-4: No concerns on doc-block addition.
16-16: Import for AvailabilityList collection looks good.
22-24: Newly imported exceptions are appropriate for handling error conditions.
48-48: Blank line change noted.
No functional impact.
49-55: AvailabilityList and scope checks are valid, but partially rely on the first array element.
If future requirements allow multiple scopes in one request, you might need additional checks to confirm they match across all items.
56-67: Creating a new collection and validating IDs is correct.
Consider concurrency or version checks if multiple users can update the same entity simultaneously.
68-69: Adding validated entities to the new collection is straightforward.
74-75: Selected date construction is fine.
79-83: Merging existing availabilities is implemented clearly.
84-101: Validation loop is correct.
Time calculations seem coherent and the call tovalidateInputsis aligned with the newly introduced method.
107-113: Locating the 'origin' availability is fine if only one such entry exists.
115-121: Filtering out exclusions and origin is a valid approach.
123-129: Additional conflict check is commendable.
Again, consider a more descriptive exception message on line 128 if needed.
131-136: Updating each new entity individually, then recalculating slots is consistent with earlier logic.
139-139: Response message assignment is straightforward.
142-142: JSON rendering with status code is standard.
145-146: Method signature for writeEntityUpdate is logically consistent with usage.
147-147: Repository usage repeated here is fine, matching earlier patterns.
166-166: Returning $updatedEntity is correct to complete the update process.
181-189: Filtering only new records by checking “temp” is explicit.zmsentities/src/Zmsentities/Collection/AvailabilityList.php (5)
21-21: Indexing workstationCount directly is safe if we are sure it always exists.
164-174: New doc-block and method signature for validateInputs are clear.
175-193: Combining validation checks into $errorList is a clean approach.
195-234: Method getDateTimeRangeFromList correctly calculates earliest/largest date range.
Be mindful of time zones if the environment changes.
236-236: Method signature for checkAllVsExistingConflicts updated.
Looks consistent with usage in AvailabilityUpdate.zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (8)
5-7: Imports for Entity, Query, and AvailabilityUpdateFailed are appropriate for test usage.
16-39: Populating test input with realistic timestamps is a good practice.
42-42: setWorkstation() call presumably sets up environment for tests. Good.
43-79: Rendering the request and verifying the response is well-structured.
83-177: testDuplicateOverlappingAvailability scenario thoroughly covers conflict checks.
179-299: testOverlappingAvailability ensures multiple IDs with partial overlapping times are handled.
301-366: testInvalidEndTime properly asserts exception for inverted schedule.
380-395: testNotFound checks behavior when entity ID is invalid.zmsadmin/js/page/availabilityDay/index.js (9)
112-112: Retain Consistent Variable NamingRetrieving
selectedDateusingformatTimestampDatefor clarity is a good addition. Ensure all similarly derived date variables in the code follow consistent naming, e.g.selectedDateovercurrentDateordateFormatted, for uniformity.
135-138: Clarify the Payload StructureYou’ve introduced a
payloadobject containingavailabilityListandselectedDate. This is a clear approach. Confirm that all server-side endpoints and any typed interfaces reflect these fields accurately to avoid mismatches.
147-156: Good User Feedback and Smooth ScrollingLogging success, refreshing data, showing success messages, and then scrolling to the bottom is a good user flow. Keep in mind performance on large pages or slow connections, as multiple re-renders plus a forced scroll could hinder responsiveness.
165-169: Improve Error Logging StrategyWhile ignoring a 404 may be intentional, be sure that other potential HTTP edge cases are not silently dismissed. If ignoring 404 is standard for your use case, a comment explaining it would help maintainers.
347-348: Validate Potential Off-By-One IssuesWhen computing
endDateTimestampbased onyesterday.unix()vs.availability.startDate, ensure no off-by-one errors ifavailability.startDateis exactly equal toyesterday.unix(). A boundary test in your functional suite might be prudent.
503-519: Validate Timestamps Early
validateAvailabilityList()is helpful. Ensure that all new or updated availabilities go through this check before further processing, especially if code outside this file manipulates or reuses the availability data.
524-568: Promise-Based Validation Flow
getValidationList()returns aPromise. This approach is clean. Ensure that all consumer methods properly chain.thenor.catch. Also check for any race conditions where conflicts might be checked before validation completes.
639-688: Comprehensive Error Handling for Slot CalculationIn
readCalculatedAvailabilityList(), catching errors from validation is good. Double-check that partial failures don’t leave the UI in an inconsistent state. It might be beneficial to revert or reset certain changes if the subsequent slot fetch fails.
863-869: Save Bar ImplementationRendering
SaveBarconditioned onthis.state.lastSaveis a useful design. Make sure this leads to a predictable user experience in any asynchronous or error scenarios (for instance, if data is partially saved).zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityupdatefailed.twig (1)
1-7: Clear and Informative Error FeedbackThis Twig template clearly communicates conflict-related issues when updating availability. If your application supports multiple languages, consider using a translation mechanism to localize the message and title.
zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityaddfailed.twig (1)
1-7: Consistent Conflict MessagingThis newly introduced template provides coherent user feedback for failed availability creation. Verify that any associated server-side logic or exceptions throw the correct context into this template, and consider i18n if needed.
zmsapi/src/Zmsapi/Exception/Availability/AvailabilityAddFailed.php (1)
8-14: LGTM!The implementation is correct with appropriate error code (400) for client errors and a clear error message.
zmsadmin/js/page/availabilityDay/layouts/page.js (1)
14-14: LGTM!The duplicate rendering of
saveBaris intentional and acceptable, as it displays the save bar at both the top and bottom of the layout. This is consistent with previous feedback from PR #685.Also applies to: 23-23
zmsadmin/js/page/availabilityDay/form/errors.js (1)
8-14: LGTM!The enhanced error handling is well-implemented:
- Correctly handles both array and non-array items
- Generates unique keys for nested items
- Maintains accessibility with ARIA attributes
zmsadmin/js/page/availabilityDay/timetable/index.js (1)
37-37: LGTM! Verify the fix for overlapping opening hours.The restored mapping of
OpeningTimebarcomponents is crucial for the validation of overlapping opening hours.Let's verify the fix by checking for other validation-related changes:
✅ Verification successful
Validation for overlapping opening hours is properly restored
The code shows a complete validation system with:
- Time format validation in form/validate.js
- Availability list validation in index.js
- Proper mapping of OpeningTimebar components with validation data and onSelect handlers
The restored mapping ensures that each opening time slot is properly validated through the validation chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for related validation logic in the codebase # Test 1: Look for validation functions related to opening hours echo "Searching for opening hours validation..." rg -l "openinghours.*valid|valid.*openinghours" # Test 2: Look for conflict detection logic echo "Searching for conflict detection..." rg -l "conflict.*opening|opening.*conflict" # Test 3: Look for related test files echo "Searching for related test files..." fd -g "*test*.php" -g "*test*.js" | rg -l "opening|availability|conflict"Length of output: 679
Script:
#!/bin/bash # Search for OpeningTimebar component and validation logic echo "Searching for OpeningTimebar component..." rg -l "OpeningTimebar" echo -e "\nSearching for time validation..." rg -l "validateTime|timeValidation|checkTime" echo -e "\nSearching for availability validation..." rg -l "validateAvailability|checkAvailability" echo -e "\nSearching for test files..." fd . -t f -e test.js -e test.php | rg -l "opening|availability" echo -e "\nLooking for OpeningTimebar implementation..." ast-grep --pattern 'const OpeningTimebar = $_'Length of output: 18252
Script:
#!/bin/bash # Examine validation logic implementation echo "Checking time validation implementation..." rg -A 10 "validateTime|timeValidation|checkTime" zmsadmin/js/page/availabilityDay/form/validate.js echo -e "\nChecking availability validation..." rg -A 10 "validateAvailability|checkAvailability" zmsadmin/js/page/availabilityDay/index.js echo -e "\nChecking OpeningTimebar component..." rg -A 10 "OpeningTimebar" zmsadmin/js/page/availabilityDay/timetable/index.jsLength of output: 2479
Script:
#!/bin/bash # Check OpeningTimebar component implementation echo "Looking for OpeningTimebar component implementation..." rg -A 15 "const OpeningTimebar" zmsadmin/js/page/availabilityDay/widgets/timeBars/opening.js echo -e "\nChecking for time overlap validation..." rg -A 10 "overlap|conflict" zmsadmin/js/page/availabilityDay/widgets/timeBars/opening.jsLength of output: 399
Script:
#!/bin/bash # Final attempt to locate overlap validation echo "Searching for OpeningTimebar in different locations..." fd . -t f | rg -l "OpeningTimebar" echo -e "\nSearching for time-related validation..." rg "function.*valid.*time|check.*time|validate.*time" --type js echo -e "\nSearching for time comparison logic..." rg "(startTime|endTime).*(>|<|>=|<=|compare)" --type jsLength of output: 65905
zmsadmin/js/page/availabilityDay/saveBar.js (1)
11-19: LGTM! Well-implemented auto-hide functionality.The visibility state management with cleanup is implemented correctly. The useEffect hook properly handles the timer cleanup on unmount and when lastSave changes.
zmsadmin/js/page/availabilityDay/form/conflicts.js (1)
14-24: LGTM! Improved conflict message organization.The refactoring to use an array of objects with
messageanddatesproperties improves code structure and maintainability.zmsadmin/js/page/availabilityDay/form/footerButtons.js (2)
7-9: LGTM! Well-implemented availability check.The check for temporary IDs using optional chaining is a robust way to detect new availabilities.
26-26: Verify the button state logic.The disable conditions for both buttons have been updated to include
hasErrors. Please ensure this aligns with the expected behavior:
- "Abbrechen" is enabled when there are changes or issues to revert
- "Alle Änderungen aktivieren" is disabled when there are validation issues
Also applies to: 36-36
✅ Verification successful
Button state logic is correctly implemented
The button state logic has been verified and is working as expected:
- "Abbrechen" (Cancel) button is correctly enabled when there are changes or issues to revert (stateChanged, hasNewAvailabilities, hasConflicts, or hasErrors)
- "Alle Änderungen aktivieren" (Activate All Changes) button is properly disabled when there are validation issues (hasErrors, hasConflicts, or hasSlotCountError)
The implementation aligns with the expected behavior and includes proper validation handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering the button state logic rg -l "button.*disabled.*hasErrors" --type jsLength of output: 47
Script:
#!/bin/bash # Search for the component implementation and related state management echo "=== Component Implementation ===" ast-grep --pattern 'const footerButtons = $$$' echo -e "\n=== State Management ===" rg "hasErrors|stateChanged|hasNewAvailabilities|hasConflicts" -A 5 -B 5 --type js echo -e "\n=== Similar Button Patterns ===" rg "disabled=\{.*\}" --type js -A 1 -B 1Length of output: 49880
zmsadmin/js/page/availabilityDay/form/index.js (1)
54-74: LGTM! Comprehensive error handling.The component properly handles and passes error states to child components, including slot count errors and conflicts.
zmsdb/tests/Zmsdb/AvailabilityTest.php (1)
77-77: Verify the default value in the schema.The assertion change from 12 to 10 for
slotTimeInMinuteslooks good. This aligns with the schema changes.Let's verify the default value in the schema:
✅ Verification successful
Default value of 10 for
slotTimeInMinutesis correctly defined in the schema🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the default value of slotTimeInMinutes in the schema rg -A 1 "slotTimeInMinutes" zmsentities/schema/availability.jsonLength of output: 176
zmsdb/tests/Zmsdb/ProcessConflictTest.php (2)
56-57: LGTM!The comment formatting changes look good.
103-105: LGTM! Enhanced error messages.The updated error messages are more descriptive and user-friendly, providing specific time ranges for the conflicts.
Also applies to: 120-122
zmsadmin/js/page/availabilityDay/layouts/accordion.js (1)
168-170: LGTM! Props addition.The addition of data and availabilitylist props to FooterButtons looks good.
zmsadmin/js/page/availabilityDay/form/validate.js (3)
4-26: LGTM! Comprehensive validation setup.The validation setup is well-structured with clear separation of concerns for different validation types.
37-69: LGTM! Robust null value and bookable day range validation.The validation functions are well-implemented with clear error messages.
Also applies to: 71-85
87-146: LGTM! Comprehensive timestamp and time format validation.The validation logic thoroughly checks all aspects of timestamps and time formats.
zmsentities/src/Zmsentities/Availability.php (2)
115-115: Type hint improvements for better type safetyThe parameter type hints have been updated from untyped to
stringfor better type safety and API clarity.Also applies to: 139-139
775-813: Improved conflict detection with better messagingThe conflict detection logic has been enhanced with:
- More descriptive conflict messages
- Better variable names for readability
- Proper formatting for date ranges
zmsdb/src/Zmsdb/Process.php (1)
495-495: Method call updated to use new conflict checking APIThe method now uses
checkAllVsExistingConflicts()instead ofgetConflicts()and includes the end date parameter for more precise conflict detection.zmsentities/tests/Zmsentities/AvailabilityTest.php (2)
182-182: Updated test assertions for new slot calculationsThe expected values for available seconds have been updated to match the new slot calculation logic.
Also applies to: 199-199
891-957: Enhanced conflict detection test casesThe test cases have been expanded to cover:
- Equal time ranges
- Overlapping time ranges
- Slot size conflicts
The assertions now verify the exact conflict messages.zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (1)
13-26: Added API call setup with fixture dataTest setup has been improved by configuring API calls with proper fixture data, making the test more realistic and maintainable.
zmsadmin/js/page/availabilityDay/form/formButtons.js (2)
5-6: LGTM: Enhanced validation logic for form buttons.The addition of
hasErrorsandhasSlotCountErrorprops improves form validation by preventing actions when validation errors exist.
24-24: Verify isCreatingExclusion prop usage.The button's disabled state now considers
isCreatingExclusion, but this prop is not documented in PropTypes.Add the missing PropType:
FormButtons.propTypes = { // ... existing props + hasSlotCountError: PropTypes.bool, isCreatingExclusion: PropTypes.bool }zmsentities/schema/availability.json (1)
21-21: Verify impact of changing default slot time.Changing the default
slotTimeInMinutesfrom 12 to 10 could affect:
- Existing appointments
- Scheduling logic
- Integration points
Please ensure:
- Data migration strategy is in place
- All dependent systems are updated
- Impact on existing appointments is assessed
| (startDate) ? availability.startDate = startDate : null; | ||
| (endDate) ? availability.endDate = endDate : null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Assignments Within Ternaries
Using a ternary to conditionally assign availability.startDate and availability.endDate (lines 326-327) can reduce clarity:
(startDate) ? availability.startDate = startDate : null;
(endDate) ? availability.endDate = endDate : null;Instead, consider:
if (startDate) {
availability.startDate = startDate;
}
if (endDate) {
availability.endDate = endDate;
}[suspicious_noAssignInExpressions]
🧰 Tools
🪛 Biome (1.9.4)
[error] 326-326: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 327-327: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| getConflictList() { | ||
| return new Promise((resolve, reject) => { | ||
| const { availabilitylist } = this.state; | ||
| const { timestamp } = this.props; | ||
| const requestOptions = { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| availabilityList: availabilitylist.filter(a => a.kind !== 'exclusion'), | ||
| selectedDate: formatTimestampDate(timestamp) | ||
| }), | ||
| }; | ||
|
|
||
| const url = `${this.props.links.includeurl}/availability/conflicts/`; | ||
|
|
||
| fetch(url, requestOptions) | ||
| .then((res) => res.json()) | ||
| .then( | ||
| (data) => { | ||
| console.log("Conflicts fetched successfully:", data); | ||
| this.setState({ | ||
| conflictList: { | ||
| itemList: { ...data.conflictList }, | ||
| conflictIdList: data.conflictIdList, | ||
| }, | ||
| }); | ||
| } else { | ||
| console.log('conflict error', err); | ||
| if (data.conflictIdList.length > 0) { | ||
| this.errorElement?.scrollIntoView(); | ||
| } | ||
| resolve(); | ||
| }, | ||
| (err) => { | ||
| console.error("Conflict fetch error:", err); | ||
| hideSpinner(); | ||
| reject(err); | ||
| } | ||
| hideSpinner(); | ||
| } | ||
| ) | ||
| ) | ||
| .catch((error) => { | ||
| console.warn("Conflict fetch failed:", error); | ||
| reject(error); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Conflict List Could Use an Immutable Copy
From the retrieved learnings: consider creating a copy of selectedAvailability or relevant data before modifications to avoid direct state mutations. If availabilitylist is ever re-used or mutated, it might cause side effects.
| {/* Convert newlines in the message to <br /> tags */} | ||
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> |
There was a problem hiding this comment.
Security: Replace dangerouslySetInnerHTML with safe alternatives.
Using dangerouslySetInnerHTML for newline conversion exposes the application to XSS attacks if the message contains unsanitized user input.
- {/* Convert newlines in the message to <br /> tags */}
- <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} />
+ <div>
+ {item.message.split('\n').map((line, i) => (
+ <React.Fragment key={i}>
+ {line}
+ {i < item.message.split('\n').length - 1 && <br />}
+ </React.Fragment>
+ ))}
+ </div>📝 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.
| {/* Convert newlines in the message to <br /> tags */} | |
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> | |
| <div> | |
| {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)
| "kind": { | ||
| "type": "string", | ||
| "description": "format" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the 'kind' property definition.
The current definition lacks:
- Allowed values (enum)
- Clear description
- Default value
- Purpose explanation
Consider this improved definition:
"kind": {
"type": "string",
- "description": "format"
+ "enum": ["default", "special"],
+ "default": "default",
+ "description": "Specifies the format type of the availability. Use 'default' for standard availability and 'special' for exceptions."
},📝 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.
| "kind": { | |
| "type": "string", | |
| "description": "format" | |
| }, | |
| "kind": { | |
| "type": "string", | |
| "enum": ["default", "special"], | |
| "default": "default", | |
| "description": "Specifies the format type of the availability. Use 'default' for standard availability and 'special' for exceptions." | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
zmsadmin/js/page/availabilityDay/helpers.js (1)
Line range hint
82-157: LGTM! Well-structured time slot management implementation.The enhanced logic for managing time slots and handling overlaps is well-implemented. The code effectively:
- Filters and sorts existing availabilities
- Handles edge cases (current time, day boundaries)
- Prevents overlapping slots
- Uses nullish coalescing for defaults
Consider adding JSDoc documentation.
The complex time slot calculation logic would benefit from detailed documentation explaining the business rules and edge cases.
+/** + * Creates a new availability object with calculated time slots. + * @param {number} timestamp - Unix timestamp for the availability date + * @param {string} tempId - Temporary identifier for the new availability + * @param {Object} scope - Scope object containing preferences and provider data + * @param {Array} existingAvailabilities - List of existing availabilities to prevent overlaps + * @returns {Object} New availability object with calculated time slots + */ export const getNewAvailability = (timestamp, tempId, scope, existingAvailabilities = []) => {zmsadmin/js/page/availabilityDay/form/content.js (5)
21-33: Consider enhancing error type checking robustness.While the error handling is good, the error type check could be more robust using a dedicated function.
+const hasErrorOfType = (errorList, errorType) => + Object.values(errorList).some(error => + error.itemList?.some(item => + item?.some(err => err.type === errorType) + ) + ); -const hasEndTimePastError = Object.values(errorList) - .some(error => error.itemList?.[0]?.[0]?.type === 'endTimePast'); +const hasEndTimePastError = hasErrorOfType(errorList, 'endTimePast');🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: 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)
40-67: Consider moving inline styles to CSS classes.The warning message implementation is good, but the inline styles should be moved to CSS classes for better maintainability.
+// Add to your CSS file +.warning-icon-container { + position: absolute; + top: -15px; + left: 7px; + background-color: #fcaa67; + width: 30px; + height: 30px; + display: flex; + justify-content: center; + align-items: center; +} + +.warning-icon { + color: white; +} + +.warning-section { + position: relative; + border-color: #cccccc; + margin: 20px auto; +} <section - style={{ - position: 'relative', - borderColor: '#cccccc', - margin: '20px auto' - }} + className="warning-section" role="alert" > <div - style={{ - position: 'absolute', - top: '-15px', - left: '7px', - backgroundColor: '#fcaa67', - width: '30px', - height: '30px', - display: 'flex', - justifyContent: 'center', - alignItems: 'center' - }} + className="warning-icon-container" > <i className="fas fa-exclamation-circle" title="Wichtiger Hinweis" aria-hidden="true" - style={{ color: 'white' }} + className="warning-icon" /> </div>
109-113: Consider creating a custom hook for input disabled state.The disabled state logic is repeated across multiple inputs. Consider extracting it to a custom hook.
+// Create a new file: hooks/useInputDisabled.js +import { useMemo } from 'react'; + +export const useInputDisabled = (errorList, calenderDisabled) => { + return useMemo(() => { + const hasEndTimePastError = Object.values(errorList) + .some(error => error.itemList?.[0]?.[0]?.type === 'endTimePast'); + return hasEndTimePastError || calenderDisabled; + }, [errorList, calenderDisabled]); +}; // In FormContent component: +import { useInputDisabled } from '../hooks/useInputDisabled'; + const FormContent = (props) => { - const hasEndTimePastError = Object.values(errorList) - .some(error => error.itemList?.[0]?.[0]?.type === 'endTimePast'); - const inputDisabled = hasEndTimePastError || calenderDisabled; + const inputDisabled = useInputDisabled(errorList, calenderDisabled);Also applies to: 138-153, 176-180
236-298: Consider extracting duplicated workstation options logic.The workstation options generation logic is duplicated across multiple select inputs.
+const getWorkstationOptions = (count) => { + return range(0, count).map(n => { + const workstation = (n === 1) ? "Arbeitsplatz" : "Arbeitsplätze"; + return { + title: `${n} ${workstation}`, + value: `${n}`, + name: `${n} ${workstation}` + }; + }); +}; <Inputs.Select name="workstationCount_intern" value={data.workstationCount_intern} attributes={{ "id": "WsCountIntern", "disabled": inputDisabled }} {...{ onChange }} - options={range(0, 50).map(n => { - let workstation = (n == 1) ? "Arbeitsplatz" : "Arbeitsplätze"; - return { - title: `${n} ${workstation}`, - value: `${n}`, - name: `${n} ${workstation}` - } - })} + options={getWorkstationOptions(50)} />
23-23: Simplify boolean expression.The conditional expression can be simplified by removing the unnecessary ternary operator.
-const calenderDisabled = data.type && data.slotTimeInMinutes ? false : true; +const calenderDisabled = !(data.type && data.slotTimeInMinutes);🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: 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 (3)
54-56: Consider debouncing the scroll handler.The
handleScrollToBottommethod is called multiple times in quick succession, particularly in error and success callbacks. Consider debouncing this method to prevent unnecessary reflows.+ import { debounce } from 'lodash'; class AvailabilityPage extends Component { constructor(props) { super(props) + this.handleScrollToBottom = debounce(() => { + window.scrollTo(0, document.body.scrollHeight); + }, 150); } - handleScrollToBottom() { - window.scrollTo(0, document.body.scrollHeight); - }
513-531: Enhance validation with constants and type checking.The validation logic could be more maintainable with defined constants and explicit type checking.
+ const TIME_FORMAT_REGEX = /^\d{2}:\d{2}(:\d{2})?$/; + const UNIX_TIMESTAMP_MIN = 0; + validateAvailabilityList(availabilitylist) { - const timeRegex = /^\d{2}:\d{2}(:\d{2})?$/; + if (!Array.isArray(availabilitylist)) { + console.warn("Invalid input: availabilitylist must be an array"); + return []; + } const isValidTimestamp = (timestamp) => { + const timestampNum = Number(timestamp); - return !Number.isNaN(Number(timestamp)) && moment.unix(timestamp).isValid(); + return !Number.isNaN(timestampNum) && + timestampNum > UNIX_TIMESTAMP_MIN && + moment.unix(timestampNum).isValid(); }; const invalidAvailabilities = availabilitylist.filter((availability) => { const hasInvalidDates = !isValidTimestamp(availability.startDate) || !isValidTimestamp(availability.endDate); const hasInvalidTimes = - !timeRegex.test(availability.startTime) || !timeRegex.test(availability.endTime); + !TIME_FORMAT_REGEX.test(availability.startTime) || !TIME_FORMAT_REGEX.test(availability.endTime); if (hasInvalidDates || hasInvalidTimes) { console.warn("Invalid availability detected:", availability); } return hasInvalidDates || hasInvalidTimes; }); return invalidAvailabilities; }
580-621: Enhance error handling with custom error types.The error handling could be more robust with custom error types and specific error messages for different failure scenarios.
+ class ConflictFetchError extends Error { + constructor(message, status, response) { + super(message); + this.name = 'ConflictFetchError'; + this.status = status; + this.response = response; + } + } getConflictList() { return new Promise((resolve, reject) => { // ... existing code ... fetch(url, requestOptions) .then((res) => { + if (!res.ok) { + throw new ConflictFetchError( + 'Failed to fetch conflicts', + res.status, + res + ); + } return res.json(); }) .then( (data) => { + if (!data.conflictList || !data.conflictIdList) { + throw new ConflictFetchError( + 'Invalid conflict data received', + 200, + data + ); + } // ... existing success handling ... }, (err) => { - console.error("Conflict fetch error:", err); + console.error( + `Conflict fetch error (${err.status}):`, + err.message + ); hideSpinner(); reject(err); } )zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (2)
116-160: Consider refactoring for improved readability and maintainability.The nested loops and complex time comparison logic could be simplified:
- Extract the scope ID comparison into a helper method:
private static function haveSameScope($availability1, $availability2): bool { $scope1Id = is_array($availability1->scope) ? ($availability1->scope['id'] ?? null) : ($availability1->scope->id ?? null); $scope2Id = is_array($availability2->scope) ? ($availability2->scope['id'] ?? null) : ($availability2->scope->id ?? null); return $scope1Id == $scope2Id; }
- Extract the time overlap check into a helper method:
private static function hasTimeOverlap($availability1, $availability2): bool { $time1Start = (new \DateTimeImmutable())->setTimestamp($availability1->startDate) ->modify($availability1->startTime)->format('H:i'); $time1End = (new \DateTimeImmutable())->setTimestamp($availability1->endDate) ->modify($availability1->endTime)->format('H:i'); $time2Start = (new \DateTimeImmutable())->setTimestamp($availability2->startDate) ->modify($availability2->startTime)->format('H:i'); $time2End = (new \DateTimeImmutable())->setTimestamp($availability2->endDate) ->modify($availability2->endTime)->format('H:i'); return $time2Start < $time1End && $time1Start < $time2End; }
Line range hint
1-191: Consider implementing a dedicated ConflictDetector service.The current implementation handles multiple responsibilities (validation, conflict detection, data fetching). As the conflict detection logic grows more complex, consider extracting it into a dedicated service class to improve maintainability and testability.
This would allow:
- Better separation of concerns
- Easier unit testing of conflict detection logic
- Potential reuse in other parts of the system
- Simpler maintenance and future extensions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zmsadmin/js/page/availabilityDay/form/content.js(5 hunks)zmsadmin/js/page/availabilityDay/helpers.js(4 hunks)zmsadmin/js/page/availabilityDay/index.js(22 hunks)zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(4 hunks)
🧰 Additional context used
📓 Learnings (1)
zmsadmin/js/page/availabilityDay/index.js (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-01-27T16:54:44.665Z
Learning: In the eappointment system's getConflictList method, create a copy of selectedAvailability before modifying it to avoid mutating state directly, handle null selectedAvailability gracefully, clear stale conflicts on errors, and maintain a consistent promise chain to ensure proper button state management.
🪛 Biome (1.9.4)
zmsadmin/js/page/availabilityDay/form/content.js
[error] 23-23: 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] 213-214: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 336-336: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 337-337: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-unit-tests / zmsapi-test
- GitHub Check: call-unit-tests / zmsdb-test
🔇 Additional comments (7)
zmsadmin/js/page/availabilityDay/helpers.js (2)
242-246: LGTM! Improved form data handling.The enhanced logic for
startInDaysandendInDaysproperly handles empty values and defaults to scope preferences.
272-273: LGTM! Enhanced data cleanup logic.The improved cleanup logic for
open_fromandopen_tocorrectly handles falsy values and default value comparisons.zmsadmin/js/page/availabilityDay/index.js (1)
336-337:⚠️ Potential issueReplace assignments in expressions with explicit if statements.
Using assignments within ternary expressions reduces code readability and maintainability.
- (startDate) ? availability.startDate = startDate : null; - (endDate) ? availability.endDate = endDate : null; + if (startDate) { + availability.startDate = startDate; + } + if (endDate) { + availability.endDate = endDate; + }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 336-336: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 337-337: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (4)
35-41: LGTM! Input validation is thorough and well-structured.The validation checks for required fields are comprehensive and provide clear error messages.
147-149: Ensure HTML entities are properly escaped.The conflict message uses HTML entities (
 ) which might need proper escaping depending on how they're displayed.Verify that the frontend properly handles these HTML entities or consider using a more robust spacing solution.
Line range hint
164-191: LGTM! Error handling is robust and specific.The method properly handles the AvailabilityNotFound case while allowing other exceptions to propagate.
77-77: Verify handling of empty availability lists.The
getDateTimeRangeFromListcall could potentially fail iffilteredAvailabilityListis empty.Consider adding a guard clause:
+ if ($filteredAvailabilityList->count() === 0) { + return [ + 'conflictList' => [], + 'conflictIdList' => [] + ]; + } [$earliestStartDateTime, $latestEndDateTime] = $filteredAvailabilityList->getDateTimeRangeFromList($selectedDateTime);
| // Attempt to scroll immediately after state update, before the 6-second timer expires | ||
| if (this.successElement) { | ||
| this.successElement.scrollIntoView(); | ||
| } else { | ||
| // Fallback to bottom scroll if the element isn't available | ||
| this.handleScrollToBottom(); | ||
| } | ||
| }); | ||
| hideSpinner(); | ||
| this.handleScrollToBottom(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate scroll call.
There are two consecutive scroll operations: one in the setState callback and another immediately after. This could cause janky scrolling behavior.
}, () => {
if (this.successElement) {
this.successElement.scrollIntoView();
} else {
this.handleScrollToBottom();
}
});
hideSpinner();
- this.handleScrollToBottom();📝 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.
| // Attempt to scroll immediately after state update, before the 6-second timer expires | |
| if (this.successElement) { | |
| this.successElement.scrollIntoView(); | |
| } else { | |
| // Fallback to bottom scroll if the element isn't available | |
| this.handleScrollToBottom(); | |
| } | |
| }); | |
| hideSpinner(); | |
| this.handleScrollToBottom(); | |
| // Attempt to scroll immediately after state update, before the 6-second timer expires | |
| if (this.successElement) { | |
| this.successElement.scrollIntoView(); | |
| } else { | |
| // Fallback to bottom scroll if the element isn't available | |
| this.handleScrollToBottom(); | |
| } | |
| }); | |
| hideSpinner(); |
|
@coderabbitai since the daily cronjob run between 23:00-01:00 we should block that entire timespan not just 00:00 frontend validateStartTime backend validateStartTime |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the fix to block the entire 00:00-01:00 timespan: For frontend ( if ((startHour == "00") || (endHour == "00")) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 00:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}For backend ( if (($startHour == 0) || ($endHour == 0)) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 00:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}The changes:
🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 1353 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Let me provide an updated solution that better integrates with the existing test structure:
public function testMaintenanceWindowValidation()
{
$testCases = [
[
'name' => '23:00 start time',
'time' => '2025-01-30 23:00:00',
'endTime' => '2025-01-31 02:00:00'
],
[
'name' => '23:59 end time',
'time' => '2025-01-30 22:00:00',
'endTime' => '2025-01-30 23:59:00'
],
[
'name' => '00:00 start time',
'time' => '2025-01-31 00:00:00',
'endTime' => '2025-01-31 02:00:00'
],
[
'name' => '00:59 end time',
'time' => '2025-01-30 22:00:00',
'endTime' => '2025-01-31 00:59:00'
]
];
foreach ($testCases as $case) {
$availability = new Availability([
'id' => '1',
'weekday' => [
'monday' => '0',
'tuesday' => '4',
'wednesday' => '0',
'thursday' => '0',
'friday' => '0',
'saturday' => '0',
'sunday' => '0'
],
'repeat' => [
'afterWeeks' => '1',
],
'workstationCount' => [
'public' => '2',
'callcenter' => '2',
'intern' => '2'
],
'slotTimeInMinutes' => '15',
'startDate' => strtotime($case['time']),
'endDate' => strtotime($case['endTime']),
'startTime' => (new \DateTime($case['time']))->format('H:i:s'),
'endTime' => (new \DateTime($case['endTime']))->format('H:i:s'),
]);
$errors = $availability->validateStartTime(
new \DateTime('2025-01-30 10:00:00'), // today
new \DateTime('2025-01-31 10:00:00'), // tomorrow
new \DateTime($case['time']), // startDate
new \DateTime($case['endTime']), // endDate
new \DateTime('2025-01-30 10:00:00'), // selectedDate
'default' // kind
);
$this->assertNotEmpty(
$errors,
"Should block maintenance window for {$case['name']}"
);
$this->assertEquals('startOfDay', $errors[0]['type']);
$this->assertStringContainsString(
'zwischen 23:00 und 01:00',
$errors[0]['message'],
"Error message should mention maintenance window for {$case['name']}"
);
$this->assertStringContainsString(
'Cronjob',
$errors[0]['message'],
"Error message should mention cronjob for {$case['name']}"
);
}
// Test valid time outside maintenance window
$availability = new Availability([
'id' => '2',
'startDate' => strtotime('2025-01-30 10:00:00'),
'endDate' => strtotime('2025-01-30 22:00:00'),
'startTime' => '10:00:00',
'endTime' => '22:00:00',
]);
$errors = $availability->validateStartTime(
new \DateTime('2025-01-30 10:00:00'),
new \DateTime('2025-01-31 10:00:00'),
new \DateTime('2025-01-30 10:00:00'),
new \DateTime('2025-01-30 22:00:00'),
new \DateTime('2025-01-30 10:00:00'),
'default'
);
$this->assertEmpty(
$errors,
'Should allow times outside maintenance window'
);
}
import { validateStartTime } from '../../../../js/page/availabilityDay/form/validate';
import moment from 'moment';
describe('Availability Time Validation', () => {
const today = moment('2025-01-30 10:00:00');
const tomorrow = moment('2025-01-31 10:00:00');
const selectedDate = moment('2025-01-30 10:00:00');
const testCases = [
{
name: '23:00 start time',
data: {
startTime: '23:00',
endTime: '02:00',
startDate: moment('2025-01-30 23:00:00').unix(),
endDate: moment('2025-01-31 02:00:00').unix()
}
},
{
name: '23:59 end time',
data: {
startTime: '22:00',
endTime: '23:59',
startDate: moment('2025-01-30 22:00:00').unix(),
endDate: moment('2025-01-30 23:59:00').unix()
}
},
{
name: '00:00 start time',
data: {
startTime: '00:00',
endTime: '02:00',
startDate: moment('2025-01-31 00:00:00').unix(),
endDate: moment('2025-01-31 02:00:00').unix()
}
},
{
name: '00:59 end time',
data: {
startTime: '22:00',
endTime: '00:59',
startDate: moment('2025-01-30 22:00:00').unix(),
endDate: moment('2025-01-31 00:59:00').unix()
}
}
];
testCases.forEach(({ name, data }) => {
it(`should block ${name} during maintenance window`, () => {
const errors = validateStartTime(today, tomorrow, selectedDate, data);
const maintenanceError = errors.find(e => e.type === 'startOfDay');
expect(maintenanceError).toBeTruthy();
expect(maintenanceError.message).toContain('zwischen 23:00 und 01:00');
expect(maintenanceError.message).toContain('Cronjob');
});
});
it('should allow times outside maintenance window', () => {
const validData = {
startTime: '10:00',
endTime: '22:00',
startDate: moment('2025-01-30 10:00:00').unix(),
endDate: moment('2025-01-30 22:00:00').unix()
};
const errors = validateStartTime(today, tomorrow, selectedDate, validData);
expect(errors.find(e => e.type === 'startOfDay')).toBeFalsy();
});
});Key improvements:
Would you like me to:
🔗 Analysis chain@ThomasAFink Here's the complete solution to block the entire 23:00-01:00 timespan: For frontend: if ((startHour == "23" || startHour == "00") || (endHour == "23" || endHour == "00")) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}
return errorList;For backend: if (($startHour == 23 || $startHour == 0) || ($endHour == 23 || $endHour == 0)) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}Changes made:
@ThomasAFink Here's the complete solution with exact file paths: In // Replace this block:
if ((startHour == "00" && startMinute == "00") || (endHour == "00" && endMinute == "00")) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht "00:00" sein.'
})
}
// With this block:
if ((parseInt(startHour) >= 23 || parseInt(startHour) === 0) ||
(parseInt(endHour) >= 23 || parseInt(endHour) === 0)) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}In // Replace this block:
if (($startHour == 0 && $startMinute == 0) || ($endHour == 0 && $endMinute == 0)) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht "00:00" sein.'
];
}
// With this block:
if (($startHour >= 23 || $startHour === 0) || ($endHour >= 23 || $endHour === 0)) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}Changes made:
@ThomasAFink Here's the refined solution that better integrates with the existing code: In // Replace this block:
if ((startHour == "00" && startMinute == "00") || (endHour == "00" && endMinute == "00")) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht "00:00" sein.'
})
}
// With this block:
const startHourInt = parseInt(startHour);
const endHourInt = parseInt(endHour);
if (startHourInt >= 23 || startHourInt === 0 || endHourInt >= 23 || endHourInt === 0) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}In // Replace this block:
if (($startHour == 0 && $startMinute == 0) || ($endHour == 0 && $endMinute == 0)) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht "00:00" sein.'
];
}
// With this block:
if ($startHour >= 23 || $startHour === 0 || $endHour >= 23 || $endHour === 0) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}Key improvements in this version:
@ThomasAFink Here's the complete solution with exact file locations and changes: In const startHourInt = parseInt(startHour);
const endHourInt = parseInt(endHour);
// Replace the 00:00 check with:
if (startHourInt >= 23 || startHourInt === 0 || endHourInt >= 23 || endHourInt === 0) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}In // The hours are already integers from format('H')
if ($startHour >= 23 || $startHour === 0 || $endHour >= 23 || $endHour === 0) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}Changes made:
The validation will now properly block the entire maintenance window from 23:00 to 01:00, while maintaining the existing validation chain in both frontend and backend. @ThomasAFink I've double-checked our solution and it's complete. The changes will properly block the entire maintenance window (23:00-01:00) while maintaining consistency between frontend and backend. Here's a summary of what the solution achieves:
Would you like me to search for tests in a different way or are you happy with this solution? @ThomasAFink While we search for existing tests, here are the test cases we should ensure are covered: // Frontend test cases needed:
describe('Time validation', () => {
it('should block start time at 23:00', () => {
const data = { startTime: '23:00', endTime: '02:00' };
// Should return startOfDay error
});
it('should block end time at 23:59', () => {
const data = { startTime: '22:00', endTime: '23:59' };
// Should return startOfDay error
});
it('should block start time at 00:00', () => {
const data = { startTime: '00:00', endTime: '02:00' };
// Should return startOfDay error
});
it('should block end time at 00:59', () => {
const data = { startTime: '22:00', endTime: '00:59' };
// Should return startOfDay error
});
});// Backend test cases needed:
public function testTimeValidation()
{
// Test 23:00 hour
$this->assertContains('startOfDay', $this->validateTime('23:00', '02:00'));
// Test 23:59 hour
$this->assertContains('startOfDay', $this->validateTime('22:00', '23:59'));
// Test 00:00 hour
$this->assertContains('startOfDay', $this->validateTime('00:00', '02:00'));
// Test 00:59 hour
$this->assertContains('startOfDay', $this->validateTime('22:00', '00:59'));
}Would you like me to:
@ThomasAFink Let me propose a complete solution including tests. Here's what we need:
// In zmsadmin/js/page/availabilityDay/form/validate.js
const startHourInt = parseInt(startHour);
const endHourInt = parseInt(endHour);
if (startHourInt >= 23 || startHourInt === 0 || endHourInt >= 23 || endHourInt === 0) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}// In zmsentities/src/Zmsentities/Availability.php
if ($startHour >= 23 || $startHour === 0 || $endHour >= 23 || $endHour === 0) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}
// New file: zmsadmin/tests/page/availabilityDay/form/validate.test.js
import { validateStartTime, validateEndTime } from '../../../../js/page/availabilityDay/form/validate';
import moment from 'moment';
describe('Availability Time Validation', () => {
const today = moment();
const tomorrow = moment().add(1, 'days');
const selectedDate = moment();
describe('Maintenance Window Validation', () => {
it('should block start time at 23:00', () => {
const data = {
startTime: '23:00',
endTime: '02:00',
startDate: today.unix(),
endDate: tomorrow.unix()
};
const errors = validateStartTime(today, tomorrow, selectedDate, data);
expect(errors.find(e => e.type === 'startOfDay')).toBeTruthy();
expect(errors.find(e => e.type === 'startOfDay').message)
.toContain('zwischen 23:00 und 01:00');
});
it('should block end time at 23:59', () => {
const data = {
startTime: '22:00',
endTime: '23:59',
startDate: today.unix(),
endDate: today.unix()
};
const errors = validateStartTime(today, tomorrow, selectedDate, data);
expect(errors.find(e => e.type === 'startOfDay')).toBeTruthy();
});
it('should block start time at 00:00', () => {
const data = {
startTime: '00:00',
endTime: '02:00',
startDate: tomorrow.unix(),
endDate: tomorrow.unix()
};
const errors = validateStartTime(today, tomorrow, selectedDate, data);
expect(errors.find(e => e.type === 'startOfDay')).toBeTruthy();
});
it('should block end time at 00:59', () => {
const data = {
startTime: '22:00',
endTime: '00:59',
startDate: today.unix(),
endDate: tomorrow.unix()
};
const errors = validateStartTime(today, tomorrow, selectedDate, data);
expect(errors.find(e => e.type === 'startOfDay')).toBeTruthy();
});
});
});// New file: zmsentities/tests/Zmsentities/AvailabilityValidationTest.php
<?php
namespace BO\Zmsentities\Tests;
use PHPUnit\Framework\TestCase;
use BO\Zmsentities\Availability;
class AvailabilityValidationTest extends TestCase
{
private function createDateTime($timeString): \DateTime
{
return new \DateTime($timeString);
}
public function testMaintenanceWindowValidation(): void
{
$availability = new Availability();
// Test 23:00 hour
$errors = $availability->validateStartTime(
$this->createDateTime('2025-01-30 10:00'), // today
$this->createDateTime('2025-01-31 10:00'), // tomorrow
$this->createDateTime('2025-01-30 23:00'), // startDate
$this->createDateTime('2025-01-31 02:00'), // endDate
$this->createDateTime('2025-01-30 10:00'), // selectedDate
'default' // kind
);
$this->assertContains('startOfDay', array_column($errors, 'type'));
$this->assertStringContainsString(
'zwischen 23:00 und 01:00',
$errors[0]['message']
);
// Test 23:59 hour
$errors = $availability->validateStartTime(
$this->createDateTime('2025-01-30 10:00'),
$this->createDateTime('2025-01-31 10:00'),
$this->createDateTime('2025-01-30 22:00'),
$this->createDateTime('2025-01-30 23:59'),
$this->createDateTime('2025-01-30 10:00'),
'default'
);
$this->assertContains('startOfDay', array_column($errors, 'type'));
// Test 00:00 hour
$errors = $availability->validateStartTime(
$this->createDateTime('2025-01-30 10:00'),
$this->createDateTime('2025-01-31 10:00'),
$this->createDateTime('2025-01-31 00:00'),
$this->createDateTime('2025-01-31 02:00'),
$this->createDateTime('2025-01-30 10:00'),
'default'
);
$this->assertContains('startOfDay', array_column($errors, 'type'));
// Test 00:59 hour
$errors = $availability->validateStartTime(
$this->createDateTime('2025-01-30 10:00'),
$this->createDateTime('2025-01-31 10:00'),
$this->createDateTime('2025-01-30 22:00'),
$this->createDateTime('2025-01-31 00:59'),
$this->createDateTime('2025-01-30 10:00'),
'default'
);
$this->assertContains('startOfDay', array_column($errors, 'type'));
}
}These tests will verify:
Would you like me to:
@ThomasAFink Let me refine our test implementation to better match the project structure:
import { validateStartTime } from '../../../../js/page/availabilityDay/form/validate';
import moment from 'moment';
describe('Availability Time Validation', () => {
const today = moment();
const tomorrow = moment().add(1, 'days');
const selectedDate = moment();
const testCases = [
{
name: '23:00 start time',
data: {
startTime: '23:00',
endTime: '02:00',
startDate: today.unix(),
endDate: tomorrow.unix()
}
},
{
name: '23:59 end time',
data: {
startTime: '22:00',
endTime: '23:59',
startDate: today.unix(),
endDate: today.unix()
}
},
{
name: '00:00 start time',
data: {
startTime: '00:00',
endTime: '02:00',
startDate: tomorrow.unix(),
endDate: tomorrow.unix()
}
},
{
name: '00:59 end time',
data: {
startTime: '22:00',
endTime: '00:59',
startDate: today.unix(),
endDate: tomorrow.unix()
}
}
];
testCases.forEach(({ name, data }) => {
it(`should block ${name} during maintenance window`, () => {
const errors = validateStartTime(today, tomorrow, selectedDate, data);
const maintenanceError = errors.find(e => e.type === 'startOfDay');
expect(maintenanceError).toBeTruthy();
expect(maintenanceError.message).toContain('zwischen 23:00 und 01:00');
expect(maintenanceError.message).toContain('Cronjob');
});
});
it('should allow times outside maintenance window', () => {
const validData = {
startTime: '08:00',
endTime: '22:00',
startDate: today.unix(),
endDate: today.unix()
};
const errors = validateStartTime(today, tomorrow, selectedDate, validData);
expect(errors.find(e => e.type === 'startOfDay')).toBeFalsy();
});
});
public function testMaintenanceWindowValidation()
{
$availability = new Availability();
$testCases = [
[
'name' => '23:00 start time',
'start' => '2025-01-30 23:00',
'end' => '2025-01-31 02:00'
],
[
'name' => '23:59 end time',
'start' => '2025-01-30 22:00',
'end' => '2025-01-30 23:59'
],
[
'name' => '00:00 start time',
'start' => '2025-01-31 00:00',
'end' => '2025-01-31 02:00'
],
[
'name' => '00:59 end time',
'start' => '2025-01-30 22:00',
'end' => '2025-01-31 00:59'
]
];
foreach ($testCases as $case) {
$errors = $availability->validateStartTime(
new \DateTime('2025-01-30 10:00'), // today
new \DateTime('2025-01-31 10:00'), // tomorrow
new \DateTime($case['start']), // startDate
new \DateTime($case['end']), // endDate
new \DateTime('2025-01-30 10:00'), // selectedDate
'default' // kind
);
$this->assertNotEmpty($errors, "Should block {$case['name']}");
$this->assertEquals('startOfDay', $errors[0]['type']);
$this->assertStringContainsString('zwischen 23:00 und 01:00', $errors[0]['message']);
$this->assertStringContainsString('Cronjob', $errors[0]['message']);
}
// Test valid time
$errors = $availability->validateStartTime(
new \DateTime('2025-01-30 10:00'),
new \DateTime('2025-01-31 10:00'),
new \DateTime('2025-01-30 08:00'),
new \DateTime('2025-01-30 22:00'),
new \DateTime('2025-01-30 10:00'),
'default'
);
$this->assertEmpty($errors, 'Should allow times outside maintenance window');
}Key improvements:
Would you like me to:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 232 Script: Length of output: 1867 Script: Length of output: 3772 Script: Length of output: 158 Script: Length of output: 1468 Script: Length of output: 492 Script: Length of output: 364 Script: Length of output: 503 Script: Length of output: 6119 Script: Length of output: 42328 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the complete solution: For frontend ( const startHourInt = parseInt(startHour);
const endHourInt = parseInt(endHour);
if (startHourInt >= 23 || startHourInt === 0 || endHourInt >= 23 || endHourInt === 0) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}For backend ( if ($startHour >= 23 || $startHour === 0 || $endHour >= 23 || $endHour === 0) {
$errorList[] = [
'type' => 'startOfDay',
'message' => 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
];
}Would you like me to provide the test implementations as well? I can create comprehensive test cases covering all boundary conditions. ✏️ Learnings added
|
@coderabbitai currently only 00:00 is disabled how do we disable the full timespan in the timepicker? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's how to disable the entire maintenance window in the timepicker. Update the const filterPassedTime = (time) => {
const timeToCheck = moment(time);
const hour = timeToCheck.hour();
// Block maintenance window (23:00-01:00)
if (hour >= 23 || hour === 0) {
return false;
}
// Check for past times only on the current day
if (moment(this.state.selectedDate).isSame(moment.unix(this.props.attributes.today), 'day')) {
const currentTime = moment();
return timeToCheck.hour() > currentTime.hour() ||
(timeToCheck.hour() === currentTime.hour() && timeToCheck.minute() > currentTime.minute());
}
return true;
};Changes made:
This will prevent users from selecting any time between 23:00 and 01:00 in the timepicker, complementing the validation we added earlier. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 4244 Script: Length of output: 16678 |
It's 17:57 on the current day the following exist in this case 18:00 should be disabled from the timepicker because the opening hour starting from 18:00 is adjecent to the current time 17:57 but it it isn't |
|
@coderabbitai Putting the 23-01 check in filterPassedTime only disables it for the current day date but not for all dates. |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink You're right. Let's move the maintenance window check to setExcludeTimesForDay() {
if (this.state.kind === 'exclusion') {
return;
}
var times = [];
// Add maintenance window times (23:00-01:00)
const selectedDate = moment(this.state.selectedDate);
// Add 23:00-23:59
for (let minute = 0; minute < 60; minute++) {
times.push(selectedDate.clone().hour(23).minute(minute).toDate());
}
// Add 00:00-00:59
for (let minute = 0; minute < 60; minute++) {
times.push(selectedDate.clone().hour(0).minute(minute).toDate());
}
// Add existing availability times
const availabilities = [...this.state.availabilityList]
.filter(availability =>
availability.id !== this.state.availability.id &&
availability.type == this.state.availability.type &&
this.isWeekDaySelected(this.state.selectedDate, availability)
)
.sort((a, b) => moment(a.startTime, 'HH:mm').valueOf() - moment(b.startTime, 'HH:mm').valueOf());
// Rest of your existing code for handling availabilities...
this.setState({
excludeTimeList: times
});
}This solution:
Would you like me to add any additional time ranges to block? ✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 8949 |
@coderabbitai give full code for setExcludeTimesForDay |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the complete implementation of setExcludeTimesForDay() {
if (this.state.kind === 'exclusion') {
return;
}
var times = [];
// Add maintenance window times (23:00-01:00)
const selectedDate = moment(this.state.selectedDate);
// Add 23:00-23:59
for (let minute = 0; minute < 60; minute++) {
times.push(selectedDate.clone().hour(23).minute(minute).toDate());
}
// Add 00:00-00:59
for (let minute = 0; minute < 60; minute++) {
times.push(selectedDate.clone().hour(0).minute(minute).toDate());
}
// Filter and sort availabilities
const availabilities = [...this.state.availabilityList]
.filter(availability =>
availability.id !== this.state.availability.id &&
availability.type == this.state.availability.type &&
this.isWeekDaySelected(this.state.selectedDate, availability)
)
.sort((a, b) => {
const timeA = moment(a.startTime, 'HH:mm');
const timeB = moment(b.startTime, 'HH:mm');
return timeA.diff(timeB);
});
// Add regular excluded times
availabilities.forEach(availability => {
const startTime = moment(availability.startTime, 'hh:mm')
.add(this.state.availability.slotTimeInMinutes, "m");
const startOnDay = moment(this.state.selectedDate)
.set({"h": startTime.hours(), "m": startTime.minutes()})
.toDate();
const endTime = moment(availability.endTime, 'hh:mm')
.subtract(this.state.availability.slotTimeInMinutes, "m");
const endOnDay = moment(this.state.selectedDate)
.set({"h": endTime.hours(), "m": endTime.minutes()})
.toDate();
var currentTime = new Date(startOnDay);
while (currentTime < endOnDay) {
times.push(new Date(currentTime));
currentTime = moment(currentTime)
.add(this.state.availability.slotTimeInMinutes, "m")
.toDate();
}
times.push(endOnDay);
});
// Add boundary timestamps between adjacent availabilities
for (let i = 0; i < availabilities.length - 1; i++) {
const current = availabilities[i];
const next = availabilities[i + 1];
const currentEnd = moment(current.endTime, 'HH:mm');
const nextStart = moment(next.startTime, 'HH:mm');
// If they're adjacent (end time of one equals start time of next)
if (currentEnd.format('HH:mm') === nextStart.format('HH:mm')) {
// Add the boundary timestamp to excluded times
const boundaryTime = moment(this.state.selectedDate)
.set({"h": currentEnd.hours(), "m": currentEnd.minutes()})
.toDate();
times.push(boundaryTime);
}
}
this.setState({excludeTimeList: times});
}Key changes:
Would you like me to:
✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 308 Script: Length of output: 20002 |
It's 17:57 on the current day the following exist in this case 18:00 should be disabled from the timepicker because the opening hour starting from 18:00 is adjecent to the current time 17:57 but it it isn't |
|
It's 17:57 on the current day the following exist in this case 18:00 should be disabled from the timepicker because the opening hour starting from 18:00 is adjecent to the current time 17:57 but it it isn't |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here's the fix for the timepicker to properly handle times close to the current time: const filterPassedTime = (time) => {
if (!moment(this.state.selectedDate).isSame(moment.unix(this.props.attributes.today), 'day')) {
return true;
}
const BUFFER_MINUTES = 5; // Minimum minutes needed between current time and next slot
const currentTime = moment();
const timeToCheck = moment(time);
// Calculate minutes difference between times
const minutesDiff = timeToCheck.diff(currentTime, 'minutes');
// Block if time is in past or too close to current time
return minutesDiff >= BUFFER_MINUTES;
};Changes made:
This will ensure that times too close to the current time (like 18:00 when it's 17:57) are properly disabled in the timepicker. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
zmsadmin/js/page/availabilityDay/form/validate.js (1)
Line range hint
266-283: Add validation for overlapping hours of same appointment type.The PR's main objective is to prevent overlapping hours for the same appointment type, but this validation is missing.
Would you like me to help implement the overlapping hours validation? Here's a suggested implementation:
+function validateOverlappingHours(data, existingAvailabilities) { + const errorList = []; + if (!existingAvailabilities) return errorList; + + const newStart = parseTimestampAndTime(data.startDate, data.startTime); + const newEnd = parseTimestampAndTime(data.endDate, data.endTime); + + for (const existing of existingAvailabilities) { + if (existing.id === data.id || existing.type !== data.type) continue; + + const existingStart = parseTimestampAndTime(existing.startDate, existing.startTime); + const existingEnd = parseTimestampAndTime(existing.endDate, existing.endTime); + + if (newStart < existingEnd && newEnd > existingStart) { + errorList.push({ + type: 'overlappingHours', + message: 'Öffnungszeiten des gleichen Termintyps dürfen sich nicht überschneiden.' + }); + break; + } + } + return errorList; +}This would need to be integrated into the main validate function:
const validate = (data, props) => { // ... existing code ... + errorList.itemList.push(validateOverlappingHours(data, props.existingAvailabilities)); // ... existing code ... };
🧹 Nitpick comments (3)
zmsadmin/js/page/availabilityDay/form/validate.js (2)
29-29: Simplify the boolean expression.The ternary operation 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] 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)
Line range hint
196-223: Refactor time parsing logic to reduce code duplication.Both
validateStartTimeandvalidateEndTimeduplicate time parsing logic. Consider extracting this to a utility function.+function parseTimeComponents(timeStr) { + const [hours, minutes] = timeStr.split(':').map(val => parseInt(val)); + return { hours, minutes, dayMinutes: hours * 60 + minutes }; +} 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 { dayMinutes: dayMinutesStart, hours: startHour, minutes: startMinute } = parseTimeComponents(data.startTime); + const { dayMinutes: dayMinutesEnd, hours: endHour, minutes: endMinute } = parseTimeComponents(data.endTime);zmsadmin/js/page/availabilityDay/form/datepicker.js (1)
138-149: Optimize availability filtering performance.The current implementation creates a new array and performs multiple operations. Consider memoizing the filtered and sorted availabilities to improve performance, especially with large datasets.
+ // Add memoization to prevent unnecessary recalculations + const getMemoizedAvailabilities = React.useMemo(() => { return [...this.state.availabilityList] .filter(availability => availability.id !== this.state.availability.id && availability.type == this.state.availability.type && this.isWeekDaySelected(this.state.selectedDate, availability) ) .sort((a, b) => { const timeA = moment(a.startTime, 'HH:mm'); const timeB = moment(b.startTime, 'HH:mm'); return timeA.diff(timeB); }); + }, [this.state.availabilityList, this.state.availability.id, this.state.availability.type, this.state.selectedDate]); + + const availabilities = getMemoizedAvailabilities;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zmsadmin/js/page/availabilityDay/form/datepicker.js(3 hunks)zmsadmin/js/page/availabilityDay/form/validate.js(5 hunks)zmsentities/src/Zmsentities/Availability.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsentities/src/Zmsentities/Availability.php
🧰 Additional context used
🪛 Biome (1.9.4)
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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: call-unit-tests / zmsdb-test
- GitHub Check: call-unit-tests / zmsapi-test
🔇 Additional comments (3)
zmsadmin/js/page/availabilityDay/form/validate.js (1)
3-8: Add type checking for props parameter.The function assumes that
props.todayexists but doesn't validate it. Consider adding type checking to prevent runtime errors.const validate = (data, props) => { + if (!props?.today) { + throw new Error('props.today is required for validation'); + } const currentTime = new Date(); const today = moment(props.today, 'X');zmsadmin/js/page/availabilityDay/form/datepicker.js (2)
388-388: Verify error handling for time selection.The
filterTimeprop is correctly applied, but ensure that appropriate error messages are displayed to users when they attempt to select a blocked time.
129-136: Consider timezone safety for maintenance window calculation.The maintenance window calculation might be affected by timezone changes. Consider using UTC-based calculations to ensure consistent behavior across different timezones.
- const selectedDate = moment(this.state.selectedDate); + const selectedDate = moment(this.state.selectedDate).utc();
| } else { | ||
| errorList.push({ | ||
| type: 'startTimeMissing', | ||
| message: 'Die Startzeit darf nicht leer sein.' | ||
| }); | ||
| } | ||
|
|
||
| if (data.endTime) { | ||
| if (!timeRegex.test(data.endTime)) { | ||
| errorList.push({ | ||
| type: 'endTimeFormat', | ||
| message: 'Die Endzeit muss im Format "HH:mm:ss" oder "HH:mm" vorliegen.' | ||
| }); | ||
| } | ||
| } else { | ||
| errorList.push({ | ||
| type: 'endTimeMissing', | ||
| message: 'Die Endzeit darf nicht leer sein.' | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate null checks.
The null checks for startTime and endTime are already performed in validateNullValues. This creates redundant error messages.
if (data.startTime) {
if (!timeRegex.test(data.startTime)) {
errorList.push({
type: 'startTimeFormat',
message: 'Die Startzeit muss im Format "HH:mm:ss" oder "HH:mm" vorliegen.'
});
}
- } else {
- errorList.push({
- type: 'startTimeMissing',
- message: 'Die Startzeit darf nicht leer sein.'
- });
}
if (data.endTime) {
if (!timeRegex.test(data.endTime)) {
errorList.push({
type: 'endTimeFormat',
message: 'Die Endzeit muss im Format "HH:mm:ss" oder "HH:mm" vorliegen.'
});
}
- } else {
- errorList.push({
- type: 'endTimeMissing',
- message: 'Die Endzeit darf nicht leer sein.'
- });
}📝 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.
| } else { | |
| errorList.push({ | |
| type: 'startTimeMissing', | |
| message: 'Die Startzeit darf nicht leer sein.' | |
| }); | |
| } | |
| if (data.endTime) { | |
| if (!timeRegex.test(data.endTime)) { | |
| errorList.push({ | |
| type: 'endTimeFormat', | |
| message: 'Die Endzeit muss im Format "HH:mm:ss" oder "HH:mm" vorliegen.' | |
| }); | |
| } | |
| } else { | |
| errorList.push({ | |
| type: 'endTimeMissing', | |
| message: 'Die Endzeit darf nicht leer sein.' | |
| }); | |
| } | |
| if (data.startTime) { | |
| if (!timeRegex.test(data.startTime)) { | |
| errorList.push({ | |
| type: 'startTimeFormat', | |
| message: 'Die Startzeit muss im Format "HH:mm:ss" oder "HH:mm" vorliegen.' | |
| }); | |
| } | |
| } | |
| if (data.endTime) { | |
| if (!timeRegex.test(data.endTime)) { | |
| errorList.push({ | |
| type: 'endTimeFormat', | |
| message: 'Die Endzeit muss im Format "HH:mm:ss" oder "HH:mm" vorliegen.' | |
| }); | |
| } | |
| } |
| if (startHourInt >= 23 || startHourInt === 0 || endHourInt >= 23 || endHourInt === 0) { | ||
| errorList.push({ | ||
| type: 'startOfDay', | ||
| message: 'Die Uhrzeit darf nicht "00:00" sein.' | ||
| message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.' | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move maintenance window validation to a separate function.
The maintenance window check (23:00-01:00) should be in a dedicated function and applied consistently to both start and end times.
+function isInMaintenanceWindow(hour) {
+ return hour >= 23 || hour === 0;
+}
+
function validateStartTime(today, tomorrow, selectedDate, data) {
// ... existing code ...
- if (startHourInt >= 23 || startHourInt === 0 || endHourInt >= 23 || endHourInt === 0) {
+ if (isInMaintenanceWindow(startHourInt) || isInMaintenanceWindow(endHourInt)) {
errorList.push({
type: 'startOfDay',
message: 'Die Uhrzeit darf nicht zwischen 23:00 und 01:00 liegen, da in diesem Zeitraum der tägliche Cronjob ausgeführt wird.'
})
}Committable suggestion skipped: line range outside the PR's diff.
| function parseTimestampAndTime(dateTimestamp, timeStr) { | ||
| const date = moment.unix(dateTimestamp); | ||
| if (!date.isValid()) return null; | ||
|
|
||
| const [hours, minutes, seconds] = timeStr.split(':').map((val, index) => parseInt(val || 0)); | ||
| return date.set({ hour: hours, minute: minutes, second: seconds }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to prevent runtime errors.
The function could throw errors if timeStr is malformed. Consider adding validation and error handling.
function parseTimestampAndTime(dateTimestamp, timeStr) {
const date = moment.unix(dateTimestamp);
if (!date.isValid()) return null;
+ if (typeof timeStr !== 'string' || !/^\d{2}:\d{2}(:\d{2})?$/.test(timeStr)) {
+ return null;
+ }
+
const [hours, minutes, seconds] = timeStr.split(':').map((val, index) => parseInt(val || 0));
+ if (hours > 23 || minutes > 59 || seconds > 59) {
+ return null;
+ }
return date.set({ hour: hours, minute: minutes, second: seconds });
}📝 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.
| function parseTimestampAndTime(dateTimestamp, timeStr) { | |
| const date = moment.unix(dateTimestamp); | |
| if (!date.isValid()) return null; | |
| const [hours, minutes, seconds] = timeStr.split(':').map((val, index) => parseInt(val || 0)); | |
| return date.set({ hour: hours, minute: minutes, second: seconds }); | |
| function parseTimestampAndTime(dateTimestamp, timeStr) { | |
| const date = moment.unix(dateTimestamp); | |
| if (!date.isValid()) return null; | |
| if (typeof timeStr !== 'string' || !/^\d{2}:\d{2}(:\d{2})?$/.test(timeStr)) { | |
| return null; | |
| } | |
| const [hours, minutes, seconds] = timeStr.split(':').map((val, index) => parseInt(val || 0)); | |
| if (hours > 23 || minutes > 59 || seconds > 59) { | |
| return null; | |
| } | |
| return date.set({ hour: hours, minute: minutes, second: seconds }); | |
| } |
| if (!moment(this.state.selectedDate).isSame(moment.unix(this.props.attributes.today), 'day')) { | ||
| return true; | ||
| } | ||
|
|
||
| const BUFFER_MINUTES = 60; // Minimum minutes needed between current time and next slot | ||
| const currentTime = moment(); | ||
| const timeToCheck = moment(time); | ||
|
|
||
| // Calculate minutes difference between times | ||
| const minutesDiff = timeToCheck.diff(currentTime, 'minutes'); | ||
|
|
||
| // Disable if time is in past or too close to current time | ||
| return minutesDiff >= BUFFER_MINUTES; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance time buffer handling.
The current implementation has several potential improvements:
- The buffer time is hardcoded and could be made configurable
- Edge cases around midnight are not handled explicitly
- The function could benefit from more detailed error messages
+ // Move buffer to class constant or props for configurability
+ static DEFAULT_BUFFER_MINUTES = 60;
+
const filterPassedTime = (time) => {
if (!moment(this.state.selectedDate).isSame(moment.unix(this.props.attributes.today), 'day')) {
return true;
}
- const BUFFER_MINUTES = 60; // Minimum minutes needed between current time and next slot
+ const bufferMinutes = this.props.bufferMinutes || AvailabilityDatePicker.DEFAULT_BUFFER_MINUTES;
const currentTime = moment();
const timeToCheck = moment(time);
- // Calculate minutes difference between times
const minutesDiff = timeToCheck.diff(currentTime, 'minutes');
- // Disable if time is in past or too close to current time
- return minutesDiff >= BUFFER_MINUTES;
+ // Handle edge case when current time + buffer crosses midnight
+ if (currentTime.clone().add(bufferMinutes, 'minutes').format('YYYY-MM-DD') !== currentTime.format('YYYY-MM-DD')) {
+ // If we're too close to midnight, disable all times for today
+ return false;
+ }
+
+ return minutesDiff >= bufferMinutes;
};Committable suggestion skipped: line range outside the PR's diff.
|
@coderabbitai what unit tests are missing for the new validations in the backend? |
|
Tip For best results, initiate chat on the files or code changes. @ThomasAFink Here are the missing unit tests that need to be added to public function testMaintenanceWindowValidation()
{
$availability = new Availability();
$testCases = [
[
'name' => '23:00 start time',
'start' => '2025-01-30 23:00',
'end' => '2025-01-31 02:00',
'expected' => true // Should have error
],
[
'name' => '23:59 end time',
'start' => '2025-01-30 22:00',
'end' => '2025-01-30 23:59',
'expected' => true // Should have error
],
[
'name' => '00:00 start time',
'start' => '2025-01-31 00:00',
'end' => '2025-01-31 02:00',
'expected' => true // Should have error
],
[
'name' => '00:59 end time',
'start' => '2025-01-30 22:00',
'end' => '2025-01-31 00:59',
'expected' => true // Should have error
],
[
'name' => 'valid time',
'start' => '2025-01-30 08:00',
'end' => '2025-01-30 22:00',
'expected' => false // Should not have error
]
];
foreach ($testCases as $case) {
$errors = $availability->validateStartTime(
new \DateTime('2025-01-30 10:00'), // today
new \DateTime('2025-01-31 10:00'), // tomorrow
new \DateTime($case['start']), // startDate
new \DateTime($case['end']), // endDate
new \DateTime('2025-01-30 10:00'), // selectedDate
'default' // kind
);
$hasError = !empty(array_filter($errors, function($error) {
return $error['type'] === 'startOfDay';
}));
$this->assertEquals(
$case['expected'],
$hasError,
"Maintenance window validation failed for {$case['name']}"
);
if ($hasError) {
$error = current(array_filter($errors, function($error) {
return $error['type'] === 'startOfDay';
}));
$this->assertStringContainsString(
'zwischen 23:00 und 01:00',
$error['message'],
"Error message should mention maintenance window for {$case['name']}"
);
$this->assertStringContainsString(
'Cronjob',
$error['message'],
"Error message should mention cronjob for {$case['name']}"
);
}
}
}
public function testTimeBufferValidation()
{
$availability = new Availability();
$now = new \DateTime('2025-01-30 17:57:00');
$testCases = [
[
'name' => 'time too close to current',
'start' => '2025-01-30 18:00',
'end' => '2025-01-30 19:00',
'expected' => true // Should have error
],
[
'name' => 'time with adequate buffer',
'start' => '2025-01-30 19:00',
'end' => '2025-01-30 20:00',
'expected' => false // Should not have error
],
[
'name' => 'time at buffer boundary',
'start' => '2025-01-30 18:02',
'end' => '2025-01-30 19:00',
'expected' => true // Should have error
]
];
foreach ($testCases as $case) {
$errors = $availability->validateStartTime(
$now, // today
new \DateTime('2025-01-31 10:00'), // tomorrow
new \DateTime($case['start']), // startDate
new \DateTime($case['end']), // endDate
$now, // selectedDate
'default' // kind
);
$hasError = !empty(array_filter($errors, function($error) {
return $error['type'] === 'timeBuffer';
}));
$this->assertEquals(
$case['expected'],
$hasError,
"Time buffer validation failed for {$case['name']}"
);
if ($hasError) {
$error = current(array_filter($errors, function($error) {
return $error['type'] === 'timeBuffer';
}));
$this->assertStringContainsString(
'zu nah',
$error['message'],
"Error message should mention time being too close for {$case['name']}"
);
}
}
}These tests cover:
Would you like me to:
✏️ Learnings added🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 17921 |
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Based on the comprehensive summary of changes across multiple files, here are the high-level release notes:
New Features
Bug Fixes
Performance Improvements
User Experience
The release focuses on making availability management more robust, with particular emphasis on preventing conflicts and providing clear feedback to users during availability creation and modification processes.