fix(ZMS-3253 ZMS-3466 ZMS-3415 ZMS-1891): Replicate frontend validation in the backend for the availability opening hours to improve data integrity and frontend validation messaging #825
Conversation
…orm error messaging
This reverts commit ce5d795.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
zmsadmin/js/page/availabilityDay/layouts/accordion.js (4)
28-31: Consider improving readability of the hasNewAvailability check.The logic is correct but could be more readable by extracting the conditions into descriptive variables.
- const hasNewAvailability = this.props.availabilityList.some(availability => - (availability?.tempId?.includes('__temp__') || - availability?.kind === 'exclusion') - ); + const hasNewAvailability = this.props.availabilityList.some(availability => { + const isTemporary = availability?.tempId?.includes('__temp__'); + const isExclusion = availability?.kind === 'exclusion'; + return isTemporary || isExclusion; + });
48-60: Enhance error and conflict checking robustness.While the changes improve safety, consider adding type checking and early returns for clearer flow control.
const hasConflict = (eventId) => { + if (!eventId || !this.props.conflictList?.itemList) return false; return ( - Object.keys(this.props.conflictList.itemList).length > 0 && - this.props.conflictList.conflictIdList.includes(eventId) + Object.keys(this.props.conflictList.itemList).length > 0 && + Array.isArray(this.props.conflictList.conflictIdList) && + this.props.conflictList.conflictIdList.includes(eventId) ) } const hasError = (eventId) => { + if (!eventId || !this.props.errorList) return false; return ( - Object.keys(this.props.errorList).length > 0 && + Object.keys(this.props.errorList).length > 0 && Object.values(this.props.errorList).find(item => item.id == eventId) ) }
178-182: Simplify error checking logic in FooterButtons.The nested error checking logic could be simplified for better maintainability.
- hasErrors={Object.values(this.props.errorList).some(error => { - const hasPastTimeError = error.itemList?.flat(2) - .some(item => item?.type === 'endTimePast'); - return !hasPastTimeError; - })} + hasErrors={Object.values(this.props.errorList).some(error => + !error.itemList?.flat(2).some(item => item?.type === 'endTimePast') + )}
177-177: Simplify boolean expression.The ternary operation for hasConflicts can be simplified.
- hasConflicts={Object.keys(this.props.conflictList.itemList).length ? true : false} + hasConflicts={Boolean(Object.keys(this.props.conflictList.itemList).length)}🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsadmin/js/page/availabilityDay/layouts/accordion.js(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
zmsadmin/js/page/availabilityDay/layouts/accordion.js
[error] 177-177: 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 (8)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (1)
zmsadmin/js/page/availabilityDay/layouts/accordion.js (1)
1-15: LGTM! Clean import statements and class declaration.The formatting changes improve code readability while maintaining functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (1)
112-138: 🛠️ Refactor suggestionOptimize nested loop in getFilteredAvailabilityList.
The current implementation has O(n²) complexity due to nested loops. Consider pre-filtering future availabilities to improve performance.
private static function getFilteredAvailabilityList( AvailabilityList $availabilityList, AvailabilityList $futureAvailabilityList, bool $hasExclusionSplit, ?string $originId ) { $filteredAvailabilityList = new AvailabilityList(); + + // Pre-filter future availabilities + $relevantFutureAvailabilities = new AvailabilityList(); + if (!$hasExclusionSplit || !$originId) { + $relevantFutureAvailabilities = $futureAvailabilityList; + } else { + foreach ($futureAvailabilityList as $futureAvailability) { + if (!isset($futureAvailability->id) || $futureAvailability->id !== $originId) { + $relevantFutureAvailabilities->addEntity($futureAvailability); + } + } + } foreach ($availabilityList as $availability) { - $isSpecialKind = isset($availability->kind) && - in_array($availability->kind, ['origin', 'exclusion', 'future']); - - foreach ($futureAvailabilityList as $futureAvailability) { - if ( - !$isSpecialKind || !$hasExclusionSplit || - !isset($futureAvailability->id) || - $futureAvailability->id !== $originId - ) { - $filteredAvailabilityList->addEntity($futureAvailability); - } - } + // Add pre-filtered future availabilities + foreach ($relevantFutureAvailabilities as $futureAvailability) { + $filteredAvailabilityList->addEntity($futureAvailability); + } $filteredAvailabilityList->addEntity($availability); } return $filteredAvailabilityList; }
🧹 Nitpick comments (6)
zmsapi/src/Zmsapi/AvailabilityAdd.php (4)
23-25: Consider reducing class coupling through dependency injection.The
@SuppressWarnings(Coupling)annotation indicates high coupling. Consider:
- Injecting dependencies (repositories, validators) through constructor
- Extracting conflict detection logic into a separate service
- Using interfaces to define contracts between components
54-65: Enhance input validation with type checking.While the structure validation is good, consider adding type validation for critical fields:
private function validateInputStructure(array $input): void { if (!isset($input['availabilityList']) || !is_array($input['availabilityList'])) { throw new BadRequestException('Missing or invalid availabilityList.'); } if (empty($input['availabilityList']) || !isset($input['availabilityList'][0]['scope'])) { throw new BadRequestException('Missing or invalid scope.'); } - if (!isset($input['selectedDate'])) { + if (!isset($input['selectedDate']) || !strtotime($input['selectedDate'])) { throw new BadRequestException("'selectedDate' is required."); } + if (isset($input['availabilityList'][0]['scope']) && + !is_array($input['availabilityList'][0]['scope'])) { + throw new BadRequestException("'scope' must be an array."); + } }
314-328: Improve caching strategy for responses.The current implementation uses a static timestamp for caching. Consider:
- Using the latest modification time from the updated entities
- Adding ETag support for better cache validation
private function generateResponse( RequestInterface $request, ResponseInterface $response, Collection $updatedCollection ): ResponseInterface { $message = Response\Message::create($request); $message->data = $updatedCollection->getArrayCopy(); - $response = Render::withLastModified($response, time(), '0'); + $lastModified = max(array_map( + fn($entity) => $entity->lastChange ?? time(), + $updatedCollection->getArrayCopy() + )); + $etag = md5(json_encode($message->data)); + $response = Render::withLastModified($response, $lastModified, $etag); return Render::withJson( $response, $message->setUpdatedMetaData(), $message->getStatuscode() ); }
330-349: Add type hints for better code clarity.Consider adding PHP 7.4+ type hints to improve code clarity and catch type-related issues early:
- protected function writeEntityUpdate($entity, $resolveReferences): Entity + protected function writeEntityUpdate(Entity $entity, int $resolveReferences): Entity { $repository = new AvailabilityRepository(); - $updatedEntity = null; + $updatedEntity = null; if ($entity->id) { $oldEntity = $repository->readEntity($entity->id); if ($oldEntity !== null && $oldEntity->hasId()) { $this->writeSpontaneousEntity($oldEntity); $updatedEntity = $repository->updateEntity($entity->id, $entity, $resolveReferences); } else { throw new AvailabilityAddFailed("Entity with ID {$entity->id} not found."); } } else { $updatedEntity = $repository->writeEntity($entity, resolveReferences: $resolveReferences); } if (!$updatedEntity) { throw new AvailabilityAddFailed(); } return $updatedEntity; }zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (1)
214-227: Clarify PHPMD Suppression by Documenting Indirect UsageWe've confirmed that
sortConflictedList()is indeed used indirectly as a callback in theusortcall. The PHPMD suppression is therefore justified. However, to improve clarity for future maintainers, consider updating the docblock.- /** @SuppressWarnings(PHPMD.UnusedPrivateMethod) */ - private static function sortConflictedList($a, $b) + /** + * Used as a callback for usort to sort the conflicted list. + * Suppressing PHPMD.UnusedPrivateMethod because the indirect callback usage is not detected. + * @SuppressWarnings(PHPMD.UnusedPrivateMethod) + */ + private static function sortConflictedList($a, $b)zmsapi/src/Zmsapi/AvailabilityUpdate.php (1)
114-137: Consider extracting conflict validation thresholds to configuration.The conflict validation logic is well-structured, but the validation thresholds are hardcoded. Consider extracting these to configuration for easier maintenance and customization.
+ private const CONFLICT_VALIDATION_CONFIG = [ + 'MIN_LOYALTY_YEARS' => 3, + 'MAX_RETRIES' => 3, + 'CONFLICT_THRESHOLD' => 0 + ]; + private function validateAndCheckConflicts( Collection $newCollection, Scope $scope, DateTimeImmutable $selectedDate ): void { - if ($filteredConflicts->count() > 0) { + if ($filteredConflicts->count() > self::CONFLICT_VALIDATION_CONFIG['CONFLICT_THRESHOLD']) { throw new AvailabilityUpdateFailed(); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(3 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(1 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
zmsapi/src/Zmsapi/AvailabilityAdd.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-17T10:13:48.667Z
Learning: The readResponse() method in AvailabilityAdd.php should be refactored to reduce complexity by:
1. Separating input validation into dedicated methods
2. Breaking down collection processing logic
3. Isolating conflict detection and filtering
4. Moving entity update logic to separate methods
5. Using type hints for better code clarity
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsclient-test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (3)
zmsapi/src/Zmsapi/AvailabilityAdd.php (1)
122-124: Enhance error handling with detailed failure messages.The
AvailabilityAddFailedexception should include information about the specific conflicts that caused the failure.zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (1)
45-56: LGTM! Input validation is thorough and well-structured.The validation logic effectively checks for required fields and their correct formats, throwing appropriate exceptions with clear error messages.
zmsapi/src/Zmsapi/AvailabilityUpdate.php (1)
39-61: LGTM! Input validation is comprehensive and well-structured.The validation logic effectively checks for required fields and their correct formats, throwing appropriate exceptions with clear error messages. The separation into
validateAndGetInputandvalidateInputStructuremethods improves code organization.
…ening-hours-of-the-same-appointment-type-must-not-overlap
WalkthroughThis pull request introduces extensive updates across both frontend and backend layers of the availability management system. Frontend React components have been modified to improve conflict handling, error rendering, and validation logic. Backend PHP classes and API endpoints now incorporate comprehensive input validation, refined conflict resolution, and updated exception handling. The JSON schemas and fixtures have been updated to change the appointment slot time from 12 to 10 minutes, and new Twig templates enhance error messaging. Additional tests have been added to validate various edge cases and ensure robustness in the conflict and availability logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant Validator
participant DB
User->>UI: Click "Save" (trigger availability update)
UI->>API: POST availability data
API->>Validator: Validate input & check conflicts
Validator-->>API: Return validated data or errors
API->>DB: Process update (insert/update availability)
DB-->>API: Confirm persistence
API-->>UI: Send success/error response
UI->>User: Update UI based on response
sequenceDiagram
participant User
participant ScopeView
participant TableBody
participant Board
User->>ScopeView: Request availability for scope
ScopeView->>TableBody: Render availability list
TableBody->>Board: Populate data into layout
Board-->>ScopeView: Return completed view
ScopeView-->>User: Display updated scope view
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
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
zmsadmin/js/page/availabilityDay/layouts/tableBody.js (1)
36-79: 🛠️ Refactor suggestionPrevent variable leakage in switch cases.
Variables declared in switch cases could leak into other cases. Wrap each case in a block to prevent this.
Apply this diff:
switch (sortColumn) { case 'weekday': { const aWeekDay = Object.keys(a.weekday).find(key => parseInt(a.weekday[key], 10) > 0) || ''; const bWeekDay = Object.keys(b.weekday).find(key => parseInt(b.weekday[key], 10) > 0) || ''; return multiplier * aWeekDay.localeCompare(bWeekDay); } case 'series': { const aRepeat = availabilitySeries.find(element => element.value == repeat(a.repeat))?.name || ''; const bRepeat = availabilitySeries.find(element => element.value == repeat(b.repeat))?.name || ''; return multiplier * aRepeat.localeCompare(bRepeat); } // ... wrap remaining cases in blocks similarly🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 39-39: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 43-43: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 44-44: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 57-57: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 58-58: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 70-70: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 71-71: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🧹 Nitpick comments (41)
zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (3)
16-27: Consider factoring out date/time setup for parameterized tests.
Currently, the date calculation and the$weekdaylogic are repeated patterns that might be refactored into a data provider or utility method, making tests more concise and maintainable.
35-50: Extract large array structures into a helper method.
This block creates a big, nested array forscopeanddayoffdata. Consider extracting it into a dedicated helper to simplify test readability and foster reusability across multiple test methods.
97-105: Robustly assert exception details.
The negative time range test correctly expectsAvailabilityUpdateFailed. Consider asserting the exception message or code as well, to ensure that the correct error path is triggered.zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (2)
112-138: Nested loops may lead to performance concerns.
When either the$availabilityListor$futureAvailabilityListgrows large, this O(n²) approach might degrade performance. Consider more efficient data structures or pre-filtering strategies if large datasets are expected.
214-227: Clarify PHPMD suppression.
BecausesortConflictedListis used as a callback inusort, PHPMD doesn’t detect it as invoked. Adding a note in the docblock explaining its usage as a callback can help future maintainers.zmsapi/tests/Zmsapi/AvailabilityAddTest.php (3)
62-119: Validate error messages in overlapping availability tests.
These tests correctly expectAvailabilityAddFailed. You might also verify the exception message or response body to ensure you’re catching the specific overlap-related error.
121-178: Duplicate overlapping availability test coverage is solid.
This new test method comprehensively checks for collision scenarios. Similar to the previous test, consider verifying additional error details for clarity.
180-201: Check edge cases for invalid times.
Testing an end time earlier than the start time is good. Extending coverage to handle edge cases (e.g., start == end) might further strengthen time-based logic validation.zmsapi/src/Zmsapi/AvailabilityAdd.php (2)
43-52: Provide more descriptive error messages for BadRequestException.Currently, a generic
BadRequestException()is thrown without specifying the exact reason. A more descriptive message would make debugging and user feedback easier.- throw new BadRequestException(); + throw new BadRequestException('No valid input data found or input structure is empty.');
330-349: Return a more descriptive error or roll back transactional changes.When
$updatedEntityis not returned, the code throwsAvailabilityAddFailedwithout details. Including contextual information (e.g., which entity failed) helps troubleshooting. Also consider rolling back changes if only part of the updates succeed.- if (!$updatedEntity) { - throw new AvailabilityAddFailed(); - } + if (!$updatedEntity) { + throw new AvailabilityAddFailed( + sprintf('Failed to write entity (ID: %s).', $entity->id ?? 'N/A') + ); + }zmsadmin/js/page/availabilityDay/helpers.js (2)
86-92: Consider a more flexible rounding approach.Currently, the function rounds only in multiples of 5 or 10. If slot sizes change in the future, you might want to generalize this logic to handle any integer slot.
94-164: Refactor overlap detection for clarity.The overlap logic in
getNewAvailabilityis well-intentioned but quite complex. Extracting the overlap checks into a dedicated helper function could improve readability and simplify future maintenance.zmsadmin/js/page/availabilityDay/form/validate.js (1)
30-30: Simplify the boolean expression.Instead of using a ternary with boolean literals, use a direct assignment to make the code more concise and clear.
- let valid = (0 < errorList.itemList.length) ? false : true; + let valid = errorList.itemList.length === 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
zmsapi/src/Zmsapi/AvailabilityUpdate.php (2)
29-37: Method chaining is concise.
The sequence of retrieving input, establishing a connection, processing, and generating a response forms a clear flow. Consider adding high-level try-catch if you want to handle exceptions more granularly here.
114-137: Conflict detection approach is robust.
Collecting conflicts from new availability and cross-checking them with existing data. Consider adding a descriptive message toAvailabilityUpdateFailed()for clearer debugging.zmsadmin/js/page/availabilityDay/index.js (2)
145-205: Avoid the delete operator for tempId.
Usingdelete sendAvailability.tempId;can degrade performance in some JavaScript engines. Prefer setting it toundefinedinstead:- delete sendAvailability.tempId; + sendAvailability.tempId = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 160-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
230-282: Same delete operator concern.
Again, consider using assignment toundefinedin place ofdelete.- delete sendAvailability.tempId; + sendAvailability.tempId = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 238-239: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
zmsentities/src/Zmsentities/Availability.php (4)
116-116: Consider clarifying the docblock type.
The parameter is documented asstringwhile also mentioningfalse. Clarify asstring|boolor refactor the default if you intend to allow a boolean value.
141-141: Same docblock type issue.
Convey that$typecan accept a boolean if you truly allowfalseto ignore the type.
252-253: Consider using cloned DateTime objects.
Modifying$dateTimeand$startin-place may cause unintended side effects if these variables are needed elsewhere. Cloning them before calling->modify()avoids potential data corruption.
458-497: Consider renaming the'startOfDay'error type.
It flags maintenance window violations, not strictly start-of-day constraints. A name like'maintenanceWindow'might better reflect its meaning.zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityupdatefailed.twig (1)
1-7: LGTM! Consider extracting text to translation files.The template provides clear error messaging for availability update failures. However, the hardcoded German text could be moved to translation files for better maintainability and internationalization support.
Consider extracting the German text to translation files:
- "message":"Zu den angegebenen Daten konnte keine Öffnungszeit geändert werden. Bitte überprüfen Sie Ihre Eingaben und korrigieren Sie diese gegebenenfalls.", - "title":"Konflikt: Öffnungszeit überschneidet sich mit einer bestehenden Zeit", + "message": {{ 'availability.update.failed.message'|trans }}, + "title": {{ 'availability.update.failed.title'|trans }},zmsadmin/js/page/availabilityDay/form/errors.js (1)
21-25: Consider enhancing error message accessibility.While the error messages are properly rendered, consider adding ARIA attributes to improve screen reader support.
- <div key={`${index}-${nestedIndex}`}>{nestedItem.message}</div> + <div key={`${index}-${nestedIndex}`} role="alert" aria-atomic="true">{nestedItem.message}</div>zmsadmin/js/page/availabilityDay/timetable/scopeview.js (1)
11-34: Add default values for required props.The component requires
onDelete,onSelect, andonAbortprops but doesn't provide default values, which could lead to runtime errors if not provided.Consider adding default no-op functions:
+const noop = () => {}; + const ScopeView = (props) => { const { onDelete, onSelect, onAbort, timestamp, availabilityList, data } = props // ... } + +ScopeView.defaultProps = { + onDelete: noop, + onSelect: noop, + onAbort: noop +}zmsadmin/templates/page/availabilityday.twig (1)
17-19: Track disabled feature in issue system.The commented-out "Alle Öffnungszeiten" tab is marked as "Temporarily disabled" but lacks tracking information.
Would you like me to create an issue to track the re-enablement of this feature? This will help ensure the temporary change doesn't become permanent.
zmsadmin/js/page/availabilityDay/saveBar.js (2)
13-21: Consider using a more descriptive state variable name.The state variable
isVisiblecould be more specific, likeisMessageVisibleorshowSaveBar, to better indicate its purpose in the component.
25-35: Consider adding i18n support for messages.The hardcoded German text strings could be moved to a translation file for better maintainability and future internationalization support.
Example implementation:
- return props.success - ? <b><i className="fas fa-check-circle" aria-hidden="true" aria-label="Erfolg"></i> Öffnungszeit gelöscht, {formatDate(props.lastSave)}</b> - : <b><i className="fas fa-times-circle" aria-hidden="true" aria-label="Fehler"></i> Fehler beim Löschen der Öffnungszeit. Bitte versuchen Sie es erneut.</b> + return props.success + ? <b><i className="fas fa-check-circle" aria-hidden="true" aria-label={t('success')}></i> {t('availability.deleted')}, {formatDate(props.lastSave)}</b> + : <b><i className="fas fa-times-circle" aria-hidden="true" aria-label={t('error')}></i> {t('availability.delete_error')}</b>zmsadmin/src/Zmsadmin/AvailabilityListByScope.php (1)
57-65: Consider adding logging for debugging.Add logging statements to track exceptions and aid in debugging.
} catch (\BO\Zmsclient\Exception $exception) { + \App::$log->debug('Availability exception: ' . $exception->getMessage()); if ($exception->template == 'BO\Zmsapi\Exception\Availability\AvailabilityNotFound') { + \App::$log->info('No availability found for scope: ' . $scopeId); return \BO\Slim\Render::withJson($response, [ 'meta' => [], 'data' => [] ]); } + \App::$log->error('Unhandled availability exception: ' . $exception->getMessage()); throw $exception; }zmsadmin/js/page/availabilityDay/layouts/tableBody.js (1)
214-214: Use optional chaining for cleaner null checks.The availability type check can be simplified using optional chaining.
Apply this diff:
- <td>{availabilityType && availabilityType.name ? availabilityType.name : ""}</td> + <td>{availabilityType?.name ?? ""}</td>🧰 Tools
🪛 Biome (1.9.4)
[error] 214-214: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
zmsdb/src/Zmsdb/Process.php (1)
495-495: Method name change improves clarity.The change from
getConflictstocheckAllVsExistingConflictsbetter describes the method's functionality. Consider adding a PHPDoc comment to document the expected behavior of this method.+ /** + * Checks all availabilities against existing conflicts + * + * @param \DateTimeInterface $startDate Start date for conflict check + * @param \DateTimeInterface $endDate End date for conflict check + * @return \BO\Zmsentities\Collection\ProcessList List of conflicts found + */ $conflictList = $availabilityList->checkAllVsExistingConflicts($startDate, $endDate);zmsadmin/js/page/availabilityDay/layouts/accordion.js (7)
64-66: Assignment of Temporary IDs for AvailabilitiesWhen an availability object lacks both an
idandtempId, a temporary ID is assigned using the index. This ensures that each item can be uniquely identified in the UI. Consider adding a brief inline comment to explain why this is necessary for future maintainers.
72-80: Review of theonToggleHandlerThe
onTogglefunction manages the expansion state based on the element’s custom attribute. Two suggestions:
- Use strict equality (
===) for comparisons betweeneventIdand the attribute value to avoid any unintended type coercion.- Rename the custom attribute from
eventkeytodata-eventkeyto better align with HTML standards for custom data attributes.
115-119: Grouping Conflicts inconflictListThe logic for grouping conflicts by date works as intended. For improved readability and future maintenance, consider refactoring this block (perhaps using a
reduceor a helper function) to encapsulate the grouping behavior.
124-127: Building theerrorListCurrently, the code iterates over
this.props.errorListand pushes matching items toerrorList. Using thefilterarray method might be a more declarative and concise way to achieve the same result.
130-130: Conditional Inline Styling for Error/Conflict StateThe inline style correctly applies a red border when either conflicts or errors are detected. If this styling were to be reused elsewhere, consider moving it to a CSS class.
131-137: Custom Attribute Usage in the Button ElementThe
<button>element uses a custom attributeeventkeyto pass the identifier to the click handler. Since custom attributes should ideally be prefixed withdata-, consider renaming it todata-eventkeyand updating the corresponding access in theonTogglehandler.
166-184: Review ofBoardComponent Integration and FooterButtons PropsSeveral points to note:
hasConflictsProp: Using a ternary operator (...? true : false) to convert the length to a boolean can be simplified. You might replace it with a double negation (e.g.,!!Object.keys(this.props.conflictList.itemList).length).hasErrorsLogic: The inline function checking for past time errors is robust but slightly complex; consider extracting this logic into a helper function for clarity.availabilitylistProp: The renamed prop (availabilitylistin lowercase) should be reviewed to ensure consistency with the expected prop name in<FooterButtons>.🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: 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/formButtons.js (3)
20-21: Consider improving readability of the disabled condition.The disabled condition combines multiple checks in a single line, making it harder to read and maintain.
Consider breaking down the condition for better readability:
- const disabled = ((data && (!data.id || data.__modified === true)) || hasConflicts || hasErrors || hasSlotCountError); + const isDataModified = data && (!data.id || data.__modified === true); + const hasValidationErrors = hasConflicts || hasErrors || hasSlotCountError; + const disabled = isDataModified || hasValidationErrors;
49-57: Consider simplifying the update button's disabled condition.The disabled condition for the update button has multiple checks that could be simplified using previously defined variables.
Consider reusing the existing variables:
- disabled={ - (data && !data.id) || - !data.__modified || - hasConflicts || - hasErrors || - hasSlotCountError || - isCreatingExclusion || - isPastDate - } + disabled={!data?.id || !data.__modified || disabled || isCreatingExclusion || isPastDate}
66-78: Consider making selectedDate a required prop.Since
selectedDateis used to determineisPastDate, which affects button states, it should be marked as required to prevent potential runtime errors.- selectedDate: PropTypes.number, + selectedDate: PropTypes.number.isRequired,zmsadmin/js/page/availabilityDay/form/footerButtons.js (1)
46-74: Consider simplifying button disabled conditions.The disabled conditions are complex and could be made more readable.
Consider extracting the conditions into named variables:
+ const canModify = !isPastDate; + const hasChanges = stateChanged || hasNewAvailabilities; + const hasAnyErrors = hasSlotCountError || hasConflicts || hasErrors; + return ( <div className="form-actions" style={{ "marginTop": "0", "padding": "0.75em" }}> <button title="Neue Öffnungszeit anlegen und bearbeiten" className="button button--diamond button-new" onClick={onNew} - disabled={data || hasConflicts || hasSplitInProgress || isPastDate} + disabled={data || hasConflicts || hasSplitInProgress || !canModify} > neue Öffnungszeit </button> <button title="Alle Änderungen werden zurückgesetzt" className="button btn" type="abort" onClick={onAbort} - disabled={(!stateChanged && !hasNewAvailabilities && !hasConflicts && !hasErrors) || isPastDate} + disabled={(!hasChanges && !hasConflicts && !hasErrors) || !canModify} > Abbrechen </button> <button title="Alle Änderungen werden gespeichert" className="button button--positive button-save" type="save" value="publish" onClick={onPublish} - disabled={(!stateChanged && !hasNewAvailabilities) || hasSlotCountError || hasConflicts || hasErrors || isPastDate} + disabled={!hasChanges || hasAnyErrors || !canModify} > Alle Änderungen aktivieren </button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
zmsadmin/package-lock.jsonis excluded by!**/package-lock.jsonzmsadmin/src/Zmsadmin/debug.logis excluded by!**/*.logzmscalldisplay/package-lock.jsonis excluded by!**/package-lock.jsonzmsstatistic/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
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(10 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(5 hunks)zmsadmin/js/page/availabilityDay/helpers.js(7 hunks)zmsadmin/js/page/availabilityDay/index.js(21 hunks)zmsadmin/js/page/availabilityDay/layouts/accordion.js(6 hunks)zmsadmin/js/page/availabilityDay/layouts/page.js(2 hunks)zmsadmin/js/page/availabilityDay/layouts/tableBody.js(5 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/js/page/availabilityDay/timetable/scopeview.js(1 hunks)zmsadmin/routing.php(16 hunks)zmsadmin/src/Zmsadmin/AvailabilityConflicts.php(3 hunks)zmsadmin/src/Zmsadmin/AvailabilityListByScope.php(1 hunks)zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityaddfailed.twig(1 hunks)zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityupdatefailed.twig(1 hunks)zmsadmin/templates/page/availabilityday.twig(1 hunks)zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php(6 hunks)zmsadmin/tests/Zmsadmin/fixtures/GET_Workstation_Resolved2.json(1 hunks)zmsadmin/tests/Zmsadmin/fixtures/GET_Workstation_scope_141_multipleSlotsEnabled.json(1 hunks)zmsadmin/tests/Zmsadmin/fixtures/GET_scope_141.json(1 hunks)zmsadmin/tests/Zmsadmin/fixtures/GET_scope_141_multipleSlotsEnabled.json(1 hunks)zmsadmin/tests/Zmsadmin/fixtures/GET_scope_list.json(1 hunks)zmsapi/src/Zmsapi/AvailabilityAdd.php(1 hunks)zmsapi/src/Zmsapi/AvailabilityUpdate.php(1 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/schema/dereferenced/appointment.json(1 hunks)zmsentities/schema/dereferenced/availability.json(1 hunks)zmsentities/src/Zmsentities/Availability.php(11 hunks)zmsentities/src/Zmsentities/Collection/AvailabilityList.php(2 hunks)zmsentities/tests/Zmsentities/AvailabilityTest.php(9 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
🧰 Additional context used
🧠 Learnings (5)
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/saveBar.js (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-04T15:26:27.753Z
Learning: When implementing SaveBar functionality in React components, centralize the TTL value as a constant in the SaveBar component and export it for reuse. This ensures consistent timing across all operations that show SaveBar messages.
zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (2)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-17T09:47:44.330Z
Learning: The getAvailabilityData() method in AvailabilityConflicts.php should be refactored into smaller methods: validateInput(), processAvailabilityKinds(), getConflictList(), and filterAndSortConflicts() to improve maintainability and reduce complexity.
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-17T09:49:29.414Z
Learning: The getAvailabilityData() method in AvailabilityConflicts.php should be refactored to reduce complexity by:
1. Extracting validation logic into separate methods
2. Breaking down availability kind processing
3. Isolating conflict filtering logic
4. Moving sorting to dedicated methods
5. Using early returns to avoid nested conditions
zmsadmin/js/page/availabilityDay/index.js (2)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-01-30T15:35:15.938Z
Learning: In the eappointment system's handleChange method, when validating form changes, run getValidationList and getConflictList in parallel using Promise.all, and only proceed with readCalculatedAvailabilityList after both validations complete to avoid race conditions.
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.
zmsapi/src/Zmsapi/AvailabilityUpdate.php (1)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-02-17T10:19:41.023Z
Learning: The readResponse() method in AvailabilityUpdate.php should be refactored to reduce complexity by:
1. Separating input validation into dedicated methods
2. Breaking down collection processing logic
3. Isolating conflict detection and filtering
4. Moving entity update logic to separate methods
5. Using type hints for better code clarity
🪛 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/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] 24-24: 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
[error] 30-30: 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] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 160-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 238-239: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
zmsadmin/js/page/availabilityDay/layouts/tableBody.js
[error] 38-38: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 39-39: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 43-43: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 44-44: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 57-57: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 58-58: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 70-70: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 71-71: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 214-214: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
zmsadmin/js/page/availabilityDay/layouts/accordion.js
[error] 172-172: 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 (4)
- GitHub Check: call-owasp-security-checks / zap-scan (zmscitizenapi, http://localhost:8080/citizen-api)
- GitHub Check: call-owasp-security-checks / zap-scan (zmsapi, http://localhost:8080/api)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (157)
zmsapi/tests/Zmsapi/AvailabilityUpdateTest.php (1)
5-7: Imports are consistent and in use.
These imports correctly reference classes used within the tests (particularlyAvailabilityUpdateFailed). No immediate issues found.zmsadmin/src/Zmsadmin/AvailabilityConflicts.php (4)
26-43: Good extraction of logic into smaller methods.
The approach of delegating input validation and conflict analysis to separate helper methods greatly enhances readability and maintainability, aligning with prior refactoring advice.
45-56: Extend input validation if needed.
While current checks foravailabilityListandselectedDateare appropriate, consider whether additional validation (e.g., checking presence/format of times) is required for this controller’s responsibilities.
58-75: Multiple 'origin' entries can overwrite $originId.
If the list unexpectedly contains multiple'origin'availabilities, the last one processed will override$originId. Confirm whether this scenario is valid or should be handled explicitly (e.g., erroring out).
120-135: Optimize nested loop in getFilteredAvailabilityList.
This closely mirrors an earlier comment about reducing nested loops to improve performance.zmsapi/tests/Zmsapi/AvailabilityAddTest.php (2)
5-6: New import usage looks correct.
AvailabilityAddFailedis used in the newly added test methods. No concerns here.
220-236: Rename from testUpdateFailed to testAddFailed aligns with usage.
The change accurately reflects the code under test. The new exception expectation is consistent with the refactored logic.zmsapi/src/Zmsapi/AvailabilityAdd.php (3)
20-20: No issues observed.
33-41: Consider wrapping database operations in a transaction.This block retrieves the write connection but doesn't begin or commit a transaction. Previously, reviewers have also suggested adding explicit transaction handling to avoid partial updates if an exception is thrown.
122-125: Include conflict details in the exception message.Throwing
AvailabilityAddFailedwithout specifying which conflicts were found makes troubleshooting harder. A prior review comment already addressed this, so consider adding details to improve debuggability.zmsadmin/js/page/availabilityDay/helpers.js (3)
3-17: Improved state merging logic looks good.Allowing an
existingStateparameter helps preserve save-related data on re-renders. This is a sensible approach and aligns with React best practices for merging state.
54-55: Verify potential redundant triggers of stateChanged.Line 55 sets
stateChanged: truewithin the mapping operation, and line 62 does it again. Confirm whether triggering state changes multiple times in the same flow is intentional or if you can unify it into a single assignment to avoid unnecessary re-renders.
83-83: No issues found.Defaulting
saveTypeto'save'seems appropriate for the initial use case.zmsadmin/js/page/availabilityDay/form/validate.js (1)
345-363: Verify slotTime non-zero to avoid potential division by zero errors.Before performing the modulo check, ensure
data.slotTimeInMinutesis non-zero. Large or zero values could cause unintended behavior or errors.- if (totalMinutes % slotTime > 0) { + if (!slotTime || (totalMinutes % slotTime !== 0)) { errorList.push({ type: 'slotCount', message: 'Zeitschlitze müssen sich gleichmäßig in der Öffnungszeit aufteilen lassen.' }) }zmsapi/src/Zmsapi/AvailabilityUpdate.php (23)
10-12: Imports look good.
These imports appropriately organize the references toCollection,ProcessList, andScope. No issues found.
16-18: Specialized exception imports are appropriate.
Bringing in domain-specific exceptions clarifies error handling.
39-48: Input validation logic is well-structured.
Ensuring the request body is JSON and not empty handles basic input integrity.
50-61: Comprehensive structure checks.
The code checks foravailabilityList,scope, andselectedDatefields, throwing specificBadRequestExceptions if missing.
63-66: Resolving references.
FetchingresolveReferencesfrom query parameters with a default is helpful.
68-77: Process workflow is well-defined.
Creating, validating, and then updating entities is coherent. Exceptions inside sub-methods handle failures.
79-96: Entity creation and validation.
Constructing entities, runningtestValid(), and optionally validating existing entities ensures data consistency.
98-104: Existing entity validation.
Reading and checking the persisted entity before proceeding helps avoid referencing non-existent data.
106-112: DateTime creation.
Appending00:00:00to the selected date is a straightforward approach to unify the daily scope.
139-145: Gathering conflicts from newly added availabilities.
The code centralizes new vs. new conflict logic in one place. Nicely done.
147-157: Merged collections.
Retrieving existing availability by scope is helpful for bridging new and old data.
159-176: Validating newly added entries.
Merging new availabilities back into the collection ensures subsequent validations encompass all changes.
178-201: Per-item validation strategy looks good.
Extracting timestamps and callingvalidateInputs()centralizes the checks in the collection. Watch out for null timestamps, but presumably they're handled by the underlying validations.
[approve]
203-214: Filtered collection logic is succinct.
Using an origin ID to exclude or include certain availabilities is clear.
216-228: Origin ID retrieval.
Short-circuiting once the origin is found is efficient.
230-234: Inclusion checks.
Checks forexclusionand ID matching the origin allow more flexible handling of specialized availabilities.
236-249: Existing conflicts detection.
Capturing the earliest start and latest end date helps unify conflict checks. Good approach.
251-271: Filtering conflicts by weekday.
Partitioning conflicts by day ensures the final list is relevant to the selected date’s weekday.
273-288: Finding matching availability.
Matching based onidortempIdis helpful in identifying overlapping entries.
290-313: Weekday conflict logic.
Using numeric weekday to map to the string form is intuitive. Condition checks for> 0are self-explanatory.
315-324: Batch updating new collection.
Using a separate method to update entities in bulk fosters clarity.
326-340: Response generation.
Populating$message->datawith the updated collection and returning JSON is consistent with typical patterns.
342-364: Enhance error handling in writeEntityUpdate.
This code mirrors a past recommendation to improve error messages for edge cases.zmsentities/src/Zmsentities/Collection/AvailabilityList.php (4)
22-22: Synchronized approach to workstation counts.
This single-line check prevents overshadowing the max count.
165-193: Robust input validation method.
Looping over each availability and applying multiple validation checks is comprehensive. Logged warnings for invalid data are helpful as well.
195-234: DateTime range extraction.
Adjusting the start date if it's prior to the selected date ensures consistent bounding. The logic is clear and adequately documented.
236-290: New-vs-new conflict detection.
Nested iterating is a straightforward solution. While it might grow costly for very large sets, it's acceptable for typical batch sizes.zmsadmin/js/page/availabilityDay/index.js (29)
10-10: New ScopeView import.
The import seamlessly integrates a new view component.
42-45: Storing a fullAvailabilityList in state.
This provides a convenient reference to comprehensive scope data without cluttering the primary availability list.
58-83: Fetching all scope availabilities.
Performing a GET for the next year and then triggering local validation upon success is a pragmatic approach.
85-87: Utility for scrolling.
Simple but useful. No issues here.
116-130: State update helper.
Merging new props and reapplying slot calculations to the availability list is neatly encapsulated.
138-138: Refresh data callback.
Good approach to unify the updated results in one place.
210-226: Revert updates logic.
Clearing errors, refreshing data, and re-fetching conflicts ensures a full reset.
284-290: Track save bar state.
This helps surface feedback to the user. No issues found.
303-309: Filtering the fullAvailabilityList.
Neatly removing the deleted item from state.
310-316: Quick re-check of conflicts and validations.
Triggering them upon changes fosters data consistency.
396-401: Setting an exclusion flow.
The logic for decidingendDateTimestampis consistent with prior usage.
406-407: Removing keywords from the name.
Proactive string replacement is a straightforward approach to keep labels consistent.
408-414: Origin availability creation.
UsingeditExclusionAvailabilitywith an'origin'kind helps differentiate updated openings from the baseline.
419-419: Conditional description update.
Allows more dynamic naming based on whether the availability starts on the selected day.
424-424: Exclusion entry creation.
Splitting logic for partial coverage is consistent with the ‘origin/future’ approach.
448-450: Merging and setting state.
Props for conflict detection and final validations are triggered in the callback.Also applies to: 455-455
462-464: Future editing boundary.
Determining a cutoff date fororiginAvailabilityis handled similarly here.
468-468: Apply transformations consistently.
UsingeditExclusionAvailabilityensures each subset is updated with the correct kind and timeframe.Also applies to: 490-493, 496-497
502-527: Introducing a new availability.
Creating a default “appointment” type and storing it in both lists ensures consistent references.
561-580: Regex-based validation for date & time.
Rejecting bad timestamps and times at this stage prevents broken availability entries from persisting.
582-626: Centralized validation logic.
Combining a localvalidate(...)call with the final list simplifies referencing any discovered errors.
628-670: Asynchronous conflict retrieval.
Returns a promise, allowing the caller to chain further logic or handle errors.
682-698: Dynamic views by tab.
Showing either a graph, scope, or table view is straightforward. The fallback isTableView.
715-766: Slot recalculation.
ChaininggetValidationListresult before fetching slots is a good guard to avoid stale states.
768-795: Conditional re-validation on handleChange.
Only re-checking when data is actually modified helps prevent redundant validations.
805-809: Exclusion handling.
Logic fororigin,exclusion, andfuturechanges is well compartmentalized.
915-949: Accordion layout bridging.
The event handlers align well with the features we’ve seen: copy, new, exclusion, future edits, and single updates.
942-948: Conflict list fallback.
Using an empty fallback object ensures the AcccordionLayout doesn’t break if the conflict data is missing.
955-963: SaveBar rendering logic.
Conditionally rendering based onlastSaveis neat.zmsentities/src/Zmsentities/Availability.php (15)
284-284: No issues found.
This usage ofmodify('today ...')is consistent with your approach of normalizing the date portion.
299-299: No issues found.
Same rationale as line 284.
313-313: No issues found.
Returning day-difference via"%a"is acceptable for the intended usage.
442-442: No issues found.
Adding theboolreturn type enhances clarity.
499-525: No issues found.
The end-time checks look logically consistent, ensuring end time is after start time.
527-553: No issues found.
Properly validating origin end time for temporal constraints is well-handled.
555-565: No issues found.
The simple type check is consistent with your new validation approach.
567-597: No issues found.
Ensures slot time is nonzero and divides available minutes evenly.
599-610: No issues found.
The range check is straightforward, ensuringstartInDays <= endInDays.
649-655: No issues found.
These lines correctly compare weekday flags to identify mismatches.
662-669: Potential confusion in shared weekday condition.
The condition uses!=for each weekday but is combined with&&, which inverts the typical "shared day" check. Verify if this logic truly reflects your intent for "hasSharedWeekdayWith."
672-737: No issues found.
Validation ensures at least one weekday is selected and that it appears in the date range.
852-871: No issues found.
Provides a concise and user-friendly list of selected German weekday names.
881-929: No issues found.
Robust date overlap check with time-range conflict messaging appears complete.
1026-1047: No issues found.
Selective pruning of unused keys aligns with your partial data approach.zmsentities/tests/Zmsentities/AvailabilityTest.php (29)
182-182: No issues found.
The updated test expectation for available seconds suggests a refined calculation.
199-199: No issues found.
Similarly adjusting the assertion aligns with the revised logic.
511-511: No issues found.
The test now expects 33 slots, indicating an updated slot calculation.
553-553: No issues found.
Ensuring'public'slot calculation is correct with99.
870-929: No issues found.
Comprehensive conflict checks with new detailed assertions.
959-1015: No issues found.
Valid partial overlap scenarios are thoroughly tested.
1017-1061: No issues found.
Back-to-back times recognized as non-overlapping is a good edge-case coverage.
1063-1109: No issues found.
Validates that separate weekdays or different weeks do not conflict.
1149-1149: No issues found.
Method boundary updated with new lines for the next tests.
1150-1173: No issues found.
Testing start-time validation in detail.
1175-1195: No issues found.
Ensuring end-time constraints are enforced.
1197-1216: No issues found.
Past vs. future time checks for origin end times.
1218-1228: No issues found.
Type validation logic is tested thoroughly.
1230-1249: No issues found.
Slot time must be non-zero and properly divisible.
1251-1261: No issues found.
Day range checks mirror the main code’s logic.
1263-1299: No issues found.
Maintenance window logic is tested systematically.
1301-1325: No issues found.
Future date allowances are validated correctly.
1327-1341: No issues found.
Ensures minute granularity in end-time checks.
1343-1362: No issues found.
Verifies slot-time divisibility thoroughly.
1364-1377: No issues found.
Bookable day range ordering is validated properly.
1379-1392: No issues found.
Multiple valid types tested for correctness.
1394-1427: No issues found.
Summarized slot counts confirm the correct number of slots.
1429-1446: No issues found.
Ensures defaults match the initial expected values.
1448-1479: No issues found.
Compares availability objects for exact matches and differences.
1481-1505: No issues found.
Verifies cache clearing for start/end time changes.
1507-1527: No issues found.
Edge-case testing around boundary times is handled.
1529-1545: No issues found.
Per-day seat multiplication verified for each workstation type.
1547-1595: No issues found.
Tests that overlapping Tuesday schedules do not conflict on Wednesday.
1597-1648: No issues found.
Confirming conflicts are detected for overlapping Tuesday times.zmsadmin/templates/exception/bo/zmsapi/exception/availability/availabilityaddfailed.twig (1)
1-7: No issues found.
Includes a clear conflict message and triggers a page reload after user acknowledgement.zmsapi/src/Zmsapi/Exception/Availability/AvailabilityAddFailed.php (1)
1-13: LGTM! Well-structured exception class.The implementation follows best practices:
- Appropriate HTTP 400 status code for client errors
- Clear and actionable error message
- Proper namespace and class inheritance
zmsadmin/js/page/availabilityDay/layouts/page.js (1)
14-14: LGTM! Duplicate saveBar rendering is intentional.The duplicate rendering of the saveBar component is intentional and acceptable, as it displays the save bar at both the top and bottom of the layout for better user experience.
Also applies to: 23-23
zmsadmin/js/page/availabilityDay/form/errors.js (1)
4-17: LGTM! Enhanced error handling with nested array support.The renderErrors function now properly handles both single and nested array error messages, with appropriate key generation for React's reconciliation.
zmsadmin/js/page/availabilityDay/timetable/index.js (1)
35-37: LGTM! Restored OpeningTimebar mapping.The restoration of OpeningTimebar mapping is essential for proper display of opening hours, which is crucial for the validation system.
zmsadmin/js/page/availabilityDay/saveBar.js (2)
1-10: LGTM! Good practice centralizing the TTL value.The exported
SAVE_BAR_TTLconstant follows the best practice of centralizing timeout values for consistent timing across operations.
47-61: LGTM! Well-defined PropTypes and defaults.The component has proper PropTypes validation and default props, following React best practices.
zmsadmin/src/Zmsadmin/AvailabilityListByScope.php (1)
32-33: Enhance date validation.The current validation only checks if the dates are strings. Consider validating date format and range.
zmsadmin/js/page/availabilityDay/form/index.js (3)
6-6: LGTM!The import of
hasSlotCountErroraligns with the PR objective to improve frontend validation.
41-50: LGTM!The simplified conflict check and new error handling logic improve code readability while maintaining functionality.
🧰 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)
[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)
61-73: LGTM!The
FormButtonscomponent is correctly updated with new error handling props, enhancing the validation feedback mechanism.zmsdb/tests/Zmsdb/AvailabilityTest.php (1)
77-77: LGTM!The assertion is correctly updated to reflect the new 10-minute slot time.
Note: As previously identified, please ensure all slot time references are consistently updated across the codebase, particularly in ProcessTest.php.
zmsdb/tests/Zmsdb/ProcessConflictTest.php (1)
103-105: LGTM!The conflict messages are enhanced with more detailed information about opening times, improving clarity for users.
Also applies to: 120-122
zmsadmin/js/page/availabilityDay/layouts/tableBody.js (1)
8-122: LGTM!The sorting implementation enhances the table's functionality by:
- Adding column-based sorting with visual indicators
- Maintaining sort state using React hooks
- Implementing comprehensive sort logic for all columns
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 39-39: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 43-43: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 44-44: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 57-57: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 58-58: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 70-70: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 71-71: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
zmsadmin/js/page/availabilityDay/form/datepicker.js (6)
24-24: LGTM: Maintenance window time limit.The maxTime state is correctly set to 22:59, aligning with the maintenance window requirement (22:00-01:00).
121-134: LGTM: Maintenance window time exclusions.The
getMaintenanceWindowTimesmethod correctly implements the maintenance window by:
- Excluding 22:00-23:59
- Excluding 00:00-00:59
136-148: LGTM: Availability filtering logic.The
getFilteredAvailabilitiesmethod effectively:
- Filters out current availability by ID
- Matches availability type
- Checks weekday selection
- Sorts by start time for consistent processing
150-166: LGTM: Time exclusion calculation.The
getExcludedTimesForAvailabilitymethod correctly:
- Calculates excluded times between start and end
- Handles minute-by-minute progression
- Preserves the selected date context
168-185: LGTM: Boundary timestamp handling.The
getBoundaryTimestampsmethod properly:
- Identifies adjacent availability boundaries
- Handles exact time matches
- Maintains selected date context
307-321: LGTM: Enhanced time filtering logic.The
filterPassedTimefunction now correctly:
- Handles future dates by only checking maintenance window
- Validates current day times against current time
- Calculates minute-level differences for precise filtering
zmsadmin/js/page/availabilityDay/form/content.js (3)
21-25: LGTM: End time validation error handling.The error detection logic correctly identifies end time past errors by:
- Checking error type at multiple levels
- Using optional chaining for safe property access
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: 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)
70-105: LGTM: Enhanced error message display.The error message section effectively:
- Filters and displays end time past errors
- Uses consistent styling with warning icon
- Provides clear message heading and body
29-34: LGTM: Error filtering logic.The filtered error list correctly:
- Excludes end time past errors
- Preserves error ID mapping
- Maintains error structure
zmsadmin/routing.php (2)
18-34: LGTM: New availability management routes.The new routes effectively support availability management:
- List updates (
/availability/)- Slot calculations (
/availability/slots/)- Single item operations (delete, save)
- Conflict checking
- Scope-based listing
67-80: LGTM: Mail template management routes.The mail template routes properly handle:
- Template updates
- Customization management
- Preview functionality
zmsentities/tests/Zmsentities/ProcessTest.php (1)
494-494: LGTM: Updated test assertion for end time calculation.The test now correctly expects end time '19:12' based on:
- Slot time of 12 minutes
- Slot count of 2 (12 × 2 = 24 minutes)
- Start time of 18:52
zmsadmin/tests/Zmsadmin/AvailabilityConflictsTest.php (1)
1959-2038: Comprehensive test coverage for availability conflicts.The new test methods provide thorough coverage of different conflict scenarios:
- Different weekdays
- Different types
- Multiple conflicts
- Day-specific conflicts (Wednesday/Tuesday)
The test data is well-structured and includes consistent dayoff entries.
zmsentities/schema/availability.json (2)
21-21: Verify impact of reduced slot time.The slot time has been reduced from 12 to 10 minutes. This change could affect existing appointments and scheduling logic.
Please verify:
- Impact on existing appointments
- Frontend handling of the new duration
- Database migration requirements
122-125: New 'kind' property extends availability categorization.The addition of the 'kind' property allows for more detailed categorization beyond the existing 'type' property.
Consider documenting valid values for the 'kind' property in the description.
zmsadmin/tests/Zmsadmin/fixtures/GET_scope_141_multipleSlotsEnabled.json (1)
98-98: Test fixture updated for new slot time.The slotTimeInMinutes value has been updated to match the schema change from 12 to 10 minutes.
zmsadmin/tests/Zmsadmin/fixtures/GET_scope_141.json (1)
100-101: Updated Appointment Slot Duration in Fixture
The"slotTimeInMinutes"property has been modified from 12 to 10 to reflect the updated appointment duration requirements. This change appears consistent with the overall update strategy across the testing fixtures.zmsadmin/tests/Zmsadmin/fixtures/GET_scope_list.json (1)
101-102: Consistent Slot Duration Update
The"slotTimeInMinutes"value is now set to 10, aligning this fixture with the revised appointment slot standards. The change is clear and maintains data consistency across different test cases.zmsadmin/tests/Zmsadmin/fixtures/GET_Workstation_scope_141_multipleSlotsEnabled.json (1)
144-145: Refined Appointment Slot Time
The update of the"slotTimeInMinutes"property to 10 ensures that the fixture for multiple slots enabled scenarios reflects the new duration constraint. This update supports both data integrity and consistency across test cases.zmsadmin/tests/Zmsadmin/fixtures/GET_Workstation_Resolved2.json (1)
153-154: Updated Slot Duration in Workstation Fixture
The provider’s"slotTimeInMinutes"within this fixture has been updated to 10. This modification adheres to the updated specification for appointment durations and helps maintain uniformity across all test fixtures.zmsentities/schema/dereferenced/availability.json (1)
21-22: Schema Adjustment for Slot Duration
The JSON schema now specifies"slotTimeInMinutes": 10, reflecting the new appointment slot time. This update ensures downstream calculations—such as those multiplying this value with a slot count—will use the revised duration, aligning with the broader changes made in the test fixtures and application logic.zmsentities/schema/dereferenced/appointment.json (1)
38-38: UpdateslotTimeInMinutesto 10The
slotTimeInMinutesproperty has been updated from 12 to 10 in the availability example. This change ensures that the calculated appointment or process duration reflects the new slot configuration. Please verify that all related components and backend validations make use of this updated duration.zmsadmin/js/page/availabilityDay/layouts/accordion.js (4)
6-6: Consistent Import FormattingThe import of the helper (
accordionTitle) now follows a consistent style. This improves readability and maintainability.
28-31: Addition ofhasNewAvailabilityFlagThe new
hasNewAvailabilityconstant efficiently checks if any availability item carries a temporary ID (matching__temp__) or is flagged as an exclusion. This added flag is then used to set theisCreatingExclusionprop downstream.
142-157: Prop Handling forAvailabilityFormThe props passed into
<AvailabilityForm>are well organized:
- Cloning the
conflictListviaObject.assign({}, conflictList)provides immutability safety.- The combined flag for
isCreatingExclusion(usingthis.props.isCreatingExclusion || hasNewAvailability) clearly communicates the intended behavior.
These changes are aligned with the objective to replicate and enhance the frontend validation logic.
214-214: Component Export ConfirmationThe
Accordioncomponent is correctly exported as the default export, which is in line with common module practices.zmsadmin/js/page/availabilityDay/form/formButtons.js (1)
3-18: LGTM! Props align with validation improvements.The new props
hasErrors,hasSlotCountError, andisCreatingExclusionenhance validation capabilities, aligning with the PR objectives.zmsadmin/js/page/availabilityDay/form/footerButtons.js (2)
3-17: LGTM! Props enhance validation capabilities.The new props align with the PR objectives to improve validation and data integrity.
78-89: LGTM! PropTypes are well-defined.The PropTypes are correctly defined with
selectedDateappropriately marked as required.
| const { onDelete, onSelect, onAbort, timestamp, availabilityList, data } = props | ||
| const titleTime = `Alle Öffnungszeiten für den Standort ${props.scope?.contact?.name || ''}` |
There was a problem hiding this comment.
Sanitize scope.contact.name before rendering.
The title concatenation with scope?.contact?.name could be vulnerable to XSS if the name contains malicious HTML/JavaScript.
Consider using a sanitization library or escaping HTML characters:
- const titleTime = `Alle Öffnungszeiten für den Standort ${props.scope?.contact?.name || ''}`
+ const escapedName = props.scope?.contact?.name?.replace(/[&<>"']/g, char => ({
+ '&': '&',
+ '<': '<',
+ '>': '>',
+ '"': '"',
+ "'": '''
+ })[char]) || '';
+ const titleTime = `Alle Öffnungszeiten für den Standort ${escapedName}`📝 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.
| const { onDelete, onSelect, onAbort, timestamp, availabilityList, data } = props | |
| const titleTime = `Alle Öffnungszeiten für den Standort ${props.scope?.contact?.name || ''}` | |
| const { onDelete, onSelect, onAbort, timestamp, availabilityList, data } = props | |
| const escapedName = props.scope?.contact?.name?.replace(/[&<>"']/g, char => ({ | |
| '&': '&', | |
| '<': '<', | |
| '>': '>', | |
| '"': '"', | |
| "'": ''' | |
| })[char]) || ''; | |
| const titleTime = `Alle Öffnungszeiten für den Standort ${escapedName}` |
| conflictDatesByMessage.map((item, index) => ( | ||
| <div key={index} style={{ marginBottom: '1rem' }}> | ||
| {/* Convert newlines in the message to <br /> tags */} | ||
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> |
There was a problem hiding this comment.
Security Risk: Unsafe HTML rendering.
Using dangerouslySetInnerHTML with unsanitized content from item.message exposes the application to XSS attacks.
Consider these safer alternatives:
- Use a sanitization library:
- <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} />
+ <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(item.message.replace(/\n/g, '<br />')) }} />- Or better, use CSS white-space handling:
- <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} />
+ <div style={{ whiteSpace: 'pre-line' }}>{item.message}</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.
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> | |
| import DOMPurify from 'dompurify'; | |
| // ... other imports | |
| // Line 33 update: | |
| <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(item.message.replace(/\n/g, '<br />')) }} /> |
| <div dangerouslySetInnerHTML={{ __html: item.message.replace(/\n/g, '<br />') }} /> | |
| // Line 33 update: | |
| <div style={{ whiteSpace: 'pre-line' }}>{item.message}</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)
| try { | ||
| $availabilityList = \App::$http | ||
| ->readGetResult( | ||
| '/scope/' . $scopeId . '/availability/', | ||
| [ | ||
| 'startDate' => $startDate, | ||
| 'endDate' => $endDate, | ||
| 'resolveReferences' => 2 | ||
| ] | ||
| ) | ||
| ->getCollection() | ||
| ->sortByCustomKey('startDate'); | ||
|
|
||
| $data = $availabilityList->getArrayCopy(); | ||
| if (!is_array($data) && is_object($data)) { | ||
| $data = array_values((array)$data); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add specific error handling for HTTP request failures.
Consider adding specific error handling for different HTTP status codes and network failures.
Example implementation:
try {
+ if (!App::$http) {
+ throw new \RuntimeException('HTTP client not initialized');
+ }
+
$availabilityList = \App::$http
->readGetResult(
'/scope/' . $scopeId . '/availability/',
[
'startDate' => $startDate,
'endDate' => $endDate,
'resolveReferences' => 2
]
)
->getCollection()
->sortByCustomKey('startDate');
+} catch (\GuzzleHttp\Exception\ConnectException $e) {
+ throw new \RuntimeException('Failed to connect to availability service: ' . $e->getMessage());
+} catch (\GuzzleHttp\Exception\ClientException $e) {
+ if ($e->getResponse()->getStatusCode() === 404) {
+ return \BO\Slim\Render::withJson($response, [
+ 'meta' => [],
+ 'data' => []
+ ]);
+ }
+ throw $e;📝 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.
| try { | |
| $availabilityList = \App::$http | |
| ->readGetResult( | |
| '/scope/' . $scopeId . '/availability/', | |
| [ | |
| 'startDate' => $startDate, | |
| 'endDate' => $endDate, | |
| 'resolveReferences' => 2 | |
| ] | |
| ) | |
| ->getCollection() | |
| ->sortByCustomKey('startDate'); | |
| $data = $availabilityList->getArrayCopy(); | |
| if (!is_array($data) && is_object($data)) { | |
| $data = array_values((array)$data); | |
| } | |
| try { | |
| if (!\App::$http) { | |
| throw new \RuntimeException('HTTP client not initialized'); | |
| } | |
| $availabilityList = \App::$http | |
| ->readGetResult( | |
| '/scope/' . $scopeId . '/availability/', | |
| [ | |
| 'startDate' => $startDate, | |
| 'endDate' => $endDate, | |
| 'resolveReferences' => 2 | |
| ] | |
| ) | |
| ->getCollection() | |
| ->sortByCustomKey('startDate'); | |
| } catch (\GuzzleHttp\Exception\ConnectException $e) { | |
| throw new \RuntimeException('Failed to connect to availability service: ' . $e->getMessage()); | |
| } catch (\GuzzleHttp\Exception\ClientException $e) { | |
| if ($e->getResponse()->getStatusCode() === 404) { | |
| return \BO\Slim\Render::withJson($response, [ | |
| 'meta' => [], | |
| 'data' => [] | |
| ]); | |
| } | |
| throw $e; | |
| } | |
| $data = $availabilityList->getArrayCopy(); | |
| if (!is_array($data) && is_object($data)) { | |
| $data = array_values((array)$data); | |
| } |
| const hasNewAvailabilities = availabilitylist?.some( | ||
| availability => availability?.tempId?.includes('__temp__') | ||
| ); | ||
|
|
||
| const hasSplitInProgress = (() => { | ||
| let hasOriginWithId = false; | ||
| let hasExclusion = false; | ||
| let hasFuture = false; | ||
|
|
||
| availabilitylist?.forEach(availability => { | ||
| if (availability?.kind) { | ||
| if (availability.kind === 'origin' && availability.id) { | ||
| hasOriginWithId = true; | ||
| } else if (availability.kind === 'exclusion') { | ||
| hasExclusion = true; | ||
| } else if (availability.kind === 'future') { | ||
| hasFuture = true; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return hasOriginWithId && (hasExclusion || hasFuture); | ||
| })(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for better maintainability and testability.
The current implementation has two areas for improvement:
- The IIFE makes the code harder to test.
- The hasNewAvailabilities check could be simplified.
Consider these improvements:
- const hasNewAvailabilities = availabilitylist?.some(
- availability => availability?.tempId?.includes('__temp__')
- );
+ const hasNewAvailabilities = availabilitylist?.some(
+ availability => availability?.tempId?.includes('__temp__')
+ ) ?? false;
- const hasSplitInProgress = (() => {
- let hasOriginWithId = false;
- let hasExclusion = false;
- let hasFuture = false;
-
- availabilitylist?.forEach(availability => {
- if (availability?.kind) {
- if (availability.kind === 'origin' && availability.id) {
- hasOriginWithId = true;
- } else if (availability.kind === 'exclusion') {
- hasExclusion = true;
- } else if (availability.kind === 'future') {
- hasFuture = true;
- }
- }
- });
-
- return hasOriginWithId && (hasExclusion || hasFuture);
- })();
+ const checkSplitInProgress = (availabilitylist) => {
+ const kinds = availabilitylist?.reduce((acc, availability) => {
+ if (availability?.kind) {
+ acc[availability.kind] = true;
+ if (availability.kind === 'origin' && availability.id) {
+ acc.hasOriginWithId = true;
+ }
+ }
+ return acc;
+ }, {});
+ return kinds?.hasOriginWithId && (kinds?.exclusion || kinds?.future) ?? false;
+ };
+ const hasSplitInProgress = checkSplitInProgress(availabilitylist);This refactoring:
- Extracts the IIFE into a named function for better testability
- Uses reduce for a more functional approach
- Adds null coalescing operators for safety
- Makes the code more maintainable
📝 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.
| const hasNewAvailabilities = availabilitylist?.some( | |
| availability => availability?.tempId?.includes('__temp__') | |
| ); | |
| const hasSplitInProgress = (() => { | |
| let hasOriginWithId = false; | |
| let hasExclusion = false; | |
| let hasFuture = false; | |
| availabilitylist?.forEach(availability => { | |
| if (availability?.kind) { | |
| if (availability.kind === 'origin' && availability.id) { | |
| hasOriginWithId = true; | |
| } else if (availability.kind === 'exclusion') { | |
| hasExclusion = true; | |
| } else if (availability.kind === 'future') { | |
| hasFuture = true; | |
| } | |
| } | |
| }); | |
| return hasOriginWithId && (hasExclusion || hasFuture); | |
| })(); | |
| const hasNewAvailabilities = availabilitylist?.some( | |
| availability => availability?.tempId?.includes('__temp__') | |
| ) ?? false; | |
| const checkSplitInProgress = (availabilitylist) => { | |
| const kinds = availabilitylist?.reduce((acc, availability) => { | |
| if (availability?.kind) { | |
| acc[availability.kind] = true; | |
| if (availability.kind === 'origin' && availability.id) { | |
| acc.hasOriginWithId = true; | |
| } | |
| } | |
| return acc; | |
| }, {}); | |
| return kinds?.hasOriginWithId && (kinds?.exclusion || kinds?.future) ?? false; | |
| }; | |
| const hasSplitInProgress = checkSplitInProgress(availabilitylist); |
…ening-hours-of-the-same-appointment-type-must-not-overlap
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
zmsadmin/package.json (1)
59-59: New Dependency Addition: "svgo"
The inclusion of"svgo": "^3.3.2"in thedevDependenciesprovides a useful tool for optimizing SVG assets. This change aligns with the frontend enhancements, especially if your components (e.g., error icons in the Availability UI) leverage SVGs. Ensure the following:
- The version
"^3.3.2"is compatible with your current tooling and build process.- Its usage is integrated into your build scripts or asset optimization routines to fully utilize its benefits.
zmsadmin/js/page/availabilityDay/index.js (4)
10-10: Add JSDoc documentation for the new state property.Consider documenting the purpose and structure of the
fullAvailabilityListstate property to improve maintainability.constructor(props) { super(props) + /** + * @type {Object} state + * @property {Array|null} fullAvailabilityList - Stores availability data for the entire scope over a year + */ this.state = { ...getInitialState(props), fullAvailabilityList: null }Also applies to: 42-45
561-580: Enhance validation robustness in validateAvailabilityList.The validation could be improved to:
- Check for valid time ranges (end time > start time)
- Validate against business hours
- Check for overlapping availabilities
validateAvailabilityList(availabilitylist) { const timeRegex = /^\d{2}:\d{2}(:\d{2})?$/; const isValidTimestamp = (timestamp) => !Number.isNaN(Number(timestamp)) && moment.unix(timestamp).isValid(); + + const isValidTimeRange = (availability) => { + const startMoment = moment(`${availability.startTime}`, 'HH:mm:ss'); + const endMoment = moment(`${availability.endTime}`, 'HH:mm:ss'); + return endMoment.isAfter(startMoment); + }; const invalidAvailabilities = availabilitylist.filter((availability) => { const hasInvalidDates = !isValidTimestamp(availability.startDate) || !isValidTimestamp(availability.endDate); const hasInvalidTimes = !timeRegex.test(availability.startTime) || !timeRegex.test(availability.endTime); + const hasInvalidTimeRange = !isValidTimeRange(availability); if (hasInvalidDates || hasInvalidTimes) { console.warn("Invalid availability detected:", availability); } - return hasInvalidDates || hasInvalidTimes; + return hasInvalidDates || hasInvalidTimes || hasInvalidTimeRange; }); return invalidAvailabilities; }
237-239: Optimize performance by avoiding the delete operator.The static analysis tool suggests avoiding the delete operator for better performance.
-if (availability.tempId) { - delete sendAvailability.tempId; -} +if (availability.tempId) { + sendAvailability.tempId = undefined; +}Also applies to: 159-161
🧰 Tools
🪛 Biome (1.9.4)
[error] 238-239: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
681-698: Enhance accessibility in view switching logic.Consider adding ARIA attributes and keyboard navigation support for the view switching functionality.
let ViewComponent let availabilityList +const viewTypes = { + graph: { component: GraphView, label: 'Graph View' }, + scope: { component: ScopeView, label: 'Scope View' }, + table: { component: TableView, label: 'Table View' } +}; switch(this.state.selectedTab) { case 'graph': - ViewComponent = GraphView + ViewComponent = viewTypes.graph.component availabilityList = this.state.availabilitylistslices || this.state.availabilitylist break case 'scope': - ViewComponent = ScopeView + ViewComponent = viewTypes.scope.component availabilityList = this.state.fullAvailabilityList || [] if (!this.state.fullAvailabilityList) { this.getAllScopeAvailabilities() } break default: - ViewComponent = TableView + ViewComponent = viewTypes.table.component availabilityList = this.state.availabilitylistslices || this.state.availabilitylist }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
zmsadmin/package-lock.jsonis excluded by!**/package-lock.jsonzmscalldisplay/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
zmsadmin/cache/.keep(0 hunks)zmsadmin/js/page/availabilityDay/index.js(21 hunks)zmsadmin/package.json(2 hunks)
💤 Files with no reviewable changes (1)
- zmsadmin/cache/.keep
🧰 Additional context used
🧠 Learnings (1)
zmsadmin/js/page/availabilityDay/index.js (2)
Learnt from: ThomasAFink
PR: it-at-m/eappointment#0
File: :0-0
Timestamp: 2025-01-30T15:35:15.938Z
Learning: In the eappointment system's handleChange method, when validating form changes, run getValidationList and getConflictList in parallel using Promise.all, and only proceed with readCalculatedAvailabilityList after both validations complete to avoid race conditions.
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/index.js
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 160-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 238-239: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: call-owasp-security-checks / security-scan (zmsclient, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsdldb, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsstatistic, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsmessaging, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsentities, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsdldb, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: Analyze (python)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: Analyze (javascript-typescript)
| getValidationList() { | ||
| return new Promise((resolve, reject) => { | ||
| const validateData = (data) => { | ||
| const validationResult = validate(data, this.props); | ||
| if (!validationResult.valid) { | ||
| return validationResult.errorList; | ||
| } | ||
| return []; | ||
| }; | ||
|
|
||
| try { | ||
| const list = this.state.availabilitylist | ||
| .map(validateData) | ||
| .flat(); | ||
|
|
||
| console.log("Validations fetched successfully:", list); | ||
|
|
||
| this.setState( | ||
| { | ||
| errorList: list.length ? list : [], | ||
| }, | ||
| () => { | ||
| if (list.length > 0) { | ||
| const nonPastTimeErrors = list.filter(error => | ||
| !error.itemList?.flat(2).some(item => item?.type === 'endTimePast') | ||
| ); | ||
|
|
||
| if (nonPastTimeErrors.length > 0) { | ||
| console.warn("Validation failed with errors:", nonPastTimeErrors); | ||
| this.errorElement?.scrollIntoView(); | ||
| } else { | ||
| console.log("Validation passed with only past time errors."); | ||
| } | ||
| ) | ||
| }) | ||
| if (data.conflictIdList.length > 0) { | ||
| this.errorElement.scrollIntoView() | ||
| } else { | ||
| console.log("Validation passed successfully."); | ||
| resolve(); | ||
| } | ||
| } | ||
| }, | ||
| (err) => { | ||
| let isException = err.responseText.toLowerCase().includes('exception'); | ||
| if (err.status >= 500 && isException) { | ||
| new ExceptionHandler($('.opened'), { | ||
| code: err.status, | ||
| message: err.responseText | ||
| ); | ||
| } catch (error) { | ||
| console.error("Validation error:", error); | ||
| reject(error); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement parallel validation based on past learnings.
Based on the retrieved learning from ThomasAFink, we should run validations in parallel using Promise.all to avoid race conditions.
getValidationList() {
return new Promise((resolve, reject) => {
const validateData = (data) => {
const validationResult = validate(data, this.props);
if (!validationResult.valid) {
return validationResult.errorList;
}
return [];
};
try {
const list = this.state.availabilitylist
.map(validateData)
.flat();
console.log("Validations fetched successfully:", list);
- this.setState(
- {
- errorList: list.length ? list : [],
- },
- () => {
- if (list.length > 0) {
- const nonPastTimeErrors = list.filter(error =>
- !error.itemList?.flat(2).some(item => item?.type === 'endTimePast')
- );
-
- if (nonPastTimeErrors.length > 0) {
- console.warn("Validation failed with errors:", nonPastTimeErrors);
- this.errorElement?.scrollIntoView();
- } else {
- console.log("Validation passed with only past time errors.");
- }
- } else {
- console.log("Validation passed successfully.");
- resolve();
- }
- }
- );
+ Promise.all([
+ new Promise(resolveState => {
+ this.setState(
+ { errorList: list.length ? list : [] },
+ resolveState
+ );
+ }),
+ this.getConflictList()
+ ]).then(() => {
+ if (list.length > 0) {
+ const nonPastTimeErrors = list.filter(error =>
+ !error.itemList?.flat(2).some(item => item?.type === 'endTimePast')
+ );
+
+ if (nonPastTimeErrors.length > 0) {
+ console.warn("Validation failed with errors:", nonPastTimeErrors);
+ this.errorElement?.scrollIntoView();
+ } else {
+ console.log("Validation passed with only past time errors.");
+ }
+ } else {
+ console.log("Validation passed successfully.");
+ }
+ resolve();
+ });
} catch (error) {
console.error("Validation error:", error);
reject(error);
}
});
}📝 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.
| getValidationList() { | |
| return new Promise((resolve, reject) => { | |
| const validateData = (data) => { | |
| const validationResult = validate(data, this.props); | |
| if (!validationResult.valid) { | |
| return validationResult.errorList; | |
| } | |
| return []; | |
| }; | |
| try { | |
| const list = this.state.availabilitylist | |
| .map(validateData) | |
| .flat(); | |
| console.log("Validations fetched successfully:", list); | |
| this.setState( | |
| { | |
| errorList: list.length ? list : [], | |
| }, | |
| () => { | |
| if (list.length > 0) { | |
| const nonPastTimeErrors = list.filter(error => | |
| !error.itemList?.flat(2).some(item => item?.type === 'endTimePast') | |
| ); | |
| if (nonPastTimeErrors.length > 0) { | |
| console.warn("Validation failed with errors:", nonPastTimeErrors); | |
| this.errorElement?.scrollIntoView(); | |
| } else { | |
| console.log("Validation passed with only past time errors."); | |
| } | |
| ) | |
| }) | |
| if (data.conflictIdList.length > 0) { | |
| this.errorElement.scrollIntoView() | |
| } else { | |
| console.log("Validation passed successfully."); | |
| resolve(); | |
| } | |
| } | |
| }, | |
| (err) => { | |
| let isException = err.responseText.toLowerCase().includes('exception'); | |
| if (err.status >= 500 && isException) { | |
| new ExceptionHandler($('.opened'), { | |
| code: err.status, | |
| message: err.responseText | |
| ); | |
| } catch (error) { | |
| console.error("Validation error:", error); | |
| reject(error); | |
| } | |
| }); | |
| } | |
| getValidationList() { | |
| return new Promise((resolve, reject) => { | |
| const validateData = (data) => { | |
| const validationResult = validate(data, this.props); | |
| if (!validationResult.valid) { | |
| return validationResult.errorList; | |
| } | |
| return []; | |
| }; | |
| try { | |
| const list = this.state.availabilitylist | |
| .map(validateData) | |
| .flat(); | |
| console.log("Validations fetched successfully:", list); | |
| Promise.all([ | |
| new Promise(resolveState => { | |
| this.setState( | |
| { errorList: list.length ? list : [] }, | |
| resolveState | |
| ); | |
| }), | |
| this.getConflictList() | |
| ]).then(() => { | |
| if (list.length > 0) { | |
| const nonPastTimeErrors = list.filter(error => | |
| !error.itemList?.flat(2).some(item => item?.type === 'endTimePast') | |
| ); | |
| if (nonPastTimeErrors.length > 0) { | |
| console.warn("Validation failed with errors:", nonPastTimeErrors); | |
| this.errorElement?.scrollIntoView(); | |
| } else { | |
| console.log("Validation passed with only past time errors."); | |
| } | |
| } else { | |
| console.log("Validation passed successfully."); | |
| } | |
| resolve(); | |
| }); | |
| } catch (error) { | |
| console.error("Validation error:", error); | |
| reject(error); | |
| } | |
| }); | |
| } |
| getAllScopeAvailabilities() { | ||
| const startDate = moment().startOf('year').format('YYYY-MM-DD') | ||
| const endDate = moment().add(1, 'year').format('YYYY-MM-DD') | ||
|
|
||
| const url = `${this.props.links.includeurl}/scope/${this.props.scope.id}/availability/?startDate=${startDate}&endDate=${endDate}` | ||
|
|
||
| $.ajax(url, { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Accept': 'application/json', | ||
| 'X-Requested-With': 'XMLHttpRequest' | ||
| } | ||
| }).done(response => { | ||
| const availabilityList = response.data ? Object.values(response.data) : []; | ||
|
|
||
| this.setState({ | ||
| fullAvailabilityList: availabilityList | ||
| }, () => { | ||
| if (availabilityList.length > 0) { | ||
| this.getValidationList(); | ||
| } | ||
| }); | ||
| }).fail(err => { | ||
| console.error('getAllScopeAvailabilities error', err); | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in getAllScopeAvailabilities.
The error handling could be improved to:
- Handle network failures gracefully
- Notify the user of failures
- Retry failed requests
getAllScopeAvailabilities() {
const startDate = moment().startOf('year').format('YYYY-MM-DD')
const endDate = moment().add(1, 'year').format('YYYY-MM-DD')
const url = `${this.props.links.includeurl}/scope/${this.props.scope.id}/availability/?startDate=${startDate}&endDate=${endDate}`
+ const maxRetries = 3;
+ let retryCount = 0;
+
+ const fetchWithRetry = () => {
$.ajax(url, {
method: 'GET',
headers: {
'Accept': 'application/json',
'X-Requested-With': 'XMLHttpRequest'
}
}).done(response => {
const availabilityList = response.data ? Object.values(response.data) : [];
this.setState({
fullAvailabilityList: availabilityList
}, () => {
if (availabilityList.length > 0) {
this.getValidationList();
}
});
}).fail(err => {
- console.error('getAllScopeAvailabilities error', err);
+ console.error('getAllScopeAvailabilities error', err);
+ if (retryCount < maxRetries && (err.status >= 500 || err.status === 0)) {
+ retryCount++;
+ setTimeout(fetchWithRetry, 1000 * retryCount);
+ } else {
+ new ExceptionHandler($('.opened'), {
+ code: err.status,
+ message: 'Failed to fetch availability data. Please try again later.'
+ });
+ }
})
+ }
+
+ fetchWithRetry();
}📝 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.
| getAllScopeAvailabilities() { | |
| const startDate = moment().startOf('year').format('YYYY-MM-DD') | |
| const endDate = moment().add(1, 'year').format('YYYY-MM-DD') | |
| const url = `${this.props.links.includeurl}/scope/${this.props.scope.id}/availability/?startDate=${startDate}&endDate=${endDate}` | |
| $.ajax(url, { | |
| method: 'GET', | |
| headers: { | |
| 'Accept': 'application/json', | |
| 'X-Requested-With': 'XMLHttpRequest' | |
| } | |
| }).done(response => { | |
| const availabilityList = response.data ? Object.values(response.data) : []; | |
| this.setState({ | |
| fullAvailabilityList: availabilityList | |
| }, () => { | |
| if (availabilityList.length > 0) { | |
| this.getValidationList(); | |
| } | |
| }); | |
| }).fail(err => { | |
| console.error('getAllScopeAvailabilities error', err); | |
| }) | |
| } | |
| getAllScopeAvailabilities() { | |
| const startDate = moment().startOf('year').format('YYYY-MM-DD') | |
| const endDate = moment().add(1, 'year').format('YYYY-MM-DD') | |
| const url = `${this.props.links.includeurl}/scope/${this.props.scope.id}/availability/?startDate=${startDate}&endDate=${endDate}` | |
| const maxRetries = 3; | |
| let retryCount = 0; | |
| const fetchWithRetry = () => { | |
| $.ajax(url, { | |
| method: 'GET', | |
| headers: { | |
| 'Accept': 'application/json', | |
| 'X-Requested-With': 'XMLHttpRequest' | |
| } | |
| }).done(response => { | |
| const availabilityList = response.data ? Object.values(response.data) : []; | |
| this.setState({ | |
| fullAvailabilityList: availabilityList | |
| }, () => { | |
| if (availabilityList.length > 0) { | |
| this.getValidationList(); | |
| } | |
| }); | |
| }).fail(err => { | |
| console.error('getAllScopeAvailabilities error', err); | |
| if (retryCount < maxRetries && (err.status >= 500 || err.status === 0)) { | |
| retryCount++; | |
| setTimeout(fetchWithRetry, 1000 * retryCount); | |
| } else { | |
| new ExceptionHandler($('.opened'), { | |
| code: err.status, | |
| message: 'Failed to fetch availability data. Please try again later.' | |
| }); | |
| } | |
| }) | |
| } | |
| fetchWithRetry(); | |
| } |
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Bug Fixes
Refactor