diff --git a/.planning/phases/08-backend-aggregation-filtering/08-REVIEW.md b/.planning/phases/08-backend-aggregation-filtering/08-REVIEW.md new file mode 100644 index 0000000..b669797 --- /dev/null +++ b/.planning/phases/08-backend-aggregation-filtering/08-REVIEW.md @@ -0,0 +1,91 @@ +--- +phase: 08-backend-aggregation-filtering +reviewed: 2026-04-09T12:00:00Z +depth: standard +files_reviewed: 6 +files_reviewed_list: + - Controller/HeatmapController.php + - Service/HeatmapService.php + - Tests/Controller/HeatmapControllerTest.php + - Tests/Service/HeatmapServiceTest.php + - Tests/bootstrap.php + - assets/src/types.ts +findings: + critical: 0 + warning: 2 + info: 2 + total: 4 +status: issues_found +--- + +# Phase 8: Code Review Report + +**Reviewed:** 2026-04-09T12:00:00Z +**Depth:** standard +**Files Reviewed:** 6 +**Status:** issues_found + +## Summary + +The backend aggregation and filtering implementation is solid overall. The controller uses proper Symfony auth guards and parameterized queries throughout, so there are no injection or auth bypass risks. The main concern is a DST-related logic bug in the timezone offset calculation for hourly aggregations, and a type naming mismatch between the TypeScript frontend types and the backend API modes. + +## Warnings + +### WR-01: Static timezone offset ignores DST transitions + +**File:** `Service/HeatmapService.php:236-244` +**Issue:** `computeTimezoneOffset()` calculates the UTC offset at the current moment (`new \DateTime('now')`), then uses that fixed offset string in `CONVERT_TZ()` for queries spanning an entire year. For any timezone with daylight saving time (e.g., Europe/Berlin which shifts between +01:00 and +02:00), entries on the "wrong" side of the DST boundary will have their hour slot shifted by one. This means hourly and day-hour aggregations will misattribute hours near the DST transition periods. +**Fix:** Pass the timezone name string directly to `CONVERT_TZ()` instead of a computed offset. MySQL supports named timezones if the `mysql.time_zone_name` table is populated (which it is on most installations): +```php +// Instead of computing offset, pass the IANA timezone name directly: +$sql = 'SELECT HOUR(CONVERT_TZ(t.start_time, \'+00:00\', :tz)) as hour_slot, ...'; +// where :tz = 'Europe/Berlin' instead of '+02:00' +``` +If the MySQL timezone tables aren't guaranteed to be populated, an alternative is to compute the offset per-row using the row's own date, but the named timezone approach is cleaner. + +### WR-02: Frontend TypeScript mode enum does not match backend API values + +**File:** `assets/src/types.ts:52` +**Issue:** `HeatmapMode` is defined as `'year' | 'week' | 'day' | 'combined'`, but the controller's `mode` query parameter expects `'daily'`, `'hourly'`, or `'dayhour'` (line 31-43 of `HeatmapController.php`). If the frontend sends the TypeScript enum values, the backend will fall through to the `default` branch and always return daily data regardless of the intended mode. This is either a naming mismatch that needs reconciling, or the TypeScript type represents a UI-layer concept that maps to the API values elsewhere -- but as written, using `HeatmapMode` as the API parameter value would be a bug. +**Fix:** Either align the TypeScript type with the API: +```typescript +export type HeatmapMode = 'daily' | 'hourly' | 'dayhour'; +``` +Or keep the UI-facing names and add an explicit mapping layer in the frontend fetch code that converts `'year' -> 'daily'`, `'day' -> 'hourly'`, `'combined' -> 'dayhour'`, etc. + +## Info + +### IN-01: Duplicated SQL filter clause construction + +**File:** `Service/HeatmapService.php:88-101` and `Service/HeatmapService.php:138-151` +**Issue:** The project/activity/customer filter SQL clauses are identical between `getHourlyAggregation()` and `getDayHourAggregation()`. This duplication means filter logic changes must be applied in two places. +**Fix:** Extract a private helper method: +```php +private function applyNativeFilters(string &$sql, array &$params, ?int $projectId, ?int $customerId, ?int $activityId): void +{ + if ($projectId !== null) { + $sql .= ' AND t.project_id = :project'; + $params['project'] = $projectId; + } + if ($activityId !== null) { + $sql .= ' AND t.activity_id = :activity'; + $params['activity'] = $activityId; + } + if ($customerId !== null) { + $sql .= ' AND t.project_id IN (SELECT p.id FROM kimai2_projects p WHERE p.customer_id = :customer)'; + $params['customer'] = $customerId; + } +} +``` + +### IN-02: Unused variable in test helper + +**File:** `Tests/Controller/HeatmapControllerTest.php:30` +**Issue:** `createControllerWithUser()` returns `[$controller, $user]` and most callers destructure both, but in several tests (lines 42, 80, 98, 123, 136, 148, 168, 188) the `$user` variable is assigned but never used. +**Fix:** Use `[$controller]` or `[$controller, $_]` in tests that don't need the user. This is a minor readability issue -- no functional impact. + +--- + +_Reviewed: 2026-04-09T12:00:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_