-
Notifications
You must be signed in to change notification settings - Fork 1
feat(ZMSKVR-1123): new table for total number of customers in waiting statistics #1813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
feat(ZMSKVR-1123): new table for total number of customers in waiting statistics #1813
Conversation
…-and-location-for-screen-readers Merge branch 'zmskvr-900' into feature-zmskvr-1123-combining-waiting-times-for-spontaneous-and-scheduled-customers
📝 WalkthroughWalkthroughChanges introduce i18n support for accessibility labels in Vue appointment UI components (CalendarView, ListView, TimeSlotGrid) and refactor PHP report generation to support three distinct customer categories (Gesamt/total, Terminkunden, Spontankunden) with corresponding new aggregation methods and multi-sheet Excel output. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsstatistic/src/Zmsstatistic/Download/WaitingReport.php (1)
39-39: Reconsider the @SuppressWarnings annotation.The
@SuppressWarnings(Param)annotation suggests a code quality issue that could be addressed through refactoring rather than suppression. Consider fixing the underlying issue instead of suppressing the warning.As per coding guidelines, excessive use of @SuppressWarnings without proper justification may indicate technical debt.
🧹 Nitpick comments (6)
zmsstatistic/tests/Zmsstatistic/ReportWaitingOrganisationTest.php (1)
166-178: Good XLSX validation, but consider explicit error handling.The implementation correctly validates the XLSX structure, including sheet names and the active sheet. However, the
@unlink($tmp)on line 178 suppresses any deletion errors.Consider replacing:
@unlink($tmp);With explicit cleanup:
if (file_exists($tmp)) { unlink($tmp); }This provides better visibility if cleanup fails.
zmsstatistic/tests/Zmsstatistic/ReportWaitingDepartmentTest.php (1)
166-178: Good XLSX validation, but consider explicit error handling.Same as ReportWaitingOrganisationTest: consider replacing
@unlink($tmp)with explicit file existence check before deletion for better error visibility.zmsstatistic/src/Zmsstatistic/ReportWaitingDepartment.php (1)
52-70: Consider extracting duplicated processing logic.The data processing sequence (lines 52-70) is duplicated across:
ReportWaitingDepartment.php(this file)ReportWaitingOrganisation.phpReportWaitingService.php(bothgetExchangeWaitingForPeriodandcreateFilteredExchangeWaiting)This violates the DRY principle. Consider extracting this sequence into a shared method in
ReportHelper:public static function processWaitingData($exchangeWaiting, array $hashset): mixed { $exchangeWaiting = self::withTotalCustomers($exchangeWaiting); $exchangeWaiting = $exchangeWaiting ->withMaxByHour($hashset) ->withMaxAndAverageFromWaitingTime(); // Apply per-date aggregations foreach (['waitingtime', 'waitingtime_termin', 'waytime', 'waytime_termin', 'waitingtime_total', 'waytime_total'] as $key) { $exchangeWaiting = self::withMaxAndAverage($exchangeWaiting, $key); } // Apply global aggregations for totals $exchangeWaiting = self::withGlobalMaxAndAverage($exchangeWaiting, 'waitingtime_total'); $exchangeWaiting = self::withGlobalMaxAndAverage($exchangeWaiting, 'waytime_total'); return $exchangeWaiting; }This would reduce maintenance burden and ensure consistency across all three locations.
zmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.php (1)
91-112: Significant code duplication within this file and across related files.The processing logic in
getExchangeWaitingForPeriod(lines 91-112) andcreateFilteredExchangeWaiting(lines 197-211) is nearly identical. Additionally, this same pattern is duplicated in:
ReportWaitingDepartment.phpReportWaitingOrganisation.phpRecommended refactoring:
Extract the common processing into a helper method in
ReportHelper:public static function applyWaitingAggregations($exchangeWaiting, array $hashset): mixed { $exchangeWaiting = self::withTotalCustomers($exchangeWaiting); $exchangeWaiting = $exchangeWaiting ->withMaxByHour($hashset) ->withMaxAndAverageFromWaitingTime(); // Apply all per-date aggregations foreach (['waitingtime', 'waitingtime_termin', 'waytime', 'waytime_termin', 'waitingtime_total', 'waytime_total'] as $key) { $exchangeWaiting = self::withMaxAndAverage($exchangeWaiting, $key); } // Apply global aggregations for totals $exchangeWaiting = self::withGlobalMaxAndAverage($exchangeWaiting, 'waitingtime_total'); $exchangeWaiting = self::withGlobalMaxAndAverage($exchangeWaiting, 'waytime_total'); return $exchangeWaiting; }Then replace all occurrences with:
$exchangeWaiting = ReportHelper::applyWaitingAggregations($exchangeWaiting, $this->hashset);This eliminates duplication across 4 locations and ensures consistency.
Also applies to: 197-211
zmsstatistic/src/Zmsstatistic/ReportWaitingOrganisation.php (1)
52-70: Code duplication with ReportWaitingDepartment and ReportWaitingService.This processing logic (lines 52-70) is identical to
ReportWaitingDepartment.php(lines 52-70) and similar to logic inReportWaitingService.php. Please see the comment onReportWaitingDepartment.phpfor the recommended refactoring approach to extract this into a sharedReportHelper::processWaitingData()method.zmsstatistic/src/Zmsstatistic/Helper/ReportHelper.php (1)
69-110: LGTM! Well-designed global aggregation method.The
withGlobalMaxAndAveragemethod correctly:
- Aggregates maximum and average values across all date/hour entries
- Guards against division by zero (line 93)
- Handles both object and array data structures (lines 95-107)
- Filters out zero/null values appropriately (line 84)
Optional enhancement for clarity:
Consider extracting the duplicate logic for setting max/average into a helper:
private static function setMaxAndAverageData(&$data, string $targetKey, float $maxima, float $average): void { if (!isset($data['max']) || !is_array($data['max'])) { $data['max'] = []; } $data['max']['max_' . $targetKey] = $maxima; $data['max']['average_' . $targetKey] = $average; }Then lines 95-107 become:
if (is_object($entity->data)) { $data = &$entity->data; self::setMaxAndAverageData($data, $targetKey, $maxima, $average); } elseif (is_array($entity->data)) { self::setMaxAndAverageData($entity->data, $targetKey, $maxima, $average); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
zmscitizenview/src/components/Appointment/AppointmentSelection/CalendarView.vue(2 hunks)zmscitizenview/src/components/Appointment/AppointmentSelection/ListView.vue(2 hunks)zmscitizenview/src/components/Appointment/AppointmentSelection/TimeSlotGrid.vue(3 hunks)zmscitizenview/tests/unit/Appointment/AppointmentSelection/TimeSlotGrid.spec.ts(2 hunks)zmsstatistic/src/Zmsstatistic/Download/WaitingReport.php(3 hunks)zmsstatistic/src/Zmsstatistic/Helper/ReportHelper.php(1 hunks)zmsstatistic/src/Zmsstatistic/ReportWaitingDepartment.php(2 hunks)zmsstatistic/src/Zmsstatistic/ReportWaitingOrganisation.php(2 hunks)zmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.php(5 hunks)zmsstatistic/templates/page/reportWaitingIndex.twig(1 hunks)zmsstatistic/tests/Zmsstatistic/ReportWaitingDepartmentTest.php(3 hunks)zmsstatistic/tests/Zmsstatistic/ReportWaitingOrganisationTest.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:
zmsstatistic/tests/Zmsstatistic/ReportWaitingOrganisationTest.phpzmsstatistic/src/Zmsstatistic/Helper/ReportHelper.phpzmscitizenview/src/components/Appointment/AppointmentSelection/ListView.vuezmscitizenview/src/components/Appointment/AppointmentSelection/TimeSlotGrid.vuezmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.phpzmsstatistic/tests/Zmsstatistic/ReportWaitingDepartmentTest.phpzmscitizenview/tests/unit/Appointment/AppointmentSelection/TimeSlotGrid.spec.tszmsstatistic/templates/page/reportWaitingIndex.twigzmsstatistic/src/Zmsstatistic/ReportWaitingOrganisation.phpzmsstatistic/src/Zmsstatistic/ReportWaitingDepartment.phpzmsstatistic/src/Zmsstatistic/Download/WaitingReport.phpzmscitizenview/src/components/Appointment/AppointmentSelection/CalendarView.vue
**/*.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:
zmsstatistic/tests/Zmsstatistic/ReportWaitingOrganisationTest.phpzmsstatistic/src/Zmsstatistic/Helper/ReportHelper.phpzmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.phpzmsstatistic/tests/Zmsstatistic/ReportWaitingDepartmentTest.phpzmsstatistic/src/Zmsstatistic/ReportWaitingOrganisation.phpzmsstatistic/src/Zmsstatistic/ReportWaitingDepartment.phpzmsstatistic/src/Zmsstatistic/Download/WaitingReport.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:
zmscitizenview/tests/unit/Appointment/AppointmentSelection/TimeSlotGrid.spec.ts
🧬 Code graph analysis (3)
zmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.php (2)
zmsentities/src/Zmsentities/Exchange.php (1)
toGrouped(247-253)zmsstatistic/src/Zmsstatistic/Helper/ReportHelper.php (4)
ReportHelper(7-187)withTotalCustomers(32-67)withMaxAndAverage(9-30)withGlobalMaxAndAverage(69-110)
zmsstatistic/src/Zmsstatistic/ReportWaitingDepartment.php (2)
zmsentities/src/Zmsentities/Exchange.php (3)
toGrouped(247-253)withMaxByHour(108-125)withMaxAndAverageFromWaitingTime(186-206)zmsstatistic/src/Zmsstatistic/Helper/ReportHelper.php (4)
ReportHelper(7-187)withTotalCustomers(32-67)withMaxAndAverage(9-30)withGlobalMaxAndAverage(69-110)
zmsstatistic/src/Zmsstatistic/Download/WaitingReport.php (3)
zmsstatistic/src/Zmsstatistic/Download/Base.php (1)
writeInfoHeader(66-108)zmsstatistic/src/Zmsstatistic/Helper/Download.php (1)
writeDownload(40-59)zmsstatistic/src/Zmsstatistic/Download/RequestReport.php (1)
writeHeader(71-83)
🔇 Additional comments (13)
zmscitizenview/src/components/Appointment/AppointmentSelection/TimeSlotGrid.vue (1)
31-31: LGTM! Excellent accessibility enhancement.The addition of the
aria-labelbinding with the newtprop andtimeSlotAriaLabelhelper function significantly improves screen reader support. The implementation correctly combines the formatted time, localized timestamp suffix, and optional office name into a descriptive label.Also applies to: 53-53, 63-72
zmscitizenview/src/components/Appointment/AppointmentSelection/CalendarView.vue (1)
99-99: LGTM! Proper translation function forwarding.The translation function prop is correctly passed to both TimeSlotGrid instances (hourly view and day-part view), enabling localized accessibility labels.
Also applies to: 223-223
zmscitizenview/src/components/Appointment/AppointmentSelection/ListView.vue (1)
125-125: LGTM! Consistent translation function forwarding.The translation function is correctly passed to TimeSlotGrid in both hourly and day-part views, maintaining consistency with CalendarView.vue.
Also applies to: 197-197
zmscitizenview/tests/unit/Appointment/AppointmentSelection/TimeSlotGrid.spec.ts (1)
19-22: LGTM! Good test coverage for the accessibility feature.The mock translation function and new test case properly verify that the aria-label is constructed correctly with all expected components (formatted time, localized suffix, and office name).
Also applies to: 31-31, 94-101
zmsstatistic/templates/page/reportWaitingIndex.twig (1)
20-24: LGTM! Clean addition of the Gesamt section.The new "Gesamt" (total) section is properly positioned first in the array and follows the same structure as the existing sections. This aligns well with the backend changes that compute the
_totalsuffix fields.zmsstatistic/tests/Zmsstatistic/ReportWaitingOrganisationTest.php (2)
4-4: LGTM! Proper import for XLSX validation.The IOFactory import is necessary for the new XLSX validation logic added in testWithDownloadXLSX.
106-114: LGTM! Comprehensive validation of section ordering.The test correctly verifies both the presence and the order of the three sections (Gesamt, Terminkunden, Spontankunden), ensuring the new total section is properly integrated into the rendered output.
zmsstatistic/tests/Zmsstatistic/ReportWaitingDepartmentTest.php (2)
4-4: LGTM! Proper import for XLSX validation.
106-113: LGTM! Comprehensive validation of section ordering.The assertions correctly verify the presence and order of the three sections in the rendered output.
zmsstatistic/src/Zmsstatistic/ReportWaitingDepartment.php (1)
26-28: LGTM! Proper addition of total fields to hashset.The three new total fields (
waitingcount_total,waitingtime_total,waytime_total) are correctly added to the hashset for grouping operations.zmsstatistic/src/Zmsstatistic/Service/ReportWaitingService.php (1)
26-28: LGTM! Proper addition of total fields to hashset.zmsstatistic/src/Zmsstatistic/ReportWaitingOrganisation.php (1)
26-28: LGTM! Proper addition of total fields to hashset.zmsstatistic/src/Zmsstatistic/Helper/ReportHelper.php (1)
32-67: LGTM! Correct implementation of weighted average for total customers.The
withTotalCustomersmethod correctly:
- Computes total customer count as sum of spontaneous and scheduled customers
- Calculates weighted averages for waiting time and way time using the formula:
(value1 * count1 + value2 * count2) / totalCount- Guards against division by zero with
$countTotal > 0checks- Handles non-array items gracefully with
continuestatementsThe logic is sound and the implementation is clean.
…ombining-waiting-times-for-spontaneous-and-scheduled-customers
…for-spontaneous-and-scheduled-customers
|
@coderabbitai would you rename customer to citizen or appointment (which one?) in in assertValidCustomerType CUSTOMER_TYPE_GESAMT CUSTOMER_TYPE_TERMIN CUSTOMER_TYPE_SPONTAN later? Also would you rename gesamt termin and spontan to english later? If so create a cleanup issue for this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13619 🏁 Script executed: Length of output: 3826 🏁 Script executed: Length of output: 1191 🏁 Script executed: Length of output: 72 @ThomasAFink Yes, I would recommend renaming "customer" to "citizen" (not "appointment") based on my analysis of the codebase: Why "citizen" instead of "appointment"?
Recommended changes:1. Rename customer → citizen
2. Translate German to English
Cleanup issue created ✅I've created issue #1820 to track this refactoring work: The issue includes all the specific files and changes needed, and is assigned to you. This can be addressed in a future PR to maintain consistency across the codebase. |
…ombining-waiting-times-for-spontaneous-and-scheduled-customers
merge next into feature-zmskvr-1123-combining-waiting-times-for-spontaneous-and-scheduled-customers
…-combining-waiting-times-for-spontaneous-and-scheduled-customers' into feature-zmskvr-1123-combining-waiting-times-for-spontaneous-and-scheduled-customers
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.