Zmskvr 670 tagessperre gesamtuebersicht #1420
Conversation
WalkthroughAdds closures support to the Overall Calendar: frontend closure tracking and rendering, parallel fetches of calendar and closures, an admin proxy route/controller, a new API GET /closure/ with DB query support, SCSS styles, a template legend, and tests/fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant FE as OverallCalendar.js
participant ZA as zmsadmin (proxy)
participant API as zmsapi /closure/
participant DB as zmsdb
U->>FE: Open or refresh calendar
rect rgba(240,247,255,1)
FE->>ZA: GET /overallcalendar/overallcalendarData/
FE->>ZA: GET /overallcalendar/closureData/?scopeIds&dateFrom&dateUntil
end
ZA-->>FE: calendar JSON
ZA->>API: GET /closure/?scopeIds&dateFrom&dateUntil
API->>DB: readByScopesInRange(scopeIds, from, until)
DB-->>API: [{scopeId,date},...]
API-->>ZA: closures JSON (+Last-Modified)
ZA-->>FE: closures JSON (+Last-Modified)
FE->>FE: update calendar cache & CLOSURES
FE->>FE: render calendar marking closed headers/cells
sequenceDiagram
autonumber
participant T as Timer/WS
participant FE as OverallCalendar.js
participant ZA as zmsadmin
participant API as zmsapi
participant DB as zmsdb
T->>FE: incremental update trigger
FE->>ZA: GET calendar delta
alt delta indicates closure refresh needed
FE->>ZA: GET /overallcalendar/closureData/?...
ZA->>API: GET /closure/?...
API->>DB: readByScopesInRange(...)
DB-->>API: items
API-->>ZA: closures JSON
ZA-->>FE: closures JSON
end
ZA-->>FE: calendar delta
FE->>FE: merge delta, update CLOSURES, re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 5
🧹 Nitpick comments (10)
zmsdb/src/Zmsdb/Query/Closure.php (2)
46-72: Use IN instead of chained OR for better readability and SQL planning.
The grouped OR conditions can be simplified to a single IN clause.Apply:
- $this->query->where(function ($conditions) use ($ids) { - $first = true; - foreach ($ids as $id) { - if ($first) { - $conditions->andWith('closure.StandortID', '=', $id); - $first = false; - } else { - $conditions->orWith('closure.StandortID', '=', $id); - } - } - }); + // If ConditionBuilder supports array values for IN: + $this->query->where('closure.StandortID', 'IN', $ids); + // Otherwise, fallback to a raw, but safe, expression (ids are ints): + // $this->query->where(self::expression('closure.StandortID IN (' . implode(',', $ids) . ')'));Please confirm that ConditionBuilder handles
INwith arrays; if not, use the raw expression fallback.
74-82: Date range filter may bypass indexes; consider index-friendly predicates.
DATE(CONCAT(...))on columns disables potential (year,month,day) index usage.Two options:
- Prefer tuple-like predicates using boundary logic to leverage a composite index on
(year, month, day).- Or introduce a generated column
full_date DATE AS (CAST(CONCAT(year,'-',LPAD(month,2,'0'),'-',LPAD(day,2,'0')) AS DATE))with an index and filter on it.zmsadmin/scss/overallCalendar.scss (1)
188-205: Nice, subtle closed-state styling.
Patterns and colors are consistent with existing cancelled stripes.
- Consider extracting the stripe gradient into a SCSS variable/mixin to avoid duplication with
.overall-calendar-cancelled.- Double-check content contrast when text is present over
.overall-calendar-closed.zmsapi/routing.php (1)
4204-4247: Swagger doc OK; keep style consistent with existing responses.
Most endpoints documentmeta/data; this one introducesitems. If the handler returns{ items: [...] }, fine—otherwise align docs to actual response.Also, for consistency with other blocks that use list-style tags, you may switch
tags: [closure, scope]to the multiline form used elsewhere.zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (1)
19-26: Standardize error responses and set charsetReturn JSON with utf-8 and a consistent structure.
- $response->getBody()->write(json_encode($error)); - return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); + $response->getBody()->write(json_encode($error, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES)); + return $response + ->withStatus(400) + ->withHeader('Content-Type', 'application/json; charset=utf-8');zmsapi/src/Zmsapi/AvailabilityClosureRead.php (1)
35-37: Last-Modified OK; consider deriving from data (optional)Using current time is fine; consider max of item timestamps if available.
zmsadmin/js/page/overallCalendar/overallCalendar.js (4)
35-49: Remove debug log and unused parameterDrop console.log and the unused fullReload parameter from fetchClosures’ signature.
-async function fetchClosures({scopeIds, dateFrom, dateUntil, fullReload = true}) { - console.log("fetchClosures") +async function fetchClosures({scopeIds, dateFrom, dateUntil}) {
203-219: Don’t let closures fetch failure block incremental updatesRender calendar even if closures refresh fails; log the error.
- await fetchClosures({ scopeIds, dateFrom, dateUntil }); - renderMultiDayCalendar(calendarCache); + try { + await fetchClosures({ scopeIds, dateFrom, dateUntil }); + } catch (e) { + console.error('Closures refresh failed', e); + } + renderMultiDayCalendar(calendarCache);
267-269: Use the same date helper everywhereReuse ymdLocalFromUnix to avoid duplicated locale logic.
- const ymdLocalFromUnix = (ts) => new Date(ts * 1000).toLocaleDateString('sv-SE'); + const ymdLocalFromUnix = (ts) => new Date(ts * 1000).toLocaleDateString('sv-SE'); @@ - const dateIsoForDay = new Date(day.date * 1000).toLocaleDateString('sv-SE'); + const dateIsoForDay = ymdLocalFromUnix(day.date);Also applies to: 335-345
76-81: Parallel fetching is good; consider AbortController to avoid race conditions (optional)Cancelling in-flight requests on new submissions prevents stale renders.
Also applies to: 178-184
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
zmsadmin/js/page/overallCalendar/overallCalendar.js(11 hunks)zmsadmin/routing.php(1 hunks)zmsadmin/scss/overallCalendar.scss(1 hunks)zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php(1 hunks)zmsapi/routing.php(1 hunks)zmsapi/src/Zmsapi/AvailabilityClosureRead.php(1 hunks)zmsdb/src/Zmsdb/Closure.php(1 hunks)zmsdb/src/Zmsdb/Query/Closure.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsadmin/scss/overallCalendar.scsszmsdb/src/Zmsdb/Closure.phpzmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.phpzmsadmin/routing.phpzmsapi/src/Zmsapi/AvailabilityClosureRead.phpzmsdb/src/Zmsdb/Query/Closure.phpzmsapi/routing.phpzmsadmin/js/page/overallCalendar/overallCalendar.js
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Closure.phpzmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.phpzmsadmin/routing.phpzmsapi/src/Zmsapi/AvailabilityClosureRead.phpzmsdb/src/Zmsdb/Query/Closure.phpzmsapi/routing.php
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{js,jsx,ts,tsx}: Flag any usage of console.log() as it should be replaced with proper logging or removed:
- For development: console.debug()
- For production: Remove console.log() statements or use structured logging
- For errors: Use error console.error()
Example replacement:
// Instead of: console.log('User data:', userData); // Use: console.debug('Processing user data', { userData }); // or for development only: Remove the console.log entirelyFlag specific logging violations:
- console.log(), console.debug(), console.warn() usage (except console.error in catch blocks)
- Any debug output that should use proper logging frameworks
Example replacements:
// Instead of: console.log('User data:', userData); console.debug('Processing...'); // Use: // Remove console.log entirely or use proper logging // Only console.error in catch blocks is acceptable try { processData(userData); } catch (error) { console.error('Processing failed:', error); }Flag JavaScript security and UX issues:
- alert(), confirm(), prompt() usage (poor UX)
- eval() usage (security risk)
- innerHTML with user input (XSS risk)
- Unfiltered user input in DOM manipulation
Example replacements:
// Instead of: alert('Error occurred'); eval(userInput); // Use: // Use proper error handling and UI components this.showErrorNotification('Error occurred'); // Avoid eval() entirelyFlag TODO/FIXME comments that indicate technical debt:
- TODO comments without clear action items
- FIXME comments indicating broken functionality
- HACK comments indicating temporary workarounds
- XXX comments indicating problematic code
These should be converted to proper issues or addressed:
// Instead of: // TODO: fix this later // FIXME: this is broken // Use: // Create proper issue and reference it // @see Issue #123: Refactor validation logicFlag code complexity issues:
- Functions longer than 50 lines
- Deep nesting (...
Files:
zmsadmin/js/page/overallCalendar/overallCalendar.js
🧬 Code graph analysis (5)
zmsdb/src/Zmsdb/Closure.php (3)
zmsdb/src/Zmsdb/Query/Closure.php (4)
Closure(8-107)addSelectVirtualDate(84-92)addConditionScopeIds(46-72)addConditionDateRange(74-82)zmsentities/src/Zmsentities/Closure.php (2)
Closure(5-50)getDateTime(28-31)zmsdb/src/Zmsdb/Query/Base.php (1)
addEntityMapping(340-345)
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (2)
zmsclient/src/Zmsclient/Http.php (1)
Http(16-260)zmsclient/src/Zmsclient/Result.php (1)
getResponse(131-134)
zmsadmin/routing.php (1)
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (1)
OverallCalendarClosureLoadData(8-47)
zmsapi/src/Zmsapi/AvailabilityClosureRead.php (4)
zmscitizenapi/src/Zmscitizenapi/BaseController.php (1)
BaseController(14-87)mellon/src/Mellon/Validator.php (1)
param(127-131)mellon/src/Mellon/ValidDate.php (1)
isDate(23-37)zmsdb/src/Zmsdb/Closure.php (1)
readByScopesInRange(38-59)
zmsdb/src/Zmsdb/Query/Closure.php (4)
zmsadmin/js/page/overallCalendar/overallCalendar.js (2)
scopeIds(152-152)ids(9-9)zmsdb/src/Zmsdb/Query/Builder/Where.php (1)
where(47-57)zmsdb/src/Zmsdb/Query/Base.php (2)
expression(330-333)getPrefixed(347-350)zmsdb/src/Zmsdb/Query/Builder/ConditionBuilder.php (3)
conditions(101-104)andWith(35-39)orWith(49-53)
🪛 GitHub Actions: Combined Workflow
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php
[error] 1-1: End of line character is invalid; expected "\n" but found "\r\n". PHPCS command: vendor/bin/phpcs --standard=psr12 --warning-severity=0 src/.
[error] 47-47: Expected 1 newline at end of file; 0 found. PHPCS command: vendor/bin/phpcs --standard=psr12 --warning-severity=0 src/.
zmsapi/src/Zmsapi/AvailabilityClosureRead.php
[error] 1-1: PHPCS: End of line character is invalid (CRLF); expected LF but found CRLF in zmsapi/src/Zmsapi/AvailabilityClosureRead.php.
🔇 Additional comments (8)
zmsdb/src/Zmsdb/Query/Closure.php (2)
24-29: Add mapping for scopeId looks good.
Matches downstream consumption and keeps names consistent.
84-92: Use unprefixed “date” alias for virtual date
Replace the alias key fromgetPrefixed('date')(e.g.closure.date) to plain'date', since downstream readers (e.g.readByScopesInRange) access$e['date'], not$e['closure.date'].- $this->query->select([ - $this->getPrefixed('date') => self::expression( + $this->query->select([ + 'date' => self::expression( "DATE(CONCAT(closure.year,'-',LPAD(closure.month,2,'0'),'-',LPAD(closure.day,2,'0')))" ]);Verify other virtual‐column selects for consistent alias usage.
zmsadmin/routing.php (1)
57-61: Route wiring looks correct and consistent with existing Overall-Calendar endpoints.
Name and path align with current conventions.zmsadmin/scss/overallCalendar.scss (1)
202-205: Header closed-state highlight LGTM.
Class naming matches JS usage (is-closed).zmsdb/src/Zmsdb/Closure.php (1)
38-59: Ensure safe access on returned Entities
Use object properties or getters instead of array indexing (or implement ArrayAccess) to avoid fatal errors ifEntitydoesn’t support array access. For example:foreach ($entities as $e) { $scopeId = $e->scopeId ?? 0; $dateVal = $e->date ?? null; $date = $dateVal ? (string)$dateVal : $e->getDateTime()->format('Y-m-d'); $out[] = [ 'scopeId' => (int)$scopeId, 'date' => $date, ]; }Confirm whether your
Schema\EntityimplementsArrayAccess.
Optionally, add a deterministicORDER BYto the query:$q->orderBy('closure.year ASC, closure.month ASC, closure.day ASC, closure.StandortID ASC');zmsapi/routing.php (1)
4244-4247: Route registration LGTM.
Handler and name are clear; path fits API grouping.zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (1)
37-41: Good: Last-Modified passthroughPropagating the upstream Last-Modified enables client caching. LGTM.
zmsadmin/js/page/overallCalendar/overallCalendar.js (1)
428-430: Closure lookup approach LGTMSet-based keying keeps checks O(1) and simple.
…ange in AvailabilityClosureRead , Use PSR-7 query parsing and forward Last-Modified header in OverallCalendarClosureLoadData
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (1)
1-1: Fix CRLF and add trailing newline (pipeline).
Keep LF endings and EOF newline to satisfy PSR-12 checks.
🧹 Nitpick comments (9)
zmsapi/tests/Zmsapi/AvailabilityClosureReadTest.php (3)
23-25: Relax Content-Type equality to tolerate charsets.
Strict equality may fail if the controller returns "application/json; charset=utf-8".Apply:
- $this->assertEquals('application/json', $response->getHeaderLine('Content-Type')); + $this->assertStringContainsString('application/json', $response->getHeaderLine('Content-Type'));
11-40: Minor: consolidate item checks for clarity.
Use a lookup to avoid loop-side flags.- $have58 = false; - $have59 = false; - foreach ($body['data']['items'] as $it) { - if ((int)$it['scopeId'] === 58 && $it['date'] === '2025-09-03') $have58 = true; - if ( (int)$it['scopeId'] === 59 && $it['date'] === '2025-09-04') $have59 = true; - } + $pairs = array_map(fn($it) => ((int)$it['scopeId']).'|'.$it['date'], $body['data']['items']); + $have58 = in_array('58|2025-09-03', $pairs, true); + $have59 = in_array('59|2025-09-04', $pairs, true);
1-71: Optional: add missing-parameter test at API level.
You already cover bad range and bad scopeIds; a "missing scopeIds/dateFrom/dateUntil → 400" test would round out validation.zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (3)
5-6: Avoid superglobals; use PSR-7 query params.
Rely on the request, not $_GET, for testability and consistency.-use Psr\Http\Message\RequestInterface; +use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; @@ - public function readResponse( - RequestInterface $request, + public function readResponse( + ServerRequestInterface $request, ResponseInterface $response, array $args ) { - $query = []; - $uriQuery = $request->getUri()->getQuery(); - if ($uriQuery !== '') { - parse_str($uriQuery, $query); - } - - $scopeIds = $query['scopeIds'] ?? ($_GET['scopeIds'] ?? null); - $dateFrom = $query['dateFrom'] ?? ($_GET['dateFrom'] ?? null); - $dateUntil = $query['dateUntil'] ?? ($_GET['dateUntil'] ?? null); + $query = $request->getQueryParams(); + $scopeIds = $query['scopeIds'] ?? null; + $dateFrom = $query['dateFrom'] ?? null; + $dateUntil = $query['dateUntil'] ?? null;Also applies to: 10-14, 15-23
50-58: Forward common caching headers from upstream.
Propagate ETag/Cache-Control/Expires to let clients cache effectively.$contentType = $apiResp->getHeaderLine('Content-Type'); $response = $response->withHeader( 'Content-Type', $contentType !== '' ? $contentType : 'application/json' ); + foreach (['ETag', 'Cache-Control', 'Expires'] as $h) { + if ('' !== ($v = $apiResp->getHeaderLine($h))) { + $response = $response->withHeader($h, $v); + } + }
60-66: Prefer explicit getContents() after rewind.
Casting to string is fine but less explicit.- $bodyStream = $apiResp->getBody(); - if ($bodyStream->isSeekable()) { - $bodyStream->rewind(); - } - $rawBody = (string) $bodyStream; + $bodyStream = $apiResp->getBody(); + if ($bodyStream->isSeekable()) { + $bodyStream->rewind(); + } + $rawBody = $bodyStream->getContents();zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php (3)
18-19: Relax header equality to tolerate charsets.
Prevent brittleness if upstream sets "application/json; charset=utf-8".- $this->assertEquals('application/json', $response->getHeaderLine('Content-Type')); + $this->assertStringContainsString('application/json', $response->getHeaderLine('Content-Type'));Also applies to: 34-35, 70-71, 107-109
45-83: Reduce duplication between valid-path tests.
testValidRequestandtestResponseStructureshare setup; extract a helper to arrange stubbed GET /closure/.
1-116: Optional: add passthrough header/status coverage.
Add a test asserting Last-Modified and non-200 statuses are forwarded when present.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
zmsdb/tests/Zmsdb/fixtures/mysql_mandantory_unittests_data.sql.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php(1 hunks)zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php(1 hunks)zmsadmin/tests/Zmsadmin/fixtures/GET_Closure_Data.json(1 hunks)zmsapi/src/Zmsapi/AvailabilityClosureRead.php(1 hunks)zmsapi/tests/Zmsapi/AvailabilityClosureReadTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsapi/src/Zmsapi/AvailabilityClosureRead.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsadmin/tests/Zmsadmin/fixtures/GET_Closure_Data.jsonzmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.phpzmsapi/tests/Zmsapi/AvailabilityClosureReadTest.phpzmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.phpzmsapi/tests/Zmsapi/AvailabilityClosureReadTest.phpzmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php
🧬 Code graph analysis (3)
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php (1)
zmscitizenapi/tests/Zmscitizenapi/HttpMockTrait.php (1)
setApiCalls(8-50)
zmsapi/tests/Zmsapi/AvailabilityClosureReadTest.php (1)
zmscitizenapi/src/Zmscitizenapi/Helper/ErrorHandler.php (1)
getStatusCode(39-46)
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php (2)
zmsclient/src/Zmsclient/Http.php (1)
Http(16-260)zmsclient/src/Zmsclient/Result.php (1)
getResponse(131-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (1)
zmsadmin/tests/Zmsadmin/fixtures/GET_Closure_Data.json (1)
1-15: LGTM: Fixture shape and sample items are consistent with tests.
Values and keys align with expected response; good for deterministic tests.
…e-gesamtuebersicht
…e-gesamtuebersicht
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
zmsadmin/js/page/overallCalendar/overallCalendar.js (5)
35-48: Remove unused param and tighten i18n/error handling in fetchClosures.
- fullReload is unused; drop it.
- Align wording to German domain terms (“Schließtage” instead of “Closures”).
- Minor: include status text for easier troubleshooting.
-async function fetchClosures({scopeIds, dateFrom, dateUntil, fullReload = true}) { +async function fetchClosures({scopeIds, dateFrom, dateUntil}) { const res = await fetch(`closureData/?${new URLSearchParams({ scopeIds: scopeIds.join(','), dateFrom, dateUntil })}`); - if (!res.ok) throw new Error('Fehler beim Laden der Closures'); + if (!res.ok) { + throw new Error(`Fehler beim Laden der Schließtage (${res.status} ${res.statusText})`); + } const { data } = await res.json(); const set = new Set(); for (const it of (data?.items || [])) { set.add(`${it.date}|${it.scopeId}`); } CLOSURES = set; }
70-83: Replace alert() with accessible inline error UI.Use the existing #scope-error container with aria-live instead of blocking alerts.
- } catch (e) { - alert('Fehler beim Aktualisieren: ' + e.message); - } + } catch (e) { + showInlineError('Fehler beim Aktualisieren: ' + e.message); + }- if (workdays > 5) { - alert('Bitte wählen Sie maximal 5 Werktage (Mo–Fr) aus.'); - return; - } + if (workdays > 5) { + showInlineError('Bitte wählen Sie maximal 5 Werktage (Mo–Fr) aus.'); + return; + }- } catch (e) { - alert(e.message); - } + } catch (e) { + showInlineError(e.message || 'Unbekannter Fehler.'); + }Add this helper (place once near the top-level of this module):
function showInlineError(msg) { const box = document.getElementById('scope-error'); if (!box) return; const msgNode = box.querySelector('.msg'); if (msgNode) msgNode.textContent = msg; box.setAttribute('role', 'alert'); box.setAttribute('aria-live', 'polite'); box.style.display = 'inline-flex'; }Also applies to: 165-175, 184-186
178-183: Nice parallelization on submit; consider abort/disable to avoid duplicate requests.
- Disable the submit+refresh buttons while Promise.all runs; re-enable in finally.
- Optionally use AbortController to cancel in-flight fetches when a new submit occurs.
Would you like a minimal AbortController patch for fetchCalendar/fetchClosures?
333-344: Reuse the ymd helper to avoid drift and duplicate logic.- const dateIsoForDay = new Date(day.date * 1000).toLocaleDateString('sv-SE'); + const dateIsoForDay = ymdLocalFromUnix(day.date); day.scopes.forEach((scope, scopeIdx) => {
258-419: Batch DOM writes for renderMultiDayCalendar to reduce layout thrash.Build in a DocumentFragment, append once at the end. This typically yields noticeable gains with large grids.
const frag = document.createDocumentFragment(); // replace container.appendChild(div) inside addCell with: frag.appendChild(div) // ... container.appendChild(frag);zmsadmin/templates/page/overallCalendar.twig (2)
75-85: Avoid inline styles; use SCSS classes and semantic markup.
- Drop inline styles to keep styling in SCSS.
- Optional: render as a list for better semantics/accessibility.
- <div class="overall-calendar-legend" style="margin:10px 0;"> + <div class="overall-calendar-legend"> <strong>Legende:</strong> - <span class="overall-calendar-seat overall-calendar-closed" style="display:inline-block; width:80px; height:18px; margin-left:8px; vertical-align:middle;"></span> + <span class="overall-calendar-seat overall-calendar-closed"></span> <span style="margin-right:12px;">Schließtage</span> - <span class="overall-calendar-seat overall-calendar-cancelled" style="display:inline-block; width:80px; height:18px; vertical-align:middle;"></span> + <span class="overall-calendar-seat overall-calendar-cancelled"></span> <span style="margin-right:12px;">Storniert</span> - <span class="overall-calendar-seat overall-calendar-open" style="display:inline-block; width:80px; height:18px; vertical-align:middle;"></span> + <span class="overall-calendar-seat overall-calendar-open"></span> <span>Freie Zeiten</span> </div>
16-23: Guard back-button against external referrers.Prevent navigating to third-party origins; fall back to the safe internal href.
- document.getElementById('back-button').addEventListener('click', function(e) { - if (document.referrer && document.referrer !== window.location.href) { - e.preventDefault(); - window.location.href = document.referrer; - } - }); + document.getElementById('back-button').addEventListener('click', function(e) { + const ref = document.referrer; + if (!ref || ref === window.location.href) return; + try { + const u = new URL(ref); + if (u.origin === window.location.origin) { + e.preventDefault(); + window.location.href = ref; + } + } catch (_) { /* ignore malformed referrers */ } + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
zmsadmin/js/page/overallCalendar/overallCalendar.js(11 hunks)zmsadmin/scss/overallCalendar.scss(1 hunks)zmsadmin/templates/page/overallCalendar.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsadmin/scss/overallCalendar.scss
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsadmin/templates/page/overallCalendar.twigzmsadmin/js/page/overallCalendar/overallCalendar.js
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{js,jsx,ts,tsx}: Flag any usage of console.log() as it should be replaced with proper logging or removed:
- For development: console.debug()
- For production: Remove console.log() statements or use structured logging
- For errors: Use error console.error()
Example replacement:
// Instead of: console.log('User data:', userData); // Use: console.debug('Processing user data', { userData }); // or for development only: Remove the console.log entirelyFlag specific logging violations:
- console.log(), console.debug(), console.warn() usage (except console.error in catch blocks)
- Any debug output that should use proper logging frameworks
Example replacements:
// Instead of: console.log('User data:', userData); console.debug('Processing...'); // Use: // Remove console.log entirely or use proper logging // Only console.error in catch blocks is acceptable try { processData(userData); } catch (error) { console.error('Processing failed:', error); }Flag JavaScript security and UX issues:
- alert(), confirm(), prompt() usage (poor UX)
- eval() usage (security risk)
- innerHTML with user input (XSS risk)
- Unfiltered user input in DOM manipulation
Example replacements:
// Instead of: alert('Error occurred'); eval(userInput); // Use: // Use proper error handling and UI components this.showErrorNotification('Error occurred'); // Avoid eval() entirelyFlag TODO/FIXME comments that indicate technical debt:
- TODO comments without clear action items
- FIXME comments indicating broken functionality
- HACK comments indicating temporary workarounds
- XXX comments indicating problematic code
These should be converted to proper issues or addressed:
// Instead of: // TODO: fix this later // FIXME: this is broken // Use: // Create proper issue and reference it // @see Issue #123: Refactor validation logicFlag code complexity issues:
- Functions longer than 50 lines
- Deep nesting (...
Files:
zmsadmin/js/page/overallCalendar/overallCalendar.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (5)
zmsadmin/js/page/overallCalendar/overallCalendar.js (5)
6-6: LGTM on introducing CLOSURES set.Appropriate lightweight structure for O(1) lookups.
124-145: Incremental/full reload flow looks sound.Parameterization and updateAfter handling are clear; merging vs. replace is correct.
266-267: Timezone/data-key consistency check.Using toLocaleDateString('sv-SE') yields local-time YYYY-MM-DD. Confirm backend closure dates (it.date) are the same local day to avoid off-by-one around midnight/timezone edges.
If backend dates are UTC, consider using toISOString().slice(0,10) on a UTC-normalized Date.
376-403: LGTM on closed-slot styling hook.Conditional class concat is clean and readable.
425-428: LGTM on isScopeClosed.Simple, fast membership check aligned with CLOSURES format.
| async function fetchIncrementalUpdate() { | ||
| if (!currentRequest || !lastUpdateAfter) return; | ||
| const {scopeIds, dateFrom, dateUntil} = currentRequest; | ||
| const params = new URLSearchParams({ | ||
| const { scopeIds, dateFrom, dateUntil } = currentRequest; | ||
|
|
||
| const res = await fetch(`overallcalendarData/?${new URLSearchParams({ | ||
| scopeIds: scopeIds.join(','), | ||
| dateFrom, | ||
| dateUntil, | ||
| updateAfter: lastUpdateAfter | ||
| }); | ||
| const res = await fetch(`overallcalendarData/?${params}`); | ||
| if (!res.ok) return; | ||
| })}`); | ||
| if (res.ok) { | ||
| lastUpdateAfter = toMysql(res.headers.get('Last-Modified') || new Date()); | ||
| const json = await res.json(); | ||
| mergeDelta(json.data.days); | ||
| } | ||
|
|
||
| lastUpdateAfter = toMysql(res.headers.get('Last-Modified') || new Date()); | ||
| const json = await res.json(); | ||
| mergeDelta(json.data.days); | ||
| await fetchClosures({ scopeIds, dateFrom, dateUntil }); | ||
| renderMultiDayCalendar(calendarCache); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent overlapping auto-refresh calls.
Auto-refresh every 60s can overlap on slow networks. Add an in-flight guard with try/finally.
async function fetchIncrementalUpdate() {
- if (!currentRequest || !lastUpdateAfter) return;
+ if (!currentRequest || !lastUpdateAfter || incrementalRefreshInFlight) return;
+ incrementalRefreshInFlight = true;
const { scopeIds, dateFrom, dateUntil } = currentRequest;
- const res = await fetch(`overallcalendarData/?${new URLSearchParams({
- scopeIds: scopeIds.join(','),
- dateFrom,
- dateUntil,
- updateAfter: lastUpdateAfter
- })}`);
- if (res.ok) {
- lastUpdateAfter = toMysql(res.headers.get('Last-Modified') || new Date());
- const json = await res.json();
- mergeDelta(json.data.days);
- }
-
- await fetchClosures({ scopeIds, dateFrom, dateUntil });
- renderMultiDayCalendar(calendarCache);
+ try {
+ const res = await fetch(`overallcalendarData/?${new URLSearchParams({
+ scopeIds: scopeIds.join(','),
+ dateFrom,
+ dateUntil,
+ updateAfter: lastUpdateAfter
+ })}`);
+ if (res.ok) {
+ lastUpdateAfter = toMysql(res.headers.get('Last-Modified') || new Date());
+ const json = await res.json();
+ mergeDelta(json.data.days);
+ }
+ await fetchClosures({ scopeIds, dateFrom, dateUntil });
+ renderMultiDayCalendar(calendarCache);
+ } finally {
+ incrementalRefreshInFlight = false;
+ }
}Add once near the top with other module-level vars:
let incrementalRefreshInFlight = false;🤖 Prompt for AI Agents
In zmsadmin/js/page/overallCalendar/overallCalendar.js around lines 200 to 218,
the incremental auto-refresh can start overlapping on slow networks; declare a
module-level boolean let incrementalRefreshInFlight = false near the other
top-level vars, then at the start of fetchIncrementalUpdate return immediately
if incrementalRefreshInFlight is true, set incrementalRefreshInFlight = true
before starting the async work, wrap the existing
fetch/merge/fetchClosures/render logic in a try block and set
incrementalRefreshInFlight = false in a finally block so the flag is always
cleared even on errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php (8)
7-9: Centralize auth setup/cleanup to reduce duplication and global-state bleed.Move Authorization header setup to setUp() and clear it in tearDown(). Then drop per-test header assignments.
If Base already provides auth(), prefer that instead.
class OverallCalendarClosureLoadDataTest extends Base { protected $arguments = []; protected $parameters = []; protected $classname = "OverallCalendarClosureLoadData"; + + protected function setUp(): void + { + parent::setUp(); + $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer validtoken'; + } + + protected function tearDown(): void + { + unset($_SERVER['HTTP_AUTHORIZATION']); + $_GET = []; + parent::tearDown(); + } @@ - $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer validtoken'; @@ - $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer validtoken'; @@ - $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer validtoken'; @@ - $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer validtoken';Also applies to: 13-13, 27-27, 48-48, 87-87
37-43: Assert presence of 'message' before substring checks.Prevents notices if the key is missing and clarifies failure.
$body = json_decode((string)$response->getBody(), true); $this->assertIsArray($body); $this->assertArrayHasKey('error', $body); $this->assertTrue($body['error']); + $this->assertArrayHasKey('message', $body); $this->assertStringContainsString('dateFrom', $body['message']); $this->assertStringContainsString('dateUntil', $body['message']);
19-19: Avoid brittle equality on Content-Type; allow charset.Some servers append “; charset=UTF-8”. Match prefix instead of exact equality.
- $this->assertEquals('application/json', $response->getHeaderLine('Content-Type')); + $this->assertStringStartsWith('application/json', $response->getHeaderLine('Content-Type'));Also applies to: 35-35, 71-71, 108-108
55-66: Mock matching is order-sensitive; ensure future-proofing.HttpMockTrait uses strict array equality for 'parameters'. If the controller changes param order, stubs will stop matching. Either keep this exact order or relax the comparison in the trait.
Trait-side improvement (outside this file):
- if ($response['function'] === 'readGetResult' && - $response['url'] === $url && - $response['parameters'] === $parameters) { + if ($response['function'] === 'readGetResult' && + $response['url'] === $url) { + $expected = $response['parameters']; + $actual = $parameters; + ksort($expected); + ksort($actual); + if ($expected === $actual) { … - } + } }
73-83: Optional: Strengthen data assertions.Validate basic types/format for first item.
$first = $body['data']['items'][0]; $this->assertArrayHasKey('scopeId', $first); $this->assertArrayHasKey('date', $first); + $this->assertIsInt($first['scopeId']); + $this->assertMatchesRegularExpression('/^\d{4}-\d{2}-\d{2}$/', $first['date']);
94-105: DRY: Extract a helper to stub the closure API once.Reduces duplication across tests.
- $this->setApiCalls([ - [ - 'function' => 'readGetResult', - 'url' => '/closure/', - 'parameters' => [ - 'scopeIds' => '58,59', - 'dateFrom' => '2025-09-02', - 'dateUntil' => '2025-09-05', - ], - 'response' => $this->readFixture('GET_Closure_Data.json'), - ], - ]); + $this->mockClosureApi('58,59', '2025-09-02', '2025-09-05', 'GET_Closure_Data.json');Add this private helper in the test class:
private function mockClosureApi(string $scopeIds, string $dateFrom, string $dateUntil, string $fixture): void { $this->setApiCalls([[ 'function' => 'readGetResult', 'url' => '/closure/', 'parameters' => [ 'scopeIds' => $scopeIds, 'dateFrom' => $dateFrom, 'dateUntil' => $dateUntil, ], 'response' => $this->readFixture($fixture), ]]); }
107-109: Also assert HTTP 200 in structure test.Completes the contract check.
- $response = $this->render([], [], []); - $this->assertEquals('application/json', $response->getHeaderLine('Content-Type')); + $response = $this->render([], [], []); + $this->assertEquals(200, $response->getStatusCode()); + $this->assertStringStartsWith('application/json', $response->getHeaderLine('Content-Type'));
49-53: Minimize direct superglobal writes in tests.If the controller supports it, prefer passing query params via render()’s parameter argument to avoid test pollution and improve readability (aligns with zmsapi tests).
Also applies to: 89-92
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php(1 hunks)zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php(1 hunks)zmsdb/src/Zmsdb/Closure.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsdb/src/Zmsdb/Closure.php
- zmsadmin/src/Zmsadmin/OverallCalendarClosureLoadData.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php
🧬 Code graph analysis (1)
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php (2)
zmsapi/tests/Zmsapi/AvailabilityClosureReadTest.php (1)
testRendering(23-40)zmscitizenapi/tests/Zmscitizenapi/HttpMockTrait.php (1)
setApiCalls(8-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (1)
zmsadmin/tests/Zmsadmin/OverallCalendarClosureLoadDataTest.php (1)
11-23: LGTM: No-params path returns an empty JSON array as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zmsadmin/templates/page/overallCalendar.twig (1)
76-84: Optional: prepare for translationsIf i18n is planned, externalize the static labels.
Example Twig change:
- <strong id="overall-calendar-legend-title" class="me-2">Legende:</strong> + <strong id="overall-calendar-legend-title" class="me-2">{{ 'overallCalendar.legend'|trans }}</strong> - <span class="me-3">gesperrt</span> + <span class="me-3">{{ 'overallCalendar.closed'|trans }}</span> - <span class="me-3">storniert</span> + <span class="me-3">{{ 'overallCalendar.cancelled'|trans }}</span> - <span>frei</span> + <span>{{ 'overallCalendar.open'|trans }}</span>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
zmsadmin/templates/page/overallCalendar.twig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsadmin/templates/page/overallCalendar.twig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (1)
zmsadmin/templates/page/overallCalendar.twig (1)
77-84: All calendar status classes defined in SCSS
Styles for.overall-calendar-open,.overall-calendar-cancelled, and.overall-calendar-closedare present inzmsadmin/scss/overallCalendar.scss.
| <div class="overall-calendar-legend" style="margin:10px 0;"> | ||
| <strong>Legende:</strong> | ||
| <span class="overall-calendar-seat overall-calendar-closed" style="display:inline-block; width:80px; height:18px; margin-left:8px; vertical-align:middle;"></span> | ||
| <span style="margin-right:12px;">gesperrt</span> | ||
|
|
||
| <span class="overall-calendar-seat overall-calendar-cancelled" style="display:inline-block; width:80px; height:18px; vertical-align:middle;"></span> | ||
| <span style="margin-right:12px;">storniert</span> | ||
|
|
||
| <span class="overall-calendar-seat overall-calendar-open" style="display:inline-block; width:80px; height:18px; vertical-align:middle;"></span> | ||
| <span>frei</span> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace inline styles and add accessible semantics to the legend
Move presentational styles to SCSS and add ARIA for screen readers. Also reuse Bootstrap spacing utilities for consistency.
Apply this diff to the Twig:
- <div class="overall-calendar-legend" style="margin:10px 0;">
- <strong>Legende:</strong>
- <span class="overall-calendar-seat overall-calendar-closed" style="display:inline-block; width:80px; height:18px; margin-left:8px; vertical-align:middle;"></span>
- <span style="margin-right:12px;">gesperrt</span>
-
- <span class="overall-calendar-seat overall-calendar-cancelled" style="display:inline-block; width:80px; height:18px; vertical-align:middle;"></span>
- <span style="margin-right:12px;">storniert</span>
-
- <span class="overall-calendar-seat overall-calendar-open" style="display:inline-block; width:80px; height:18px; vertical-align:middle;"></span>
- <span>frei</span>
- </div>
+ <div class="overall-calendar-legend my-2" role="group" aria-labelledby="overall-calendar-legend-title">
+ <strong id="overall-calendar-legend-title" class="me-2">Legende:</strong>
+ <span class="overall-calendar-seat overall-calendar-closed overall-calendar-legend-swatch ms-2" aria-hidden="true"></span>
+ <span class="me-3">gesperrt</span>
+
+ <span class="overall-calendar-seat overall-calendar-cancelled overall-calendar-legend-swatch" aria-hidden="true"></span>
+ <span class="me-3">storniert</span>
+
+ <span class="overall-calendar-seat overall-calendar-open overall-calendar-legend-swatch" aria-hidden="true"></span>
+ <span>frei</span>
+ </div>Add this to SCSS (overallCalendar.scss):
.overall-calendar-legend-swatch {
display: inline-block;
width: 80px;
height: 18px;
vertical-align: middle;
}🤖 Prompt for AI Agents
In zmsadmin/templates/page/overallCalendar.twig around lines 75 to 85, remove
the inline style attributes on the legend container and swatch spans, replace
presentational spacing with Bootstrap spacing utility classes (e.g. me-3, ms-2)
and apply the new SCSS class overall-calendar-legend-swatch to each swatch span;
also add accessible semantics by marking the legend container as role="list" and
each legend item as role="listitem" (or add aria-label attributes) so screen
readers can understand the color key. Update the associated overallCalendar.scss
with the provided .overall-calendar-legend-swatch rules and ensure the existing
color modifier classes (overall-calendar-closed, -cancelled, -open) only set
background-color/border, not layout.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit