feat(ZMSKVR-72): create closure for specific day and office #915
Conversation
WalkthroughThis pull request introduces a new closure management feature across multiple application layers. Two new POST routes have been added—one for the admin interface and another for the API—to toggle the closure status for a specified scope on a given date. New controller classes have been created to handle these requests. In addition, the templates are updated to display closure indicators, and the back-end has been extended with new database models, query builders, JSON schemas, and entity classes dealing with closures. Finally, an SQL migration creates the necessary table in the database. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Admin User
participant Browser as Web Browser
participant JS as ToggleFunction (JS)
participant Route as zmsadmin Routing
participant Controller as ScopeAvailabilityDayClosure
participant DB as Database Layer
User->>Browser: Load availability day page
Browser->>JS: Render toggle closure button
User->>Browser: Click "Toggle Closure"
Browser->>JS: Execute toggleClosure()
JS->>Route: AJAX POST /scope/{id}/availability/day/{date}/closure/toggle/
Route->>Controller: Dispatch request
Controller->>DB: Toggle closure record for given scope/date
DB-->>Controller: Return closure data
Controller-->>Route: Return JSON response
Route-->>JS: Send response
JS->>Browser: Update UI based on response
sequenceDiagram
participant Client as API Client
participant API_Router as zmsapi Routing
participant API_Controller as AvailabilityClosureToggle
participant DB as Database Layer
Client->>API_Router: POST /scope/{id}/availability/{date}/closure/toggle/
API_Router->>API_Controller: Dispatch request
API_Controller->>DB: Validate and toggle closure record
DB-->>API_Controller: Return updated closure info
API_Controller-->>API_Router: Construct JSON response
API_Router-->>Client: Return response
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (17)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (24)
zmsentities/schema/closure.json (1)
1-25: Schema definition follows best practices but has a capitalization inconsistencyThe schema definition for closure follows JSON schema best practices with clear type definitions, descriptions, and required fields. However, there's an inconsistency in property naming convention - "Datum" uses Pascal case while other properties like "id" and "lastChange" use camelCase.
Consider renaming "Datum" to "datum" for consistency with other property names across the codebase.
- "Datum": 1447924981 + "datum": 1447924981And the same change in the properties section:
- "Datum": { + "datum": {zmsentities/schema/dereferenced/scope.json (1)
234-257: Dereferenced schema maintains the same capitalization inconsistencyThe dereferenced schema correctly inlines the closure schema definition, but it carries forward the same capitalization inconsistency with the "Datum" property noted in the closure.json file.
For consistency with other property names in the schema, consider renaming "Datum" to "datum" here as well.
- "Datum": 1447924981 + "datum": 1447924981And:
- "Datum": { + "datum": {zmsentities/schema/dereferenced/closure.json (2)
2-25: The closure schema structure looks good, but consider some improvements.The schema is well-structured with clear property definitions and appropriate restrictions. However, there are a few suggestions:
- The property
Datum(German) is inconsistent with other English property names (id,lastChange). Consider using consistent language in the schema.- The example object could include all properties, including
lastChange.- I notice that in the SQL queries, there's a
StandortIDcolumn referenced when joining with the closures table, but this field isn't represented in this schema.
16-19: Consider using ISO 8601 for date representation.While Unix timestamps are valid for date/time representation, ISO 8601 format might be more human-readable and consistent with modern API practices.
zmsdb/src/Zmsdb/Query/Day.php (2)
87-87: LEFT JOIN implementation is correct but has naming inconsistency.The LEFT JOIN with the closures table is correctly implemented to match records based on scope, year, month, and day. However, there's a naming inconsistency:
StandortIDuses PascalCase while other columns use camelCase (scopeID,year,month,day).Consider standardizing the column naming convention across the database.
89-89: Add a comment explaining the HAVING clause.The HAVING clause correctly filters out slots that have corresponding closures. However, adding a comment would improve code readability and maintainability.
- HAVING cc.id IS NULL + -- Filter out slots that are marked as closed + HAVING cc.id IS NULLzmsdb/src/Zmsdb/Query/ProcessStatusFree.php (2)
33-34: Consider explicitly listing needed columns instead of usings.*.Using
s.*simplifies the query, but it can impact performance by retrieving unnecessary columns from the slot table. Consider explicitly listing only the required columns.
48-48: Add a comment explaining the HAVING clause.Similar to the suggestion for Day.php, adding a comment would improve code readability and maintainability.
- HAVING cc.id IS NULL + -- Filter out slots that are marked as closed + HAVING cc.id IS NULLzmsadmin/src/Zmsadmin/ScopeAvailabilityMonth.php (1)
13-13: Added import for Closure class.The import is correctly added, though it appears to be unused in the current implementation since we're only accessing the closure list through the scope object. Consider removing this import if it's not needed.
-use BO\Zmsentities\Closure;zmsapi/routing.php (1)
4142-4184: Minor typo and parameter type mismatch in Swagger documentation.The new endpoint for toggling availability closure looks well-structured, but there are two small issues in the Swagger documentation:
- In the summary, "Toogle" should be "Toggle"
- The "date" parameter is described as type "integer" but based on the format YYYY-MM-DD, it should be of type "string"
- summary: Toogle availability closure for specific day + summary: Toggle availability closure for specific day- type: integer + type: stringzmsadmin/src/Zmsadmin/ScopeAvailabilityDayClosure.php (1)
26-27: Add input validation for parametersThe code retrieves scopeId and date from the arguments array but doesn't validate these values before using them.
Consider adding validation to ensure the parameters exist and are in the expected format:
- $scopeId = $args['id']; - $date = $args['date']; + $scopeId = isset($args['id']) ? $args['id'] : null; + $date = isset($args['date']) ? $args['date'] : null; + + if (!$scopeId || !$date || !preg_match('/^\d{4}-\d{2}-\d{2}$/', $date)) { + return \BO\Slim\Render::withJson( + $response->withStatus(400), + ['error' => 'Invalid scope ID or date format'] + ); + }zmsdb/migrations/91740144530-create-closures-table.sql (3)
3-10: Add NOT NULL constraints for required fieldsThe table definition does not specify which columns are required. If certain columns like year, month, day, and StandortID are mandatory for a closure, they should have the NOT NULL constraint.
CREATE TABLE `closures` ( `id` INT(5) UNSIGNED AUTO_INCREMENT, - `year` SMALLINT(5), - `month` TINYINT(5), - `day` TINYINT(5), - `StandortID` INT(5) UNSIGNED, + `year` SMALLINT(5) NOT NULL, + `month` TINYINT(5) NOT NULL, + `day` TINYINT(5) NOT NULL, + `StandortID` INT(5) UNSIGNED NOT NULL, `updateTimestamp` TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
7-7: Consider consistent column naming conventionThe column name
StandortIDuses PascalCase while other columns use snake_case. For consistency, consider using snake_case for all column names.- `StandortID` INT(5) UNSIGNED, + `standort_id` INT(5) UNSIGNED,Note: If this is following an existing convention in your database schema, you can disregard this suggestion.
7-11: Consider adding foreign key constraintIf
StandortIDreferences a primary key in another table (like a 'standorts' or 'scopes' table), consider adding a foreign key constraint to maintain referential integrity.`StandortID` INT(5) UNSIGNED, `updateTimestamp` TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (`id`), INDEX (StandortID), - INDEX (StandortID, year, month, day) + INDEX (StandortID, year, month, day), + FOREIGN KEY (StandortID) REFERENCES standorts(id) ON DELETE CASCADEReplace
standorts(id)with the actual table and column name thatStandortIDreferences.zmsadmin/templates/page/availabilityday.twig (2)
40-65: Improve error handling and user feedback in AJAX callThe current implementation only logs errors to the console when the AJAX request fails. Users should be informed about failures.
}).fail(err => { console.log('error', err) + alert('Fehler beim Aktualisieren des Sperrstatus. Bitte versuchen Sie es erneut.') })Additionally, consider adding a loading state to the button while the request is in progress:
function toggleClosure() { const ok = confirm('Möchten Sie wirklich Die Terminbuchung für diesen Tag {% if locked %}entsperren{% else %}sperren{% endif %}?') if (!ok) { return } + const button = event.target + const originalText = button.value + button.value = 'Wird bearbeitet...' + button.disabled = true const currentDate = '{{ dateString }}' const url = `{{ includeUrl() }}/scope/{{ scope.id }}/availability/day/${currentDate}/closure/toggle/` $.ajax(url, { method: 'POST', headers: { 'Content-Type': 'application/json' }, data: JSON.stringify({}) }).done(data => { if (data.closure.existing) { document.getElementById('closure-close').classList.remove('hidden') document.getElementById('closure-open').classList.add('hidden') } else { document.getElementById('closure-close').classList.add('hidden') document.getElementById('closure-open').classList.remove('hidden') } + button.value = originalText + button.disabled = false }).fail(err => { console.log('error', err) + alert('Fehler beim Aktualisieren des Sperrstatus. Bitte versuchen Sie es erneut.') + button.value = originalText + button.disabled = false }) }
33-38: Add ARIA attributes for accessibilityThe message divs should have appropriate ARIA attributes for better accessibility.
- <div id="closure-close" class="message message--warning {% if not locked %}hidden{% endif %}"> + <div id="closure-close" class="message message--warning {% if not locked %}hidden{% endif %}" role="alert" aria-live="polite"> Die Terminbuchung für diesen Tag ist gesperrt. <input class="button btn" style="margin-top: -6px;" type="button" onclick="toggleClosure()" value="Entsperren"> </div> - <div id="closure-open" class="message message--info {% if locked %}hidden{% endif %}"> + <div id="closure-open" class="message message--info {% if locked %}hidden{% endif %}" role="alert" aria-live="polite"> Die Terminbuchung für diesen Tag ist NICHT gesperrt. <input class="button btn" style="margin-top: -6px;" onclick="toggleClosure()" type="button" value="Sperren"> </div>zmsdb/src/Zmsdb/Query/Closure.php (2)
19-28: Add type hinting and documentation to getEntityMapping methodThe
getEntityMappingmethod would benefit from return type hinting and better documentation.- public function getEntityMapping() + /** + * Returns the mapping between entity attributes and database fields + * + * @return array Associative array mapping entity attributes to database fields + */ + public function getEntityMapping(): array { return [ 'id' => 'closure.id', 'year' => 'closure.year', 'month' => 'closure.month', 'day' => 'closure.day', 'lastChange' => 'closure.updateTimestamp' ]; }
30-36: Add type validation in addConditionDate methodThe
addConditionDatemethod doesn't validate that the parameter is actually a DateTime object.- public function addConditionDate(DateTime $date) + /** + * Adds conditions to the query for a specific date + * + * @param DateTime $date The date to filter by + * @return $this For method chaining + * @throws \InvalidArgumentException If date is not a DateTime object + */ + public function addConditionDate(DateTime $date) { + if (!($date instanceof DateTime)) { + throw new \InvalidArgumentException('Date must be a DateTime object'); + } $this->query->where('closure.year', '=', $date->format('Y')); $this->query->where('closure.month', '=', $date->format('m')); $this->query->where('closure.day', '=', $date->format('d')); return $this; }Note: The type hint in the method signature already enforces that
$dateis a DateTime object, so the runtime check isn't strictly necessary. However, adding the check with an informative error message provides better diagnostics.zmsdb/src/Zmsdb/Closure.php (2)
18-24: Simplify collection handling.The current loop logic for adding entities to the collection can be simplified.
if (count($result)) { - foreach ($result as $entity) { - if ($entity instanceof Entity) { - $closureList->addEntity($entity); - } - } + $closureList->addEntities(array_filter($result, function($entity) { + return $entity instanceof Entity; + })); }
42-42: Remove unnecessary parentheses.The parentheses around the return statement are unnecessary.
-return ($this->deleteItem($query)); +return $this->deleteItem($query);zmsentities/src/Zmsentities/Closure.php (2)
38-41: Fix parameter name typo.There's a typo in the parameter name
availabiliywhich should beavailability.-public function isAffectingAvailability(Availability $availabiliy, \DateTimeInterface $now) +public function isAffectingAvailability(Availability $availability, \DateTimeInterface $now) { - return $availabiliy->isBookable($this->getDateTime(), $now); + return $availability->isBookable($this->getDateTime(), $now); }
22-22: Ensure date formatting is robust.The date components are concatenated with hyphens, which might cause issues if month or day is single-digit.
-return (new \BO\Zmsentities\Helper\DateTime($this->year . '-' . $this->month . '-' . $this->day)); +return (new \BO\Zmsentities\Helper\DateTime(sprintf('%04d-%02d-%02d', $this->year, $this->month, $this->day)));zmsentities/src/Zmsentities/Collection/ClosureList.php (2)
25-34: Optimize getEntityByName method.The current implementation continues iterating after finding a match. Use early return for better performance.
public function getEntityByName($name) { - $result = null; foreach ($this as $entity) { if ($entity->name == $name) { - $result = $entity; + return $entity; } } - return $result; + return null; }
60-69: Enhance documentation for withNew method.This method appears to create a new collection containing only the entities from the input collection that aren't already in the current collection, based on date. Consider adding more comprehensive documentation.
+/** + * Returns a new collection containing only entities from the input + * collection that don't have matching dates in the current collection. + * + * @param ClosureList $closureList The input collection to check + * @return ClosureList A new collection with only new entities + */ public function withNew(ClosureList $closureList)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
zmsadmin/routing.php(1 hunks)zmsadmin/src/Zmsadmin/ScopeAvailabilityDayClosure.php(1 hunks)zmsadmin/src/Zmsadmin/ScopeAvailabilityMonth.php(2 hunks)zmsadmin/templates/block/availability/monthtable.twig(1 hunks)zmsadmin/templates/block/calendar/legend.twig(1 hunks)zmsadmin/templates/page/availabilityday.twig(2 hunks)zmsapi/cron/cronjob.minutly(1 hunks)zmsapi/routing.php(1 hunks)zmsapi/src/Zmsapi/AvailabilityClosureToggle.php(1 hunks)zmsdb/migrations/91740144530-create-closures-table.sql(1 hunks)zmsdb/src/Zmsdb/Closure.php(1 hunks)zmsdb/src/Zmsdb/DayOff.php(1 hunks)zmsdb/src/Zmsdb/Query/Closure.php(1 hunks)zmsdb/src/Zmsdb/Query/Day.php(2 hunks)zmsdb/src/Zmsdb/Query/ProcessStatusFree.php(2 hunks)zmsdb/src/Zmsdb/Scope.php(1 hunks)zmsentities/schema/closure.json(1 hunks)zmsentities/schema/dereferenced/closure.json(1 hunks)zmsentities/schema/dereferenced/scope.json(1 hunks)zmsentities/schema/scope.json(1 hunks)zmsentities/src/Zmsentities/Closure.php(1 hunks)zmsentities/src/Zmsentities/Collection/ClosureList.php(1 hunks)zmsentities/src/Zmsentities/Scope.php(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- zmsdb/src/Zmsdb/DayOff.php
- zmsapi/cron/cronjob.minutly
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-owasp-security-checks / security-scan (zmsslim, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsticketprinter, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsstatistic, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsticketprinter, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsslim, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsmessaging, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsclient-test
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (15)
zmsentities/schema/scope.json (1)
136-141: Appropriate addition of closure schema referenceThe "closure" property has been correctly added to the scope schema, following the same pattern as the existing "dayoff" property. This is a clean implementation that maintains consistency with the existing schema structure.
zmsadmin/routing.php (1)
495-496: Well-structured route that follows established patternsThe new route for toggling closure status follows the existing routing patterns in the application. It's appropriately placed at the end of the file and correctly uses the POST method for a state-changing operation.
zmsadmin/templates/block/calendar/legend.twig (1)
52-52: The calendar legend entry for "Gesperrt" looks good!The addition of this entry with the red 'X' icon provides a clear visual indicator for blocked/closed days, which aligns well with the PR objective to create closures for specific days and offices.
zmsdb/src/Zmsdb/Query/Day.php (1)
77-78: Column added for closure ID looks good.Adding the
cc.idcolumn is necessary for the closure filtering logic.zmsdb/src/Zmsdb/Query/ProcessStatusFree.php (1)
46-46: LEFT JOIN implementation is consistent with Day.php.The LEFT JOIN with the closures table matches the implementation in Day.php, which ensures consistent handling of closures across different queries. However, the same naming inconsistency with
StandortIDexists here.zmsdb/src/Zmsdb/Scope.php (1)
43-43: Good integration of the closure data retrieval.The new line correctly follows the established pattern for loading related data when resolving scope references. This addition ensures that closure information will be available when requested with the proper resolve level.
zmsentities/src/Zmsentities/Scope.php (2)
5-5: Properly added import for ClosureList.The import statement is correctly placed with other imports at the beginning of the file.
108-120: Well-structured implementation of getClosureList method.The method follows the same pattern as the existing
getDayoffList()method, ensuring consistency in code style and behavior. It properly handles type checking, initialization, and entity conversion, which will make working with closure entities more reliable.zmsadmin/templates/block/availability/monthtable.twig (2)
94-96: Good addition of closure status indicator.The conditional check for closures and the corresponding icon display are implemented correctly. This provides visual feedback for users when a day has been marked as closed.
100-104: Improved HTML structure for holiday row.The minor formatting changes to the holiday row improve readability and consistency with the rest of the template.
zmsadmin/src/Zmsadmin/ScopeAvailabilityMonth.php (1)
71-71: Properly integrated closure list into the template response.The closure list is correctly added to the template context, allowing the template to access and display closure information.
zmsapi/src/Zmsapi/AvailabilityClosureToggle.php (1)
48-49:❓ Verification inconclusive
Consider validating date format.
The date parameter is directly used without validation, which could lead to errors if an invalid date is provided.
🏁 Script executed:
#!/bin/bash # Check the routing configuration to see if there's any validation at that level rg -A 5 "availability/day/{date:.+}/closure/toggle" --type phpLength of output: 206
Attention: Date Parameter Validation Requires Manual Verification
The current implementation assigns the date parameter without any explicit format validation. While the route configuration might be expected to enforce a proper format, our automated check using the regex pattern
availability/day/{date:.+}/closure/togglefailed due to a syntax issue (the curly braces were not escaped). This leaves us without automated confirmation that the date is being validated elsewhere.
- The assignment of
$dateinzmsapi/src/Zmsapi/AvailabilityClosureToggle.php(lines 48-49) does not include any validation.- The attempted route-level verification yielded a regex parse error, so please manually verify if the routing layer or any middleware is responsible for validating the date format.
- If no validation exists upstream, consider adding a check (e.g., using
DateTime::createFromFormator a similar approach) to ensure the date parameter conforms to the expected format.zmsdb/src/Zmsdb/Closure.php (1)
50-54:❓ Verification inconclusive
Consider removing redundant type casting.
The explicit casting to int might be redundant if the database query handles it properly.
🏁 Script executed:
#!/bin/bash # Check the Closure query implementation to understand the expected data types rg -A 10 "class Closure" --type php -- "src/Zmsdb/Query"Length of output: 112
Next Steps: Manual Verification of Casting Redundancy
The search for a "src/Zmsdb/Query" file yielded no results, so we couldn’t automatically verify whether the query execution layer handles type conversion. The snippet in
zmsdb/src/Zmsdb/Closure.phpexplicitly casts date components to integers, which may be redundant if the underlying database query mechanism performs automatic type conversion.
- Verify manually if the query execution code (or driver) already enforces integer types on these parameters.
- If confirmed, consider removing the casts to simplify the code; if not, retain the casts for explicit type safety.
zmsentities/src/Zmsentities/Closure.php (1)
35-35:✅ Verification successful
Verify the existence of the lastChange property.
The method assumes
lastChangeproperty exists, but it's not explicitly defined in this file.
🏁 Script executed:
#!/bin/bash # Check the schema definition to ensure lastChange property exists fd closure.json -E node_modulesLength of output: 110
Schema Clarity: Verified Existence of
lastChangeProperty
- The JSON schema files (
zmsentities/schema/closure.jsonandzmsentities/schema/dereferenced/closure.json) include a definition for thelastChangeproperty.- Although the
Closure.phpfile does not explicitly declare the property, its existence is confirmed via the schema.zmsentities/src/Zmsentities/Collection/ClosureList.php (1)
48-58:✅ Verification successful
Verify exception class exists.
The code throws
\BO\Zmsentities\Exception\DayoffWrongYear, but it's not clear if this exception class exists.
🏁 Script executed:
#!/bin/bash # Check if the exception class exists rg -A 3 "class DayoffWrongYear" --type phpLength of output: 347
Exception class
DayoffWrongYearexists as expected.
- Verified that in
zmsentities/src/Zmsentities/Exception/DayoffWrongYear.php, the class is defined and correctly extends\Exception.
| "required": [ | ||
| "Datum" | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure property name changes are consistently applied
If you rename "Datum" to "datum" as suggested, ensure the required property list is also updated.
🏁 Script executed:
#!/bin/bash
# Check for other references to the "Datum" property
grep -r "\"Datum\"" --include="*.json" --include="*.php" .Length of output: 661
Action Required: Consistent Property Renaming Across Schemas
In zmsentities/schema/dereferenced/scope.json (lines 243-245), the "required" array still lists "Datum". If you're renaming the property to "datum", please update this as well as all other occurrences (e.g., in zmsentities/schema/dereferenced/closure.json and zmsentities/schema/closure.json) to ensure consistency across the codebase.
- "required": [
- "Datum"
- ],
+ "required": [
+ "datum"
+ ],📝 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.
| "required": [ | |
| "Datum" | |
| ], | |
| "required": [ | |
| "datum" | |
| ], |
| try { | ||
| $closureToggled = \App::$http->readPostResult( | ||
| '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/', | ||
| new \BO\Zmsentities\Closure() | ||
| )->getEntity(); | ||
| } catch (\Exception $e) { | ||
| var_dump($e->getMessage());exit; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling
The current error handling mechanism uses var_dump and exit, which is not suitable for production code. This approach provides no structured error response and terminates script execution abruptly.
Consider implementing proper error handling:
- try {
- $closureToggled = \App::$http->readPostResult(
- '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/',
- new \BO\Zmsentities\Closure()
- )->getEntity();
- } catch (\Exception $e) {
- var_dump($e->getMessage());exit;
- }
+ try {
+ $closureToggled = \App::$http->readPostResult(
+ '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/',
+ new \BO\Zmsentities\Closure()
+ )->getEntity();
+ } catch (\Exception $e) {
+ $this->logger->error('Failed to toggle closure: ' . $e->getMessage());
+ return \BO\Slim\Render::withJson(
+ $response->withStatus(500),
+ ['error' => 'Failed to toggle closure status']
+ );
+ }📝 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 { | |
| $closureToggled = \App::$http->readPostResult( | |
| '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/', | |
| new \BO\Zmsentities\Closure() | |
| )->getEntity(); | |
| } catch (\Exception $e) { | |
| var_dump($e->getMessage());exit; | |
| } | |
| try { | |
| $closureToggled = \App::$http->readPostResult( | |
| '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/', | |
| new \BO\Zmsentities\Closure() | |
| )->getEntity(); | |
| } catch (\Exception $e) { | |
| $this->logger->error('Failed to toggle closure: ' . $e->getMessage()); | |
| return \BO\Slim\Render::withJson( | |
| $response->withStatus(500), | |
| ['error' => 'Failed to toggle closure status'] | |
| ); | |
| } |
| {% set a = getNow()|date('Y-m-d') %} | ||
| {% if scope.closure.hasEntityByDate('2025-02-24') %} | ||
| X | ||
| {% endif %} |
There was a problem hiding this comment.
Remove debug/test code
This code block appears to be debug or test code checking for a hardcoded date (2025-02-24). It should be removed before merging to production.
- {% set a = getNow()|date('Y-m-d') %}
- {% if scope.closure.hasEntityByDate('2025-02-24') %}
- X
- {% endif %}📝 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.
| {% set a = getNow()|date('Y-m-d') %} | |
| {% if scope.closure.hasEntityByDate('2025-02-24') %} | |
| X | |
| {% endif %} |
| public function addConditionId($id) | ||
| { | ||
| $this->query->where('closure.id', '=', $id); | ||
| return $this; | ||
| } | ||
|
|
||
| public function addConditionClosureId($id) | ||
| { | ||
| $this->query->where('closure.id', '=', $id); | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant method
The methods addConditionId and addConditionClosureId are identical. This is redundant and can lead to confusion when maintaining the code.
Consider removing one of the methods and updating all references:
public function addConditionId($id)
{
$this->query->where('closure.id', '=', $id);
return $this;
}
- public function addConditionClosureId($id)
- {
- $this->query->where('closure.id', '=', $id);
- return $this;
- }If these methods are intended for different purposes, consider adding more detailed PHPDoc comments to explain the distinction between them.
📝 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.
| public function addConditionId($id) | |
| { | |
| $this->query->where('closure.id', '=', $id); | |
| return $this; | |
| } | |
| public function addConditionClosureId($id) | |
| { | |
| $this->query->where('closure.id', '=', $id); | |
| return $this; | |
| } | |
| public function addConditionId($id) | |
| { | |
| $this->query->where('closure.id', '=', $id); | |
| return $this; | |
| } |
| try { | ||
| $closure = (new Closure())->readByScopeIdAndDate($scopeId, new DateTime($date)); | ||
| } catch (\Exception $e) { | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling.
The current empty catch block silently swallows all exceptions, which could hide important errors and make debugging difficult.
try {
$closure = (new Closure())->readByScopeIdAndDate($scopeId, new DateTime($date));
} catch (\Exception $e) {
+ // Initialize with a new empty closure entity
+ $closure = new ClosureEntity();
}📝 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 { | |
| $closure = (new Closure())->readByScopeIdAndDate($scopeId, new DateTime($date)); | |
| } catch (\Exception $e) { | |
| } | |
| try { | |
| $closure = (new Closure())->readByScopeIdAndDate($scopeId, new DateTime($date)); | |
| } catch (\Exception $e) { | |
| // Initialize with a new empty closure entity | |
| $closure = new ClosureEntity(); | |
| } |
| $closure->existing = false; | ||
| $data['message'] = 'Closure has been deleted'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Property naming is confusing.
Similarly, setting existing = false for a deleted closure is counterintuitive.
(new Closure())->deleteEntity($closure->getId());
-$closure->existing = false;
+$closure->active = false;
$data['message'] = 'Closure has been deleted';📝 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.
| $closure->existing = false; | |
| $data['message'] = 'Closure has been deleted'; | |
| (new Closure())->deleteEntity($closure->getId()); | |
| $closure->active = false; | |
| $data['message'] = 'Closure has been deleted'; |
| $closure->existing = true; | ||
| $data['message'] = 'Closure has been created'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Property naming is confusing.
Setting existing = true for a newly created closure is counterintuitive and may lead to confusion.
$closure = (new Closure())->createOne($scopeId, new DateTime($date));
-$closure->existing = true;
+$closure->active = true;
$data['message'] = 'Closure has been created';📝 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.
| $closure->existing = true; | |
| $data['message'] = 'Closure has been created'; | |
| $closure = (new Closure())->createOne($scopeId, new DateTime($date)); | |
| $closure->active = true; | |
| $data['message'] = 'Closure has been created'; |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
zmsadmin/src/Zmsadmin/ScopeAvailabilityDayClosure.php (1)
29-32:⚠️ Potential issueAdd proper error handling for the HTTP request
The current implementation does not handle potential exceptions from the HTTP request, which could lead to unhandled exceptions and poor user experience.
Add a try-catch block to properly handle exceptions:
- $closureToggled = \App::$http->readPostResult( - '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/', - new \BO\Zmsentities\Closure() - )->getEntity(); + try { + $closureToggled = \App::$http->readPostResult( + '/scope/' . $scopeId . '/availability/' . $date . '/closure/toggle/', + new \BO\Zmsentities\Closure() + )->getEntity(); + } catch (\Exception $e) { + $this->logger->error('Failed to toggle closure: ' . $e->getMessage()); + return \BO\Slim\Render::withJson( + $response->withStatus(500), + ['error' => 'Failed to toggle closure status'] + ); + }zmsadmin/templates/page/availabilityday.twig (1)
67-70:⚠️ Potential issueRemove debug/test code
This code block appears to be debug or test code checking for a hardcoded date (2025-02-24). It should be removed before merging to production.
- {% set a = getNow()|date('Y-m-d') %} - {% if scope.closure.hasEntityByDate('2025-02-24') %} - X - {% endif %}
🧹 Nitpick comments (3)
zmsadmin/src/Zmsadmin/ScopeAvailabilityDayClosure.php (1)
11-12: Remove unused importsThe
ClosureandAvailabilityListimports are not used in this class.use BO\Slim\Render; -use BO\Zmsdb\Closure; -use BO\Zmsentities\Collection\AvailabilityList; use Psr\Http\Message\ResponseInterface;zmsadmin/templates/page/availabilityday.twig (2)
42-42: Improve confirmation dialog messageThe current confirmation message is not very descriptive. It could be improved to provide more context to the user about what they are about to do.
- const ok = confirm('Möchten Sie wirklich Die Terminbuchung für diesen Tag {% if locked %}entsperren{% else %}sperren{% endif %}?') + const ok = confirm('Möchten Sie wirklich die Terminbuchung für {{ dateString }} {% if locked %}entsperren? Benutzer werden wieder Termine für diesen Tag buchen können.{% else %}sperren? Benutzer werden keine neuen Termine für diesen Tag buchen können.{% endif %}')
33-38: Make the UI text more consistent and user-friendlyThe messages and button text could be improved to be more consistent and user-friendly.
- <div id="closure-close" class="message message--warning {% if not locked %}hidden{% endif %}"> - Die Terminbuchung für diesen Tag ist gesperrt. <input class="button btn" style="margin-top: -6px;" type="button" onclick="toggleClosure()" value="Entsperren"> + <div id="closure-close" class="message message--warning {% if not locked %}hidden{% endif %}"> + <strong>Gesperrt:</strong> Die Terminbuchung für den {{ dateString }} ist derzeit gesperrt. <input class="button btn" style="margin-top: -6px;" type="button" onclick="toggleClosure()" value="Entsperren"> </div> - <div id="closure-open" class="message message--info {% if locked %}hidden{% endif %}"> - Die Terminbuchung für diesen Tag ist NICHT gesperrt. <input class="button btn" style="margin-top: -6px;" onclick="toggleClosure()" type="button" value="Sperren"> + <div id="closure-open" class="message message--info {% if locked %}hidden{% endif %}"> + <strong>Verfügbar:</strong> Die Terminbuchung für den {{ dateString }} ist derzeit aktiv. <input class="button btn" style="margin-top: -6px;" onclick="toggleClosure()" type="button" value="Sperren"> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsadmin/src/Zmsadmin/ScopeAvailabilityDayClosure.php(1 hunks)zmsadmin/templates/page/availabilityday.twig(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- 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-owasp-security-checks / security-scan (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: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (1)
zmsadmin/src/Zmsadmin/ScopeAvailabilityDayClosure.php (1)
26-27: Validate input parametersThe
scopeIdanddateparameters are used directly without validation, which could lead to unexpected behavior if they contain invalid values.Consider adding validation for these parameters:
$scopeId = $args['id']; $date = $args['date']; + + // Validate scope ID is numeric + if (!is_numeric($scopeId)) { + return \BO\Slim\Render::withJson( + $response->withStatus(400), + ['error' => 'Invalid scope ID format'] + ); + } + + // Validate date format + if (!preg_match('/^\d{4}-\d{2}-\d{2}$/', $date)) { + return \BO\Slim\Render::withJson( + $response->withStatus(400), + ['error' => 'Invalid date format'] + ); + }
| }).fail(err => { | ||
| console.log('error', err) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in AJAX request
The current error handling only logs to the console but doesn't provide any feedback to the user when an error occurs.
}).fail(err => {
console.log('error', err)
+ alert('Ein Fehler ist aufgetreten. Bitte versuchen Sie es später erneut.')
})📝 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.
| }).fail(err => { | |
| console.log('error', err) | |
| }) | |
| }).fail(err => { | |
| console.log('error', err) | |
| alert('Ein Fehler ist aufgetreten. Bitte versuchen Sie es später erneut.') | |
| }) |
| const currentDate = '{{ dateString }}' | ||
| const url = `{{ includeUrl() }}/scope/{{ scope.id }}/availability/day/${currentDate}/closure/toggle/` | ||
| $.ajax(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| data: JSON.stringify({}) | ||
| }).done(data => { | ||
| if (data.closure.existing) { | ||
| document.getElementById('closure-close').classList.remove('hidden') | ||
| document.getElementById('closure-open').classList.add('hidden') | ||
| } else { | ||
| document.getElementById('closure-close').classList.add('hidden') | ||
| document.getElementById('closure-open').classList.remove('hidden') | ||
| } | ||
| }).fail(err => { | ||
| console.log('error', err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state to prevent multiple submissions
The current implementation does not disable the button during the AJAX request, which could lead to multiple submissions if the user clicks the button multiple times.
const currentDate = '{{ dateString }}'
const url = `{{ includeUrl() }}/scope/{{ scope.id }}/availability/day/${currentDate}/closure/toggle/`
+
+ // Disable buttons during request
+ const closeButton = document.querySelector('#closure-close input');
+ const openButton = document.querySelector('#closure-open input');
+ closeButton.disabled = true;
+ openButton.disabled = true;
+
$.ajax(url, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
data: JSON.stringify({})
}).done(data => {
if (data.closure.existing) {
document.getElementById('closure-close').classList.remove('hidden')
document.getElementById('closure-open').classList.add('hidden')
} else {
document.getElementById('closure-close').classList.add('hidden')
document.getElementById('closure-open').classList.remove('hidden')
}
+ // Re-enable buttons
+ closeButton.disabled = false;
+ openButton.disabled = false;
}).fail(err => {
console.log('error', err)
+ alert('Ein Fehler ist aufgetreten. Bitte versuchen Sie es später erneut.')
+ // Re-enable buttons
+ closeButton.disabled = false;
+ openButton.disabled = false;
})📝 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 currentDate = '{{ dateString }}' | |
| const url = `{{ includeUrl() }}/scope/{{ scope.id }}/availability/day/${currentDate}/closure/toggle/` | |
| $.ajax(url, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| data: JSON.stringify({}) | |
| }).done(data => { | |
| if (data.closure.existing) { | |
| document.getElementById('closure-close').classList.remove('hidden') | |
| document.getElementById('closure-open').classList.add('hidden') | |
| } else { | |
| document.getElementById('closure-close').classList.add('hidden') | |
| document.getElementById('closure-open').classList.remove('hidden') | |
| } | |
| }).fail(err => { | |
| console.log('error', err) | |
| }) | |
| } | |
| const currentDate = '{{ dateString }}' | |
| const url = `{{ includeUrl() }}/scope/{{ scope.id }}/availability/day/${currentDate}/closure/toggle/` | |
| // Disable buttons during request | |
| const closeButton = document.querySelector('#closure-close input'); | |
| const openButton = document.querySelector('#closure-open input'); | |
| closeButton.disabled = true; | |
| openButton.disabled = true; | |
| $.ajax(url, { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| data: JSON.stringify({}) | |
| }).done(data => { | |
| if (data.closure.existing) { | |
| document.getElementById('closure-close').classList.remove('hidden') | |
| document.getElementById('closure-open').classList.add('hidden') | |
| } else { | |
| document.getElementById('closure-close').classList.add('hidden') | |
| document.getElementById('closure-open').classList.remove('hidden') | |
| } | |
| // Re-enable buttons | |
| closeButton.disabled = false; | |
| openButton.disabled = false; | |
| }).fail(err => { | |
| console.log('error', err) | |
| alert('Ein Fehler ist aufgetreten. Bitte versuchen Sie es später erneut.') | |
| // Re-enable buttons | |
| closeButton.disabled = false; | |
| openButton.disabled = false; | |
| }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zmsdb/migrations/91740751372-create-closures-table.sql (3)
3-8: Column Definitions: Consider Enforcing NOT NULL ConstraintsThe columns
year,month,day, andStandortIDare critical for identifying closure records and are used in composite indexes. It is advisable to addNOT NULLconstraints to these columns to enforce data integrity unless aNULLvalue is explicitly required by your business logic.Proposed diff:
@@ - `year` SMALLINT(5), - `month` TINYINT(5), - `day` TINYINT(5), - `StandortID` INT(5) UNSIGNED, + `year` SMALLINT(5) NOT NULL, + `month` TINYINT(5) NOT NULL, + `day` TINYINT(5) NOT NULL, + `StandortID` INT(5) UNSIGNED NOT NULL, @@
9-11: Index Naming and Redundancy CheckThe table defines two indexes:
- A single-column index on
StandortID.- A composite index on
(StandortID, year, month, day).Since the composite index's leftmost column is
StandortID, it should also serve standalone queries filtering byStandortID. Verify if both indexes are necessary. Additionally, consider naming them explicitly (e.g.,idx_standortandidx_closure) for easier reference and maintenance during performance troubleshooting.Proposed diff for naming:
@@ - INDEX (StandortID), - INDEX (StandortID, year, month, day) + INDEX idx_standort (StandortID), + INDEX idx_closure (StandortID, year, month, day) @@
12-12: Statement Termination: Consider Adding a SemicolonIt is common practice to terminate the
CREATE TABLEstatement with a semicolon. Ensure that the migration tool or the SQL interpreter being used does not require an explicit semicolon at the end. If it does, consider adding one to align with standard SQL syntax.Proposed diff:
@@ -) +); @@
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsdb/migrations/91740751372-create-closures-table.sql(1 hunks)
⏰ 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-owasp-security-checks / security-scan (zmscitizenapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmscalldisplay, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmsentities, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsapi, 8.0)
- GitHub Check: call-owasp-security-checks / security-scan (zmsadmin, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmscitizenapi, 8.0)
- GitHub Check: call-php-unit-tests / module-test (zmscalldisplay, 8.0)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / module-test (zmsadmin, 8.0)
🔇 Additional comments (1)
zmsdb/migrations/91740751372-create-closures-table.sql (1)
1-2: Ensure Idempotency with Table DropThe
DROP TABLE IF EXISTSclause is correctly used to ensure that any pre-existing version of theclosurestable is removed before creation. This is a good practice in migration scripts.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
New Features
Chores