feat(ZMSKVR-1378): add Terminkapazität scope capacity report#2517
Open
ThomasAFink wants to merge 8 commits into
Open
feat(ZMSKVR-1378): add Terminkapazität scope capacity report#2517ThomasAFink wants to merge 8 commits into
ThomasAFink wants to merge 8 commits into
Conversation
Introduce the Terminkapazität Auswertung with multi-scope aggregation, slotscope warehouse queries (hourly up to 14 days), chart timeline filling, adaptive x-axis label intervals, scope date bounds in the filter, and an AJAX refresh control beside download.
Contributor
📝 WalkthroughWalkthroughThis PR adds an appointment capacity report: DB queries and Exchange entity, a ReportCapacityService to fetch/normalize/aggregate hourly/daily data, controller and route, frontend date-scoping and validation, Chart.js v3 rendering, templates/navigation, and tests with fixtures. ChangesAppointment Capacity Report Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
zmsstatistic/tests/Zmsstatistic/OverviewTest.php (1)
70-78: ⚡ Quick winAdd an assertion for the new capacity-report content on the overview page.
The test now mocks capacity endpoints (Lines 53–67) but never asserts that the new capacity section/link is rendered. This weakens regression detection for the new Overview integration path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/tests/Zmsstatistic/OverviewTest.php` around lines 70 - 78, The test renders the overview into $response via $this->render but never asserts the new capacity-report content; update the assertions after $response is created (using $response->getBody() cast to string) to include an assertion that the capacity section/link is present (e.g., assertStringContainsString with the expected capacity-report anchor or label such as the capacity report URL or title) so the OverviewTest (where $this->render and $response are used) verifies the new integration path.zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php (1)
173-185: 💤 Low valueUnreachable single-day branch.
When
$periodis aYYYY-MM-DDstring,resolveRangeDurationHours()already resolves bounds (viaresolveTimelineBounds) and returns a non-null value (~24h), so the firstif ($hours !== null)returns before thispreg_matchbranch is ever reached. The block at Lines 180-182 is dead code and can be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php` around lines 173 - 185, The branch in shouldFetchHourlyFromApi containing the preg_match for a YYYY-MM-DD period is unreachable because resolveRangeDurationHours (which calls resolveTimelineBounds) will already return a non-null hours value for single-day periods; remove the dead preg_match block and simplify the method to return hours <= self::MAX_HOURLY_FETCH_HOURS when resolveRangeDurationHours(...) returns non-null, otherwise return false, keeping references to shouldFetchHourlyFromApi, resolveRangeDurationHours, resolveTimelineBounds, and MAX_HOURLY_FETCH_HOURS to locate and update the code.zmsstatistic/src/Zmsstatistic/ReportCapacityIndex.php (1)
72-103: ⚡ Quick win
renderHtmlResponsetakes 8 positional parameters.This exceeds the recommended 4–5 argument limit and makes call sites positional and brittle. Consider passing a single associative array (or small view-model object) of template variables.
As per coding guidelines: "Functions/methods with more than 4-5 parameters ... Suggest refactoring to use objects or associative arrays for complex parameter sets."
♻️ Sketch
- private function renderHtmlResponse( - $response, - $args, - $capacityPeriod, - $dateRange, - $exchangeCapacity, - $exchangeCapacityChart, - $selectedScopes = [], - array $scopeDateBounds = [] - ): ResponseInterface { + private function renderHtmlResponse($response, array $viewData): ResponseInterface { return Render::withHtml( $response, 'page/reportCapacityIndex.twig', - [ - 'title' => 'Terminkapazität Standort', - ... - ] + $viewData ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/src/Zmsstatistic/ReportCapacityIndex.php` around lines 72 - 103, renderHtmlResponse currently accepts eight positional parameters which is brittle; refactor it to accept a single associative array or a small view-model object (e.g. $viewModel) containing keys: capacityPeriod, dateRange, exchangeCapacity, exchangeCapacityChart, selectedScopes, scopeDateBounds, args/period and any other template vars. Update the method signature (renderHtmlResponse) to unpack the view-model into the Render::withHtml call and remove the positional parameters, and then update all callers to pass a single associative array or the new DTO instead of eight separate arguments; ensure you preserve keys used in the template (e.g. 'period' from $args['period']) and keep $this->department/$this->organisation/$this->workstation access unchanged.zmsstatistic/js/block/warehousereport/index.js (1)
10-10: ⚡ Quick winRemove the constructor console log.
This ships debug logging on every report render. Please remove it or route it through the app’s normal logging path.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}must flagconsole.log(), console.debug(), console.warn()usage and remove or replace it with proper logging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/js/block/warehousereport/index.js` at line 10, Remove the debug console log in the Warehouse report component: delete the console.log('Component: Warehouse report', this, options) call in the constructor/initialization of the component in index.js (or replace it with the app's standard logger). Locate the call in the component's constructor/initialization block in zmsstatistic/js/block/warehousereport/index.js (the line logging "Component: Warehouse report") and either remove it entirely or route it through the project's logging utility (e.g., use the app logger method instead of console.log) to comply with the linting/guidelines for console usage.zmsstatistic/js/block/reportfilter/index.js (1)
15-15: ⚡ Quick winRemove the browser-console logging from this filter.
console.login the constructor andconsole.warnin the JSON fallback paths will ship noisy client-side logging even though these branches already recover cleanly. Please remove them or route them through the app’s normal logging/error path instead.As per coding guidelines,
**/*.{js,jsx,ts,tsx}must flagconsole.log(), console.debug(), console.warn()usage and remove or replace it with proper logging.Also applies to: 26-26, 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/js/block/reportfilter/index.js` at line 15, Remove the direct browser console calls in the ReportFilter code: replace the console.log('Component: ReportFilter', this, options) inside the ReportFilter constructor with the app's logger (e.g., this.logger.debug or imported logger.debug/info) or delete if redundant, and remove/replace console.warn in the JSON fallback branches with the app logger (logger.warn/error) or silent recovery; locate the occurrences by searching for the "Component: ReportFilter" log in the ReportFilter constructor and the JSON fallback code paths that emit console.warn and switch them to the centralized logging API used by the app (or remove them if no runtime logging is required).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zmsdb/src/Zmsdb/ExchangeSlotscope.php`:
- Around line 21-25: The failure is caused by binding :datestart/:dateend when
in unfiltered mode (where the chosen query is self::QUERY_READ_REPORT) which has
no such placeholders; update the logic in ExchangeSlotscope.php so that when
$unfiltered is true you do NOT set or bind $datestart/$dateend (or
alternatively, detect the selected query type like self::QUERY_READ_REPORT and
skip adding those PDO parameters), and ensure the code paths that build the PDO
parameter array only add datestart/dateend when the SQL actually contains
:datestart/:dateend (e.g., by checking $unfiltered or the query constant before
adding them).
In `@zmsstatistic/js/block/reportfilter/index.js`:
- Around line 159-175: The current loop in the report filter (using scopeIds and
this.scopeDateBounds) builds a union by taking the earliest start (min) and
latest end (max); change it to compute the intersection: collect each scope's
entry.min and entry.max, set min = the maximum of all entry.min values and max =
the minimum of all entry.max values (use moment comparisons consistent with
existing code), and if either bound is missing or the resulting min > max return
null; update the logic around variables named min, max and the scopeIds.forEach
block to implement this intersection behavior.
- Around line 235-258: The branch that currently forces out-of-range dates to
the lower bound should instead clamp to the nearest valid bound: replace the
direct resets to minToIso/minFromIso with calls to clampIso(value, min, max) for
the toInput and fromInput checks. Locate the checks using toInput.val() against
minToIso/maxAllowedIso and fromInput.val() against minFromIso/maxFromIso
(functions: setDateInputBound, getMinRangeStartIso) and set the input value to
clampIso(currentVal, minToIso, maxAllowedIso) or clampIso(currentVal,
minFromIso, maxFromIso) respectively so high-side overflows map to the maximum
rather than always the minimum.
In `@zmsstatistic/src/Zmsstatistic/Helper/TwigExceptionHandler.php`:
- Around line 30-34: The empty catch in TwigExceptionHandler that swallows
failures to fetch the workstation should log the exception instead of ignoring
it: in the catch block for ($workstationexception) capture and log the error via
the Monolog logger (e.g. \App::$logger or the handler's logger) with a clear
message like "Failed to fetch /workstation/ for extendedInfo" and include the
$workstationexception (or its message/stack) as context, but do not re-throw;
keep setting $extendedInfo['workstation'] unchanged on failure so rendering
continues.
In `@zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php`:
- Around line 84-93: The getCapacityPeriod() method can throw when
readGetResult() returns null and getEntity() is called; update getCapacityPeriod
to null-guard the result of
\App::$http->readGetResult('/warehouse/slotscope/'.$scopeId.'/') before calling
->getEntity(), and broaden the catch from Exception to \Throwable (matching
fetchAggregatedReport) so any PHP errors are caught; if the HTTP call or
getEntity is null/throws, return null and avoid passing null into
enrichPeriodList().
- Around line 42-77: The code calls
\App::$http->readGetResult('/warehouse/slotscope/')->getEntity() which will
throw an uncaught Error if readGetResult() returns null; fix by first assigning
the response to a variable (e.g. $response =
\App::$http->readGetResult('/warehouse/slotscope/')), guard that $response !==
null before calling ->getEntity(), then proceed to validate $subjectList and
$subjectList->data as before; also change the exception handler from catch
(Exception $exception) to catch (\Throwable $exception) so fatal errors are
caught.
In `@zmsstatistic/templates/block/report/reportFilter.twig`:
- Line 3: The template renders the data-picker-scope-ids attribute before the
local variable pickerScopeIds is computed, so the frontend rarely receives it;
move the logic that builds pickerScopeIds (and any related
defaultScopeId/scopeDateBounds handling) above the opening <div> and then render
data-picker-scope-ids="{{ pickerScopeIds|json_encode|e('html_attr') }}" (with
the existing conditional) so the attribute is populated correctly; ensure you
keep the existing conditions using allowFutureDates, scopeDateBounds, and
defaultScopeId but evaluate/assign pickerScopeIds beforehand so the attribute
reflects the computed IDs.
In `@zmsstatistic/tests/Zmsstatistic/ReportCapacityScopeTest.php`:
- Around line 166-177: The test Mock for the multi-scope case uses a
single-scope fixture and only asserts on a date, so it doesn't validate that
both scopes (141 and 142) are rendered; update the mocked response used in
ReportCapacityScopeTest (the readGetResult/readFixture call for URL
'/warehouse/slotscope/141,142/_/') to return a dedicated multi-scope fixture
that includes data for scope "142", then extend the assertion after render(...)
to check for a 142-specific piece of output (for example a row label or value
unique to scope 142) so the test fails if scope 142 is omitted by the
implementation.
---
Nitpick comments:
In `@zmsstatistic/js/block/reportfilter/index.js`:
- Line 15: Remove the direct browser console calls in the ReportFilter code:
replace the console.log('Component: ReportFilter', this, options) inside the
ReportFilter constructor with the app's logger (e.g., this.logger.debug or
imported logger.debug/info) or delete if redundant, and remove/replace
console.warn in the JSON fallback branches with the app logger
(logger.warn/error) or silent recovery; locate the occurrences by searching for
the "Component: ReportFilter" log in the ReportFilter constructor and the JSON
fallback code paths that emit console.warn and switch them to the centralized
logging API used by the app (or remove them if no runtime logging is required).
In `@zmsstatistic/js/block/warehousereport/index.js`:
- Line 10: Remove the debug console log in the Warehouse report component:
delete the console.log('Component: Warehouse report', this, options) call in the
constructor/initialization of the component in index.js (or replace it with the
app's standard logger). Locate the call in the component's
constructor/initialization block in
zmsstatistic/js/block/warehousereport/index.js (the line logging "Component:
Warehouse report") and either remove it entirely or route it through the
project's logging utility (e.g., use the app logger method instead of
console.log) to comply with the linting/guidelines for console usage.
In `@zmsstatistic/src/Zmsstatistic/ReportCapacityIndex.php`:
- Around line 72-103: renderHtmlResponse currently accepts eight positional
parameters which is brittle; refactor it to accept a single associative array or
a small view-model object (e.g. $viewModel) containing keys: capacityPeriod,
dateRange, exchangeCapacity, exchangeCapacityChart, selectedScopes,
scopeDateBounds, args/period and any other template vars. Update the method
signature (renderHtmlResponse) to unpack the view-model into the
Render::withHtml call and remove the positional parameters, and then update all
callers to pass a single associative array or the new DTO instead of eight
separate arguments; ensure you preserve keys used in the template (e.g. 'period'
from $args['period']) and keep
$this->department/$this->organisation/$this->workstation access unchanged.
In `@zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php`:
- Around line 173-185: The branch in shouldFetchHourlyFromApi containing the
preg_match for a YYYY-MM-DD period is unreachable because
resolveRangeDurationHours (which calls resolveTimelineBounds) will already
return a non-null hours value for single-day periods; remove the dead preg_match
block and simplify the method to return hours <= self::MAX_HOURLY_FETCH_HOURS
when resolveRangeDurationHours(...) returns non-null, otherwise return false,
keeping references to shouldFetchHourlyFromApi, resolveRangeDurationHours,
resolveTimelineBounds, and MAX_HOURLY_FETCH_HOURS to locate and update the code.
In `@zmsstatistic/tests/Zmsstatistic/OverviewTest.php`:
- Around line 70-78: The test renders the overview into $response via
$this->render but never asserts the new capacity-report content; update the
assertions after $response is created (using $response->getBody() cast to
string) to include an assertion that the capacity section/link is present (e.g.,
assertStringContainsString with the expected capacity-report anchor or label
such as the capacity report URL or title) so the OverviewTest (where
$this->render and $response are used) verifies the new integration path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d9bbe70-f8f4-4b96-82c8-20ae8cef89f7
⛔ Files ignored due to path filters (1)
zmsstatistic/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
zmsdb/src/Zmsdb/ExchangeSlotscope.phpzmsdb/src/Zmsdb/Query/ExchangeSlotscope.phpzmsstatistic/js/block/reportfilter/index.jszmsstatistic/js/block/warehousereport/index.jszmsstatistic/routing.phpzmsstatistic/src/Zmsstatistic/Helper/TwigExceptionHandler.phpzmsstatistic/src/Zmsstatistic/Overview.phpzmsstatistic/src/Zmsstatistic/ReportCapacityIndex.phpzmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.phpzmsstatistic/templates/block/navigation/navigation.twigzmsstatistic/templates/block/report/reportFilter.twigzmsstatistic/templates/page/overview.twigzmsstatistic/templates/page/reportCapacityIndex.twigzmsstatistic/tests/Zmsstatistic/OverviewTest.phpzmsstatistic/tests/Zmsstatistic/ReportCapacityScopeTest.phpzmsstatistic/tests/Zmsstatistic/ReportCapacityServiceTest.phpzmsstatistic/tests/Zmsstatistic/fixtures/GET_slotscope_141.jsonzmsstatistic/tests/Zmsstatistic/fixtures/GET_slotscope_141_hourly_report.jsonzmsstatistic/tests/Zmsstatistic/fixtures/GET_slotscope_141_report.jsonzmsstatistic/tests/Zmsstatistic/fixtures/GET_warehouse_slotscope.json
Comment on lines
+159
to
+175
| let min = null; | ||
| let max = null; | ||
|
|
||
| scopeIds.forEach((scopeId) => { | ||
| const entry = this.scopeDateBounds[String(scopeId)]; | ||
| if (!entry || !entry.min || !entry.max) { | ||
| return; | ||
| } | ||
| min = min === null ? entry.min : moment.min(moment(min), moment(entry.min)).format('YYYY-MM-DD'); | ||
| max = max === null ? entry.max : moment.max(moment(max), moment(entry.max)).format('YYYY-MM-DD'); | ||
| }); | ||
|
|
||
| if (min === null || max === null) { | ||
| return null; | ||
| } | ||
|
|
||
| return { min, max }; |
Contributor
There was a problem hiding this comment.
Intersect selected scope date windows instead of unioning them.
This currently builds min from the earliest selected-scope start and max from the latest selected-scope end. For multi-scope selections that widens the allowed range beyond at least one active scope, so the form can accept dates outside a selected scope’s API bounds. Use the common window instead: min = max(all mins) and max = min(all maxes).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsstatistic/js/block/reportfilter/index.js` around lines 159 - 175, The
current loop in the report filter (using scopeIds and this.scopeDateBounds)
builds a union by taking the earliest start (min) and latest end (max); change
it to compute the intersection: collect each scope's entry.min and entry.max,
set min = the maximum of all entry.min values and max = the minimum of all
entry.max values (use moment comparisons consistent with existing code), and if
either bound is missing or the resulting min > max return null; update the logic
around variables named min, max and the scopeIds.forEach block to implement this
intersection behavior.
Comment on lines
+235
to
+258
| if (toInput.val() && (toInput.val() < minToIso || toInput.val() > maxAllowedIso)) { | ||
| toInput.val(minToIso); | ||
| } | ||
| } else { | ||
| toInput.attr('min', ''); | ||
| toInput.attr('max', todayIso); | ||
| this.setDateInputBound(toInput, 'min', globalMin); | ||
| this.setDateInputBound(toInput, 'max', globalMax); | ||
| } | ||
|
|
||
| if (toInput.val()) { | ||
| const toDate = new Date(toInput.val()); | ||
| const minFromDate = new Date(toDate); | ||
| minFromDate.setDate(minFromDate.getDate() - 366); | ||
|
|
||
| const minAllowedDate = new Date(Math.max(minFromDate.getTime(), 0)); | ||
| const minAllowedIso = minAllowedDate.toISOString().slice(0, 10); | ||
|
|
||
| fromInput.attr('min', minAllowedIso); | ||
| fromInput.attr('max', toInput.val()); | ||
|
|
||
| if (fromInput.val() && (fromInput.val() < minAllowedIso || fromInput.val() > toInput.val())) { | ||
| fromInput.val(toInput.val()); | ||
| let minFromIso = this.getMinRangeStartIso(toInput.val()); | ||
| if (globalMin) { | ||
| minFromIso = moment.max(moment(minFromIso), moment(globalMin)).format('YYYY-MM-DD'); | ||
| } | ||
|
|
||
| let maxFromIso = toInput.val(); | ||
| if (globalMax) { | ||
| maxFromIso = moment.min(moment(maxFromIso), moment(globalMax)).format('YYYY-MM-DD'); | ||
| } | ||
|
|
||
| this.setDateInputBound(fromInput, 'min', minFromIso); | ||
| this.setDateInputBound(fromInput, 'max', maxFromIso); | ||
|
|
||
| if (fromInput.val() && (fromInput.val() < minFromIso || fromInput.val() > maxFromIso)) { | ||
| fromInput.val(minFromIso); |
Contributor
There was a problem hiding this comment.
Clamp edited dates to the nearest valid bound.
Both overflow branches reset the field to the lower bound (minToIso / minFromIso). If the current value is only above the max, that collapses the range to the earliest allowed date instead of the closest valid one. Reuse clampIso() with the field-specific min/max here so high-side overflows land on maxAllowedIso / maxFromIso.
Suggested fix
- if (toInput.val() && (toInput.val() < minToIso || toInput.val() > maxAllowedIso)) {
- toInput.val(minToIso);
+ if (toInput.val()) {
+ toInput.val(this.clampIso(toInput.val(), minToIso, maxAllowedIso));
}
…
- if (fromInput.val() && (fromInput.val() < minFromIso || fromInput.val() > maxFromIso)) {
- fromInput.val(minFromIso);
+ if (fromInput.val()) {
+ fromInput.val(this.clampIso(fromInput.val(), minFromIso, maxFromIso));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsstatistic/js/block/reportfilter/index.js` around lines 235 - 258, The
branch that currently forces out-of-range dates to the lower bound should
instead clamp to the nearest valid bound: replace the direct resets to
minToIso/minFromIso with calls to clampIso(value, min, max) for the toInput and
fromInput checks. Locate the checks using toInput.val() against
minToIso/maxAllowedIso and fromInput.val() against minFromIso/maxFromIso
(functions: setDateInputBound, getMinRangeStartIso) and set the input value to
clampIso(currentVal, minToIso, maxAllowedIso) or clampIso(currentVal,
minFromIso, maxFromIso) respectively so high-side overflows map to the maximum
rather than always the minimum.
Comment on lines
30
to
34
| try { | ||
| $exception->templatedata = [ | ||
| 'workstation' => \App::$http->readGetResult('/workstation/')->getEntity(), | ||
| ]; | ||
| $extendedInfo['workstation'] = \App::$http->readGetResult('/workstation/')->getEntity(); | ||
| } catch (\Exception $workstationexception) { | ||
| // ignore | ||
| } |
Contributor
There was a problem hiding this comment.
Silent catch swallows workstation fetch failures.
The empty catch with // ignore discards the exception without any logging, making failures to fetch /workstation/ invisible during diagnostics. Within exception-rendering it is reasonable not to re-throw, but the failure should still be observable via the Monolog logger rather than silently dropped.
As per coding guidelines: "Empty catch blocks", "Silent failures", and "Missing error logging" should be avoided; debug output should use the Monolog logging framework.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsstatistic/src/Zmsstatistic/Helper/TwigExceptionHandler.php` around lines
30 - 34, The empty catch in TwigExceptionHandler that swallows failures to fetch
the workstation should log the exception instead of ignoring it: in the catch
block for ($workstationexception) capture and log the error via the Monolog
logger (e.g. \App::$logger or the handler's logger) with a clear message like
"Failed to fetch /workstation/ for extendedInfo" and include the
$workstationexception (or its message/stack) as context, but do not re-throw;
keep setting $extendedInfo['workstation'] unchanged on failure so rendering
continues.
Comment on lines
+42
to
+77
| try { | ||
| $subjectList = \App::$http | ||
| ->readGetResult('/warehouse/slotscope/') | ||
| ->getEntity(); | ||
|
|
||
| if (!$subjectList instanceof Exchange || empty($subjectList->data)) { | ||
| return []; | ||
| } | ||
|
|
||
| $bounds = []; | ||
| foreach ($subjectList->data as $row) { | ||
| $scopeId = (string) ($row[0] ?? ''); | ||
| $periodStart = (string) ($row[1] ?? ''); | ||
| $periodEnd = (string) ($row[2] ?? ''); | ||
|
|
||
| if ($scopeId === '' || $periodStart === '' || $periodEnd === '') { | ||
| continue; | ||
| } | ||
|
|
||
| if (!isset($bounds[$scopeId])) { | ||
| $bounds[$scopeId] = [ | ||
| 'min' => $periodStart, | ||
| 'max' => $periodEnd, | ||
| ]; | ||
| continue; | ||
| } | ||
|
|
||
| $bounds[$scopeId]['min'] = min($bounds[$scopeId]['min'], $periodStart); | ||
| $bounds[$scopeId]['max'] = max($bounds[$scopeId]['max'], $periodEnd); | ||
| } | ||
|
|
||
| return $bounds; | ||
| } catch (Exception $exception) { | ||
| return []; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Same \Error-on-null hazard as fetchAggregatedReport.
readGetResult('/warehouse/slotscope/')->getEntity() will fatally error (uncaught \Error) when the call returns null. Apply the null guard + catch (\Throwable) fix described in the fetchAggregatedReport comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php` around lines
42 - 77, The code calls
\App::$http->readGetResult('/warehouse/slotscope/')->getEntity() which will
throw an uncaught Error if readGetResult() returns null; fix by first assigning
the response to a variable (e.g. $response =
\App::$http->readGetResult('/warehouse/slotscope/')), guard that $response !==
null before calling ->getEntity(), then proceed to validate $subjectList and
$subjectList->data as before; also change the exception handler from catch
(Exception $exception) to catch (\Throwable $exception) so fatal errors are
caught.
| {% from 'element/helper/form.twig' import formgroup, providername %} | ||
|
|
||
| <div data-report-filter> | ||
| <div data-report-filter{% if allowFutureDates|default(false) %} data-allow-future-dates="1"{% endif %}{% if scopeDateBounds is defined and scopeDateBounds %} data-scope-date-bounds="{{ scopeDateBounds|json_encode|e('html_attr') }}"{% endif %}{% if defaultScopeId is defined and defaultScopeId %} data-default-scope-id="{{ defaultScopeId }}"{% endif %}{% if pickerScopeIds is defined and pickerScopeIds %} data-picker-scope-ids="{{ pickerScopeIds|json_encode|e('html_attr') }}"{% endif %}> |
Contributor
There was a problem hiding this comment.
data-picker-scope-ids is never populated from the IDs computed in this template.
On Line 3, the attribute is rendered before local pickerScopeIds is built (Line 6, Line 12, Line 19), so the frontend usually receives no data-picker-scope-ids and falls back to defaultScopeId. That breaks the intended “use all picker scopes when none selected” date-bound behavior.
Proposed fix
-{% from 'element/helper/form.twig' import formgroup, providername %}
-
-<div data-report-filter{% if allowFutureDates|default(false) %} data-allow-future-dates="1"{% endif %}{% if scopeDateBounds is defined and scopeDateBounds %} data-scope-date-bounds="{{ scopeDateBounds|json_encode|e('html_attr') }}"{% endif %}{% if defaultScopeId is defined and defaultScopeId %} data-default-scope-id="{{ defaultScopeId }}"{% endif %}{% if pickerScopeIds is defined and pickerScopeIds %} data-picker-scope-ids="{{ pickerScopeIds|json_encode|e('html_attr') }}"{% endif %}>
+{% from 'element/helper/form.twig' import formgroup, providername %}
+
<!-- Scope Selection -->
{% set scopeOptions = [] %}
{% set pickerScopeIds = [] %}
{% for department in workstation.useraccount.departments %}
{% set scopeOptionsGroupOptions = [] %}
@@
{% set scopeOptions = scopeOptions|merge([{ 'name' : department.name, 'options' : scopeOptionsGroupOptions|msort('name') }]) %}
{% endfor %}
+
+<div data-report-filter{% if allowFutureDates|default(false) %} data-allow-future-dates="1"{% endif %}{% if scopeDateBounds is defined and scopeDateBounds %} data-scope-date-bounds="{{ scopeDateBounds|json_encode|e('html_attr') }}"{% endif %}{% if defaultScopeId is defined and defaultScopeId %} data-default-scope-id="{{ defaultScopeId }}"{% endif %}{% if pickerScopeIds %} data-picker-scope-ids="{{ pickerScopeIds|json_encode|e('html_attr') }}"{% endif %}>Also applies to: 6-20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsstatistic/templates/block/report/reportFilter.twig` at line 3, The
template renders the data-picker-scope-ids attribute before the local variable
pickerScopeIds is computed, so the frontend rarely receives it; move the logic
that builds pickerScopeIds (and any related defaultScopeId/scopeDateBounds
handling) above the opening <div> and then render data-picker-scope-ids="{{
pickerScopeIds|json_encode|e('html_attr') }}" (with the existing conditional) so
the attribute is populated correctly; ensure you keep the existing conditions
using allowFutureDates, scopeDateBounds, and defaultScopeId but evaluate/assign
pickerScopeIds beforehand so the attribute reflects the computed IDs.
Comment on lines
+166
to
+177
| 'function' => 'readGetResult', | ||
| 'url' => '/warehouse/slotscope/141,142/_/', | ||
| 'response' => $this->readFixture("GET_slotscope_141_report.json") | ||
| ], | ||
| ] | ||
| ); | ||
| $response = $this->render( | ||
| ['period' => '2016-04'], | ||
| [], | ||
| ['scopes' => ['141', '142']] | ||
| ); | ||
| $this->assertStringContainsString('01.04.2016', (string) $response->getBody()); |
Contributor
There was a problem hiding this comment.
Multi-scope test does not verify multi-scope behavior.
Line 167 mocks /warehouse/slotscope/141,142/_/ with a single-scope fixture, and Line 177 asserts only a date. This means the test still passes even if scope 142 is dropped by implementation.
Use a dedicated multi-scope fixture (including 142) and assert a 142-specific row/value in the rendered output.
Suggested minimal test hardening
- 'response' => $this->readFixture("GET_slotscope_141_report.json")
+ 'response' => $this->readFixture("GET_slotscope_141_142_report.json")
...
- $this->assertStringContainsString('01.04.2016', (string) $response->getBody());
+ $body = (string) $response->getBody();
+ $this->assertStringContainsString('01.04.2016', $body);
+ $this->assertStringContainsString('["142"', $body);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsstatistic/tests/Zmsstatistic/ReportCapacityScopeTest.php` around lines 166
- 177, The test Mock for the multi-scope case uses a single-scope fixture and
only asserts on a date, so it doesn't validate that both scopes (141 and 142)
are rendered; update the mocked response used in ReportCapacityScopeTest (the
readGetResult/readFixture call for URL '/warehouse/slotscope/141,142/_/') to
return a dedicated multi-scope fixture that includes data for scope "142", then
extend the assertion after render(...) to check for a 142-specific piece of
output (for example a row label or value unique to scope 142) so the test fails
if scope 142 is omitted by the implementation.
Unfiltered report reads only bind scopeid so PDO matches QUERY_READ_REPORT. Relax subject-list count assertion after including all slot rows in bounds.
ThomasAFink
force-pushed
the
feature-zmskvr-1378-planned-booked-capacity-statistics-mvp
branch
from
46b738c to
9d02bf6
Compare
Guard null API responses, bind empty query params as null, and filter report rows to the selected period. Fix prophecy call order in tests and load workstation before parent error rendering (no zmsslim changes).
…rder QUERY_SUBJECTS orders by description; assert scope 141 date bounds instead of assuming a fixed first row.
ThomasAFink
force-pushed
the
feature-zmskvr-1378-planned-booked-capacity-statistics-mvp
branch
from
0389f31 to
2f2e126
Compare
Restore ExchangeSlotscope and its tests to main behaviour for the superuser slotscope export. Move filtered/hourly capacity queries to ExchangeCapacityscope and wire Terminkapazität to /warehouse/capacityscope/.
…nHandler Match main for slotscope export and error handler; capacity stays on ExchangeCapacityscope and /warehouse/capacityscope/ only.
Expect 17 warehouse subjects after adding capacityscope and assert scope 141 date bounds structurally instead of hard-coded fixture dates.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
zmsdb/src/Zmsdb/Query/ExchangeCapacityscope.php (2)
79-79: 💤 Low valueUse fully qualified class reference for clarity.
The reference
Scope::TABLErelies on namespace resolution. For consistency with other constant references in this file (e.g., lines 66, 70, 73 use fully qualified names), use\BO\Zmsdb\Query\Scope::TABLE.📝 Proposed fix
- FROM ' . Scope::TABLE . ' AS scope + FROM ' . \BO\Zmsdb\Query\Scope::TABLE . ' AS scope🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsdb/src/Zmsdb/Query/ExchangeCapacityscope.php` at line 79, Replace the unqualified constant reference Scope::TABLE with the fully qualified class constant \BO\Zmsdb\Query\Scope::TABLE in the SQL string; locate the reference in the ExchangeCapacityscope class (the query-building method that constructs the "FROM ' . Scope::TABLE . ' AS scope" fragment) and update it to use the fully qualified symbol to match other constant usages in the file.
12-27: ⚡ Quick winAdd column aliases to aggregate expressions for clarity.
The
SUM(slotcount)andSUM(intern)expressions at lines 16-17 lack aliases, making the result columns accessible only by numeric index. According to Clean Code principles (Names rules: "Choose descriptive and unambiguous names"), these should have meaningful aliases matching the dictionary entries used downstream.📝 Proposed fix
SELECT `scopeID` as subjectid, CONCAT(year, "-", LPAD(month, 2, 0), "-", LPAD(day, 2, 0)) as date, - SUM(slotcount), - SUM(intern) + SUM(slotcount) as bookedcount, + SUM(intern) as plannedcount FROM (Apply the same change to
QUERY_READ_REPORT_FILTERED(lines 33-34) andQUERY_READ_REPORT_HOURLY(lines 55-56).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsdb/src/Zmsdb/Query/ExchangeCapacityscope.php` around lines 12 - 27, The aggregate columns in the SQL constants lack aliases; update QUERY_READ_REPORT to add descriptive aliases (e.g. SUM(slotcount) AS total_slots, SUM(intern) AS total_intern) so callers can access them by name, and make the same change in QUERY_READ_REPORT_FILTERED and QUERY_READ_REPORT_HOURLY to use the same alias names as used by downstream dictionary entries.zmsdb/src/Zmsdb/ExchangeCapacityscope.php (3)
23-24: ⚡ Quick winReplace magic date values with named constants.
The dates '1970-01-01' and '2099-12-31' are magic values. Extract them as class constants with descriptive names to improve code clarity.
🔧 Proposed fix
+ private const DEFAULT_MIN_DATE = '1970-01-01'; + private const DEFAULT_MAX_DATE = '2099-12-31'; + public function readEntity( ... ) { ... if ($unfiltered) { - $datestart = new \DateTimeImmutable('1970-01-01'); - $dateend = new \DateTimeImmutable('2099-12-31'); + $datestart = new \DateTimeImmutable(self::DEFAULT_MIN_DATE); + $dateend = new \DateTimeImmutable(self::DEFAULT_MAX_DATE);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` around lines 23 - 24, Extract the hard-coded dates into descriptive class constants in ExchangeCapacityscope: replace the literals '1970-01-01' and '2099-12-31' used when constructing $datestart and $dateend with constants like DEFAULT_START_DATE and DEFAULT_END_DATE defined at the top of the class; update the constructors/build logic that uses new \DateTimeImmutable(...) to refer to those constants (e.g., new \DateTimeImmutable(self::DEFAULT_START_DATE)) so the magic values are centralized and self-documenting.
19-19: ⚖️ Poor tradeoffExtract hardcoded German text for internationalization.
The string "Terminkapazität " is hardcoded in German. Consider using a translation/localization mechanism for multi-language support and consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` at line 19, The title assignment currently hardcodes German text in $entity['title'] ("Terminkapazität " . $scope->contact->name . " " . $scope->shortName); replace that literal with a call to your localization/translation helper (e.g. translator->trans(), __(), or gettext) so the prefix is translatable and the contact name and shortName are injected as parameters; update the assignment in ExchangeCapacityscope.php to use the translation function for the label while still concatenating or formatting $scope->contact->name and $scope->shortName.
17-17: ⚖️ Poor tradeoffAvoid direct instantiation; use dependency injection.
Creating
new Scope()directly violates dependency injection principles and makes the code harder to test. While per learnings the transaction is managed by middleware, the instantiation pattern should still follow DI for better testability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` at line 17, The code directly instantiates Scope with "new Scope()" when calling readEntity($firstScopeId); replace this with dependency injection: add a Scope dependency (e.g., a private $scope property) injected via the class constructor or setter and call $this->scope->readEntity($firstScopeId) instead; update the ExchangeCapacityscope class constructor signature to accept a Scope instance and assign it to the property, and adjust any instantiation sites/tests to provide a mock or real Scope dependency.zmsdb/tests/Zmsdb/ExchangeCapacityscopeTest.php (1)
14-14: ⚡ Quick winDocument or remove magic number assertion.
The assertion
assertGreaterThanOrEqual(44, count($entity->data))uses a magic number 44 without explanation. If this is a known minimum scope count in the test database, document it with a comment. Otherwise, consider whether this assertion adds value or could be replaced with a more meaningful check (e.g., just asserting the presence of scope 141).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsdb/tests/Zmsdb/ExchangeCapacityscopeTest.php` at line 14, The test uses a magic number in the assertion assertGreaterThanOrEqual(44, count($entity->data)) — either document why 44 is expected by adding a brief comment referencing the test fixture/minimum scope count, or replace the brittle numeric check with a clearer assertion: either use a named constant (e.g., MIN_EXPECTED_SCOPES) or assert the presence of a specific expected scope (e.g., assert that scope id 141 exists in $entity->data) inside the ExchangeCapacityscopeTest so the intent is explicit and the test is less fragile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php`:
- Around line 92-103: The readPeriodList method in ExchangeCapacityscope
currently ignores its parameters ($subjectid, $period), returns a hardcoded
placeholder dataset, and has an `@SuppressWarnings`(Unused) annotation; either
implement real logic using those parameters in
ExchangeCapacityscope::readPeriodList (e.g., build period-based dataset and
proper title/entries), or if the signature is required by an interface but not
applicable, remove the annotation and add a clear TODO with an issue/PR
reference and a comment explaining why parameters are unused, or alternatively
remove unused parameters if safe; ensure you update or remove the placeholder
addDataSet(["_"]) and adjust addDictionaryEntry/addDataSet calls accordingly and
include a brief justification in code when keeping the no-op behavior.
- Around line 47-58: The loop over subjectIdList that calls
$this->fetchAll(constant($queryConstant), $parameters) needs error handling:
wrap the fetchAll call inside a try-catch, catch \Exception, log the failure
with Monolog (include scopeId/$scopeId and the exception message) and then
rethrow the exception so callers can handle it; apply this change in the same
block that iterates subjectIdList and before calling $entity->addDataSet to
ensure failed queries are logged and do not silently continue.
- Around line 78-87: Wrap the call to
$this->getReader()->fetchAll(Query\ExchangeCapacityscope::QUERY_SUBJECTS, []) in
the same Monolog-style error handling used by readEntity(): try the fetch, catch
Throwable (or Exception), call $this->getLogger()->error with a descriptive
message and the exception details, and abort the method (return null or false)
when the query fails; also handle the case where fetchAll returns a falsy value
by logging and returning early before building the Exchange entity (refer to
getReader()->fetchAll, readEntity(), and the Exchange entity creation).
- Around line 9-14: Update the readEntity method signature to add proper type
hints: change the $subjectid parameter to string, ensure $datestart and $dateend
keep their \DateTimeInterface types, change $period to string, and add the
return type : Exchange (the Exchange class used by this file). Locate the
readEntity method in ExchangeCapacityscope.php and modify its signature to
reflect these types so IDEs and static analysis can validate calls and the
method is explicit about returning an Exchange.
- Line 76: The method readSubjectList lacks a return type; update its
declaration in the ExchangeCapacityscope class (readSubjectList) to declare a
return type of : Exchange, ensure the Exchange class/interface is imported or
fully-qualified, and verify that the method body always returns an instance of
Exchange (adjust return values or add type casts/guards as needed) to satisfy
the new signature.
- Line 95: The method readPeriodList currently lacks a return type; update its
signature to declare a return type of Exchange (i.e., change "public function
readPeriodList($subjectid, $period = 'day')" to "public function
readPeriodList($subjectid, $period = 'day'): Exchange") and ensure the Exchange
class is properly referenced (imported with a use statement or fully-qualified)
so the type is valid.
- Around line 15-17: The code assumes $subjectid contains data; validate it
before using explode/readEntity: check if trim($subjectid) === '' (or empty
after trimming) and handle that case (e.g., return early, set $scope = null, or
throw a clear exception) instead of calling (new Scope())->readEntity with an
empty ID; ensure the subsequent lines that use $subjectIdList and $firstScopeId
only run when a non-empty first element exists, and update the code paths that
rely on $scope accordingly.
---
Nitpick comments:
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php`:
- Around line 23-24: Extract the hard-coded dates into descriptive class
constants in ExchangeCapacityscope: replace the literals '1970-01-01' and
'2099-12-31' used when constructing $datestart and $dateend with constants like
DEFAULT_START_DATE and DEFAULT_END_DATE defined at the top of the class; update
the constructors/build logic that uses new \DateTimeImmutable(...) to refer to
those constants (e.g., new \DateTimeImmutable(self::DEFAULT_START_DATE)) so the
magic values are centralized and self-documenting.
- Line 19: The title assignment currently hardcodes German text in
$entity['title'] ("Terminkapazität " . $scope->contact->name . " " .
$scope->shortName); replace that literal with a call to your
localization/translation helper (e.g. translator->trans(), __(), or gettext) so
the prefix is translatable and the contact name and shortName are injected as
parameters; update the assignment in ExchangeCapacityscope.php to use the
translation function for the label while still concatenating or formatting
$scope->contact->name and $scope->shortName.
- Line 17: The code directly instantiates Scope with "new Scope()" when calling
readEntity($firstScopeId); replace this with dependency injection: add a Scope
dependency (e.g., a private $scope property) injected via the class constructor
or setter and call $this->scope->readEntity($firstScopeId) instead; update the
ExchangeCapacityscope class constructor signature to accept a Scope instance and
assign it to the property, and adjust any instantiation sites/tests to provide a
mock or real Scope dependency.
In `@zmsdb/src/Zmsdb/Query/ExchangeCapacityscope.php`:
- Line 79: Replace the unqualified constant reference Scope::TABLE with the
fully qualified class constant \BO\Zmsdb\Query\Scope::TABLE in the SQL string;
locate the reference in the ExchangeCapacityscope class (the query-building
method that constructs the "FROM ' . Scope::TABLE . ' AS scope" fragment) and
update it to use the fully qualified symbol to match other constant usages in
the file.
- Around line 12-27: The aggregate columns in the SQL constants lack aliases;
update QUERY_READ_REPORT to add descriptive aliases (e.g. SUM(slotcount) AS
total_slots, SUM(intern) AS total_intern) so callers can access them by name,
and make the same change in QUERY_READ_REPORT_FILTERED and
QUERY_READ_REPORT_HOURLY to use the same alias names as used by downstream
dictionary entries.
In `@zmsdb/tests/Zmsdb/ExchangeCapacityscopeTest.php`:
- Line 14: The test uses a magic number in the assertion
assertGreaterThanOrEqual(44, count($entity->data)) — either document why 44 is
expected by adding a brief comment referencing the test fixture/minimum scope
count, or replace the brittle numeric check with a clearer assertion: either use
a named constant (e.g., MIN_EXPECTED_SCOPES) or assert the presence of a
specific expected scope (e.g., assert that scope id 141 exists in $entity->data)
inside the ExchangeCapacityscopeTest so the intent is explicit and the test is
less fragile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ae7b8e2-b2f9-48ed-8eec-aa0c390d4521
📒 Files selected for processing (7)
zmsdb/src/Zmsdb/ExchangeCapacityscope.phpzmsdb/src/Zmsdb/Query/ExchangeCapacityscope.phpzmsdb/src/Zmsdb/Warehouse.phpzmsdb/tests/Zmsdb/ExchangeCapacityscopeTest.phpzmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.phpzmsstatistic/tests/Zmsstatistic/OverviewTest.phpzmsstatistic/tests/Zmsstatistic/ReportCapacityScopeTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsstatistic/tests/Zmsstatistic/ReportCapacityScopeTest.php
- zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php
Comment on lines
+9
to
+14
| public function readEntity( | ||
| $subjectid, | ||
| \DateTimeInterface $datestart = null, | ||
| \DateTimeInterface $dateend = null, | ||
| $period = 'day' | ||
| ) { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints for parameters and return type.
The method signature lacks type hints for $subjectid (should be string) and $period (should be string), and is missing a return type hint (should be : Exchange). Type hints improve type safety and IDE support.
📝 Proposed fix
- public function readEntity(
- $subjectid,
+ public function readEntity(
+ string $subjectid,
\DateTimeInterface $datestart = null,
\DateTimeInterface $dateend = null,
- $period = 'day'
- ) {
+ string $period = 'day'
+ ): Exchange {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` around lines 9 - 14, Update the
readEntity method signature to add proper type hints: change the $subjectid
parameter to string, ensure $datestart and $dateend keep their
\DateTimeInterface types, change $period to string, and add the return type :
Exchange (the Exchange class used by this file). Locate the readEntity method in
ExchangeCapacityscope.php and modify its signature to reflect these types so
IDEs and static analysis can validate calls and the method is explicit about
returning an Exchange.
Comment on lines
+15
to
+17
| $subjectIdList = explode(',', $subjectid); | ||
| $firstScopeId = $subjectIdList[0]; | ||
| $scope = (new Scope())->readEntity($firstScopeId); |
Contributor
There was a problem hiding this comment.
Add validation for empty subjectid.
If $subjectid is an empty string, explode() will produce an array with one empty element, and line 17 will attempt to read a scope with an empty ID. Add validation to prevent this runtime error.
🛡️ Proposed fix
public function readEntity(
...
) {
+ if (empty($subjectid)) {
+ throw new \InvalidArgumentException('Subject ID cannot be empty');
+ }
$subjectIdList = explode(',', $subjectid);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` around lines 15 - 17, The code
assumes $subjectid contains data; validate it before using explode/readEntity:
check if trim($subjectid) === '' (or empty after trimming) and handle that case
(e.g., return early, set $scope = null, or throw a clear exception) instead of
calling (new Scope())->readEntity with an empty ID; ensure the subsequent lines
that use $subjectIdList and $firstScopeId only run when a non-empty first
element exists, and update the code paths that rely on $scope accordingly.
Comment on lines
+47
to
+58
| foreach ($subjectIdList as $scopeId) { | ||
| $parameters = ['scopeid' => $scopeId]; | ||
| if (!$unfiltered) { | ||
| $parameters['datestart'] = $datestart->format('Y-m-d'); | ||
| $parameters['dateend'] = $dateend->format('Y-m-d'); | ||
| } | ||
|
|
||
| $raw = $this->fetchAll(constant($queryConstant), $parameters); | ||
| foreach ($raw as $entry) { | ||
| $entity->addDataSet(array_values($entry)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for database query failures.
The fetchAll() call on line 54 can fail if the query is invalid or the database connection fails. No error handling or logging is present. Consider wrapping in try-catch with proper Monolog logging per coding guidelines.
As per coding guidelines, use Monolog for error logging instead of silent failures:
try {
$raw = $this->fetchAll(constant($queryConstant), $parameters);
} catch (\Exception $e) {
$log->error('Capacity query failed', [
'scopeId' => $scopeId,
'error' => $e->getMessage()
]);
throw;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` around lines 47 - 58, The loop
over subjectIdList that calls $this->fetchAll(constant($queryConstant),
$parameters) needs error handling: wrap the fetchAll call inside a try-catch,
catch \Exception, log the failure with Monolog (include scopeId/$scopeId and the
exception message) and then rethrow the exception so callers can handle it;
apply this change in the same block that iterates subjectIdList and before
calling $entity->addDataSet to ensure failed queries are logged and do not
silently continue.
| return '\BO\Zmsdb\Query\ExchangeCapacityscope::QUERY_READ_REPORT_FILTERED'; | ||
| } | ||
|
|
||
| public function readSubjectList() |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add return type hint.
The method should declare : Exchange as its return type for type safety and documentation.
📝 Proposed fix
- public function readSubjectList()
+ public function readSubjectList(): Exchange🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` at line 76, The method
readSubjectList lacks a return type; update its declaration in the
ExchangeCapacityscope class (readSubjectList) to declare a return type of :
Exchange, ensure the Exchange class/interface is imported or fully-qualified,
and verify that the method body always returns an instance of Exchange (adjust
return values or add type casts/guards as needed) to satisfy the new signature.
Comment on lines
+78
to
+87
| $raw = $this->getReader()->fetchAll(Query\ExchangeCapacityscope::QUERY_SUBJECTS, []); | ||
| $entity = new Exchange(); | ||
| $entity['title'] = "Terminkapazität "; | ||
| $entity->addDictionaryEntry('subject', 'string', 'Standort ID', 'scope.id'); | ||
| $entity->addDictionaryEntry('periodstart', 'string', 'Datum von'); | ||
| $entity->addDictionaryEntry('periodend', 'string', 'Datum bis'); | ||
| $entity->addDictionaryEntry('description', 'string', 'Beschreibung des Standortes'); | ||
| foreach ($raw as $entry) { | ||
| $entity->addDataSet(array_values($entry)); | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for database query.
The fetchAll() call can fail without any error handling or logging, similar to the issue in readEntity(). Apply the same Monolog-based error handling pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` around lines 78 - 87, Wrap the
call to
$this->getReader()->fetchAll(Query\ExchangeCapacityscope::QUERY_SUBJECTS, []) in
the same Monolog-style error handling used by readEntity(): try the fetch, catch
Throwable (or Exception), call $this->getLogger()->error with a descriptive
message and the exception details, and abort the method (return null or false)
when the query fails; also handle the case where fetchAll returns a falsy value
by logging and returning early before building the Exchange entity (refer to
getReader()->fetchAll, readEntity(), and the Exchange entity creation).
Comment on lines
+92
to
+103
| /** | ||
| * @SuppressWarnings(Unused) | ||
| */ | ||
| public function readPeriodList($subjectid, $period = 'day') | ||
| { | ||
| $entity = new Exchange(); | ||
| $entity['title'] = "Terminkapazität "; | ||
| $entity->addDictionaryEntry('id', 'string', 'Gesamter Zeitraum', 'useraccount.rights.superuser'); | ||
| $entity->addDataSet(["_"]); | ||
|
|
||
| return $entity; | ||
| } |
Contributor
There was a problem hiding this comment.
Address @SuppressWarnings and potentially incomplete implementation.
The @SuppressWarnings(Unused) annotation and the fact that both parameters are ignored while returning a hardcoded placeholder ["_"] suggest this method may be incomplete. Per coding guidelines, @SuppressWarnings should be avoided and the underlying issue addressed.
Consider:
- If the parameters are truly unused, remove them or document why they're required by an interface
- If this is placeholder code awaiting implementation, add a TODO with an issue reference
- If complete, remove the
@SuppressWarningsand justify the design
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` around lines 92 - 103, The
readPeriodList method in ExchangeCapacityscope currently ignores its parameters
($subjectid, $period), returns a hardcoded placeholder dataset, and has an
`@SuppressWarnings`(Unused) annotation; either implement real logic using those
parameters in ExchangeCapacityscope::readPeriodList (e.g., build period-based
dataset and proper title/entries), or if the signature is required by an
interface but not applicable, remove the annotation and add a clear TODO with an
issue/PR reference and a comment explaining why parameters are unused, or
alternatively remove unused parameters if safe; ensure you update or remove the
placeholder addDataSet(["_"]) and adjust addDictionaryEntry/addDataSet calls
accordingly and include a brief justification in code when keeping the no-op
behavior.
| /** | ||
| * @SuppressWarnings(Unused) | ||
| */ | ||
| public function readPeriodList($subjectid, $period = 'day') |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add return type hint.
The method should declare : Exchange as its return type for consistency and type safety.
📝 Proposed fix
- public function readPeriodList($subjectid, $period = 'day')
+ public function readPeriodList($subjectid, $period = 'day'): Exchange🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zmsdb/src/Zmsdb/ExchangeCapacityscope.php` at line 95, The method
readPeriodList currently lacks a return type; update its signature to declare a
return type of Exchange (i.e., change "public function
readPeriodList($subjectid, $period = 'day')" to "public function
readPeriodList($subjectid, $period = 'day'): Exchange") and ensure the Exchange
class is properly referenced (imported with a use statement or fully-qualified)
so the type is valid.
Allow Auswertungen menu items and report pages to load before a default Standort is selected; users pick scopes via the filter instead.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
zmsstatistic/templates/page/reportCapacityIndex.twig (2)
62-63: ⚖️ Poor tradeoffPositional row indexes (
row[2],row[3]) are fragile.The planned/booked columns are read by fixed numeric position, so any change to the warehouse column order silently corrupts the report. Consider exposing named keys from the service/entity (e.g.
row.planned/row.booked) to remove the magic indexes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/templates/page/reportCapacityIndex.twig` around lines 62 - 63, The template uses fragile positional indexes row[2] and row[3] for booked/planned; update the data provider (service/entity that builds the rows) to return associative arrays or objects with named keys (e.g. planned and booked) and then change the template access from row[2]/row[3] to row.planned and row.booked (or row['planned']/row['booked']) in the reportCapacityIndex.twig so the booked/planned columns no longer depend on column order.
18-19: 💤 Low valueGuard
workstation.scope.idconsistently on Line 19.Line 18 protects against an undefined
scope.idvia|default(null), but Line 19 dereferencesworkstation.scope.iddirectly. Ifworkstation.scopeis unset andstrict_variablesis enabled, this branch can raise an error. Mirror the Line 18 guard for consistency.♻️ Proposed consistency fix
- selectedScopeIds: selectedScopeIds is not empty ? selectedScopeIds : (workstation.scope.id ? [workstation.scope.id] : []), + selectedScopeIds: selectedScopeIds is not empty ? selectedScopeIds : (workstation.scope.id|default(null) ? [workstation.scope.id] : []),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/templates/page/reportCapacityIndex.twig` around lines 18 - 19, The selectedScopeIds expression dereferences workstation.scope.id directly (in the ternary on the second line) which can error under strict_variables; update that branch to guard the access the same way as defaultScopeId — e.g., use the same |default(null) guard or check workstation.scope is defined before accessing .id so selectedScopeIds uses selectedScopeIds is not empty ? selectedScopeIds : ( (workstation.scope.id|default(null)) ? [workstation.scope.id|default(null)] : [] ); ensure you reference the same symbol names (workstation.scope.id and selectedScopeIds) when making the change.zmsstatistic/src/Zmsstatistic/Helper/Access.php (1)
110-110: 💤 Low valueNarrow the scope bypass condition in
Access::isPathWithoutScope()
Access::isPathWithoutScope($path)skipsWorkstationMissingScopewhen$pathcontains the substringreport(zmsstatistic/src/Zmsstatistic/Helper/Access.php:110). In currentzmsstatistic/routing.php, the only route paths containingreportare the intended/report/...pages, and the report controllers/services already handle an empty scope ID by returningnulldata—so no immediate unintended bypass. Still, substring matching is brittle for future routes; match'/report/'(or check the matched route name) instead ofstrpos($path, 'report'). [TODO at :106 indicates these rules should move into the controller.]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsstatistic/src/Zmsstatistic/Helper/Access.php` at line 110, The bypass condition in Access::isPathWithoutScope() is too broad because it checks for the substring 'report'; narrow it to detect the actual report route by matching '/report/' (or, even better, check the matched route name if available). Update the condition that currently uses strpos($path, 'report') to instead look for '/report/' (e.g., use strpos($path, '/report/') !== false) or replace with a route-name check so only true report paths are exempt; keep the rest of the boolean logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@zmsstatistic/src/Zmsstatistic/Helper/Access.php`:
- Line 110: The bypass condition in Access::isPathWithoutScope() is too broad
because it checks for the substring 'report'; narrow it to detect the actual
report route by matching '/report/' (or, even better, check the matched route
name if available). Update the condition that currently uses strpos($path,
'report') to instead look for '/report/' (e.g., use strpos($path, '/report/')
!== false) or replace with a route-name check so only true report paths are
exempt; keep the rest of the boolean logic unchanged.
In `@zmsstatistic/templates/page/reportCapacityIndex.twig`:
- Around line 62-63: The template uses fragile positional indexes row[2] and
row[3] for booked/planned; update the data provider (service/entity that builds
the rows) to return associative arrays or objects with named keys (e.g. planned
and booked) and then change the template access from row[2]/row[3] to
row.planned and row.booked (or row['planned']/row['booked']) in the
reportCapacityIndex.twig so the booked/planned columns no longer depend on
column order.
- Around line 18-19: The selectedScopeIds expression dereferences
workstation.scope.id directly (in the ternary on the second line) which can
error under strict_variables; update that branch to guard the access the same
way as defaultScopeId — e.g., use the same |default(null) guard or check
workstation.scope is defined before accessing .id so selectedScopeIds uses
selectedScopeIds is not empty ? selectedScopeIds : (
(workstation.scope.id|default(null)) ? [workstation.scope.id|default(null)] : []
); ensure you reference the same symbol names (workstation.scope.id and
selectedScopeIds) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e79b570b-2f15-45cb-8783-5b9d24d6b842
📒 Files selected for processing (17)
zmsdb/tests/Zmsdb/ExchangeCapacityscopeTest.phpzmsdb/tests/Zmsdb/ExchangeSubjectListTest.phpzmsstatistic/src/Zmsstatistic/Helper/Access.phpzmsstatistic/src/Zmsstatistic/Helper/ReportHelper.phpzmsstatistic/src/Zmsstatistic/ReportCapacityIndex.phpzmsstatistic/src/Zmsstatistic/ReportClientIndex.phpzmsstatistic/src/Zmsstatistic/ReportRequestIndex.phpzmsstatistic/src/Zmsstatistic/ReportWaitingIndex.phpzmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.phpzmsstatistic/src/Zmsstatistic/Service/ReportClientService.phpzmsstatistic/src/Zmsstatistic/Service/ReportRequestService.phpzmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.phpzmsstatistic/templates/block/header/pageheader.twigzmsstatistic/templates/block/navigation/navigation.twigzmsstatistic/templates/block/report/period.twigzmsstatistic/templates/page/reportCapacityIndex.twigzmsstatistic/tests/Zmsstatistic/ReportClientScopeTest.php
✅ Files skipped from review due to trivial changes (2)
- zmsdb/tests/Zmsdb/ExchangeSubjectListTest.php
- zmsdb/tests/Zmsdb/ExchangeCapacityscopeTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsstatistic/src/Zmsstatistic/ReportCapacityIndex.php
- zmsstatistic/src/Zmsstatistic/Service/ReportCapacityService.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce the Terminkapazität Auswertung with multi-scope aggregation, slotscope warehouse queries (hourly up to 14 days), chart timeline filling, adaptive x-axis label intervals, scope date bounds in the filter, and an AJAX refresh control beside download.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.docshinzugefügt.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests