# Code Review Request: Employee Scheduling Admin UI (Phase 4)

## Context

I'm implementing an employee scheduling system for a multi-store retail application (PHP 8.x, Slim 2.6.2, Twig, MySQL). This is Phase 4 of a 7-phase implementation. The previous phases implemented:
- Phase 1: Database migrations (9 tables)
- Phase 2: BuyerKioskSchedule provider
- Phase 3: Models, Repositories, and Services (OvertimeCalculator, LaborCostCalculator, TimesheetExporter)

Phase 4 adds the Admin UI using Syncfusion EJ2 Schedule component.

## Files to Review

Please review the following files for:
1. **Security vulnerabilities** (SQL injection, XSS, CSRF, authorization bypasses)
2. **Code quality** (SOLID principles, error handling, edge cases)
3. **PHP best practices** (type hints, null safety, exception handling)
4. **API design** (REST conventions, response formats, HTTP status codes)
5. **JavaScript quality** (error handling, memory leaks, async patterns)
6. **Performance concerns** (N+1 queries, missing indexes, inefficient algorithms)

---

## File 1: SchedulingController.php (~900 lines)

```php
<?php

namespace BuyerKiosk\Scheduling\Controllers;

use BuyerKiosk\Scheduling\Models\Shift;
use BuyerKiosk\Scheduling\Repositories\ShiftRepository;
use BuyerKiosk\Scheduling\Repositories\PositionRepository;
use BuyerKiosk\Scheduling\Repositories\ShiftAuditRepository;
use BuyerKiosk\Scheduling\Services\LaborCostCalculator;
use BuyerKiosk\Scheduling\Services\OvertimeCalculator;
use BuyerKiosk\Scheduling\Repositories\TimePunchRepository;
use DateTime;
use DateTimeZone;
use Exception;
use PDO;

/**
 * SchedulingController - REST API for Employee Scheduling
 *
 * Provides endpoints for managing employee schedules using the BuyerKiosk native provider.
 *
 * Authentication: All endpoints require session authentication + appropriate permissions:
 * - uri_schedule: Read access (GET endpoints)
 * - uri_schedule_manage: Write access (POST/PUT/DELETE for shifts)
 * - uri_schedule_config: Configuration access (PUT for config/overtime)
 */
class SchedulingController
{
    private $app;
    private $store;
    private string $typeNum;
    private ?PDO $db = null;
    private ?PDO $centralDb = null;
    private ?ShiftRepository $shiftRepository = null;
    private ?PositionRepository $positionRepository = null;
    private ?ShiftAuditRepository $shiftAuditRepository = null;
    private ?LaborCostCalculator $laborCostCalculator = null;
    private ?OvertimeCalculator $overtimeCalculator = null;

    public function __construct($app, \Store $store)
    {
        $this->app = $app;
        $this->store = $store;
        $this->typeNum = $store->getTypeNum();
    }

    // Lazy initialization methods...
    private function getDb(): PDO { /* ... */ }
    private function getCentralDb(): PDO { /* ... */ }
    private function getShiftRepository(): ShiftRepository { /* ... */ }
    // etc.

    // Authentication checks
    private function checkReadAuth(): bool
    {
        if (!isset($this->app->user) || !$this->app->user) {
            $this->sendErrorResponse('Authentication required', 401, 'UNAUTHORIZED');
            return false;
        }

        if (!$this->app->user->checkAccess('uri_schedule')) {
            $this->sendErrorResponse('Access denied. Requires uri_schedule permission', 403, 'FORBIDDEN');
            return false;
        }

        return true;
    }

    // Similar for checkWriteAuth() and checkConfigAuth()...

    // API Endpoints:

    /**
     * GET /api/:typeNum/schedule/shifts
     * Get shifts for a date range.
     */
    public function getShifts(): void
    {
        if (!$this->checkReadAuth()) return;

        $start = $this->app->request->get('start');
        $end = $this->app->request->get('end');

        if (!$start || !$end) {
            $this->sendErrorResponse('start and end parameters are required', 400, 'MISSING_PARAMS');
            return;
        }

        try {
            $startDate = new DateTime($start, new DateTimeZone($this->getStoreTimezone()));
            $endDate = new DateTime($end, new DateTimeZone($this->getStoreTimezone()));

            $shifts = $this->getShiftRepository()->findByDateRange($startDate, $endDate);

            $response = [];
            foreach ($shifts as $shift) {
                $response[] = $this->formatShiftForApi($shift);
            }

            $this->sendJsonResponse($response);
        } catch (Exception $e) {
            error_log("SchedulingController::getShifts error: " . $e->getMessage());
            $this->sendErrorResponse('Failed to retrieve shifts', 500, 'SERVER_ERROR');
        }
    }

    /**
     * POST /api/:typeNum/schedule/shifts
     * Create a new shift.
     */
    public function createShift(): void
    {
        if (!$this->checkWriteAuth()) return;

        $data = json_decode($this->app->request->getBody(), true);

        if (!$data || !isset($data['employeeId']) || !isset($data['shiftStart']) || !isset($data['shiftEnd'])) {
            $this->sendErrorResponse('employeeId, shiftStart, and shiftEnd are required', 400, 'MISSING_PARAMS');
            return;
        }

        try {
            $tz = new DateTimeZone($this->getStoreTimezone());
            $shiftStart = new DateTime($data['shiftStart'], $tz);
            $shiftEnd = new DateTime($data['shiftEnd'], $tz);

            $shift = new Shift(
                (int)$data['employeeId'],
                $shiftStart,
                $shiftEnd,
                $this->getCurrentUserId()
            );

            if (isset($data['positionId'])) {
                $shift->setPositionId((int)$data['positionId']);
            }

            $createdShift = $this->getShiftRepository()->create($shift);

            // Write audit log
            $this->getShiftAuditRepository()->logCreate(
                $createdShift->getShiftId(),
                $this->getCurrentUserId(),
                $createdShift->toDbArray()
            );

            // Calculate labor cost impact
            $weekStart = $this->getWeekStart($shiftStart);
            $laborCost = $this->getLaborCostCalculator()->calculateWeekCost(
                $this->typeNum,
                $weekStart,
                $this->getStoreTimezone()
            );

            $this->sendJsonResponse([
                'success' => true,
                'shiftId' => $createdShift->getShiftId(),
                'laborCost' => [
                    'regular' => $laborCost->getRegularCost(),
                    'overtime' => $laborCost->getOvertimeCost(),
                    'total' => $laborCost->getTotalCost()
                ]
            ], 201);
        } catch (Exception $e) {
            if (strpos($e->getMessage(), 'OVERLAP') !== false) {
                $this->sendErrorResponse('Employee already has a shift during this time', 409, 'OVERLAP');
            } else {
                error_log("SchedulingController::createShift error: " . $e->getMessage());
                $this->sendErrorResponse('Failed to create shift', 500, 'SERVER_ERROR');
            }
        }
    }

    /**
     * PUT /api/:typeNum/schedule/shifts/:shiftId
     * Update an existing shift with optimistic concurrency.
     */
    public function updateShift(int $shiftId): void
    {
        if (!$this->checkWriteAuth()) return;

        $data = json_decode($this->app->request->getBody(), true);

        try {
            $existingShift = $this->getShiftRepository()->findById($shiftId);

            if (!$existingShift) {
                $this->sendErrorResponse('Shift not found', 404, 'NOT_FOUND');
                return;
            }

            // Capture old values for audit log
            $oldValues = $existingShift->toDbArray();

            // Apply updates...
            // Optimistic concurrency check using updatedAt token

            $updatedShift = $this->getShiftRepository()->update($existingShift, $expectedUpdatedAt);

            // Write audit log with old and new values
            $this->getShiftAuditRepository()->logUpdate(
                $updatedShift->getShiftId(),
                $this->getCurrentUserId(),
                $oldValues,
                $updatedShift->toDbArray()
            );

            // Return updated labor costs...
        } catch (Exception $e) {
            if (strpos($e->getMessage(), 'STALE_WRITE') !== false) {
                $this->sendErrorResponse('Shift changed since you loaded it. Please reload.', 409, 'STALE_WRITE');
            }
            // ... other error handling
        }
    }

    // ... 10 more endpoint methods (DELETE, copy-preview, copy, employees, positions, labor-cost, config, overtime)

    /**
     * GET /api/:typeNum/schedule/employees
     * Get employees available for scheduling with weekly hours.
     */
    public function getEmployees(): void
    {
        if (!$this->checkReadAuth()) return;

        try {
            $weekStartUtc = new DateTime('now', new DateTimeZone('UTC'));
            $weekEndUtc = (clone $weekStartUtc)->modify('+7 days');

            // Note: Scheduling uses `kiosk_users.users.id` as the employee identifier (global),
            // and store membership comes from `kiosk_users.userStoreAssignments`.
            $stmt = $this->getDb()->prepare("
                SELECT
                    u.id,
                    CONCAT(u.firstName, ' ', u.lastName) as name,
                    COALESCE(wh.weeklyHours, 0) as weeklyHours
                FROM kiosk_users.users u
                INNER JOIN kiosk_users.userStoreAssignments usa ON u.id = usa.userId
                LEFT JOIN (
                    SELECT employeeId, SUM(TIMESTAMPDIFF(MINUTE, shiftStart, shiftEnd)) / 60 as weeklyHours
                    FROM scheduleShifts
                    WHERE shiftStart >= :weekStart
                      AND shiftStart < :weekEnd
                      AND deleted_at IS NULL
                    GROUP BY employeeId
                ) wh ON wh.employeeId = u.id
                WHERE usa.typeNum = :typeNum
                  AND usa.isActive = 1
                  AND u.active = 1
                ORDER BY u.lastName, u.firstName
            ");

            // Execute and return...
        } catch (Exception $e) {
            // Error handling...
        }
    }

    // Helper methods...
    private function formatShiftForApi(Shift $shift): array { /* ... */ }
    private function getWeekStart(DateTime $date): DateTime { /* ... */ }
    private function sendJsonResponse($data, int $status = 200): void { /* ... */ }
    private function sendErrorResponse(string $message, int $status = 400, string $code = 'ERROR'): void { /* ... */ }
}
```

---

## File 2: Routes - scheduling.php

```php
<?php

use BuyerKiosk\Scheduling\Controllers\SchedulingController;

// Route group: /api/:typeNum/schedule
$app->group('/:typeNum/schedule', function () use ($app) {

    $getValidatedStore = function ($typeNum) use ($app) {
        $storeController = new \BuyerKiosk\StoreController($typeNum);
        $store = $storeController->getStore();

        if (!$store) {
            $app->response->headers->set('Content-Type', 'application/json');
            $app->response->setStatus(404);
            $app->response->setBody(json_encode([
                'success' => false,
                'error' => 'Store not found',
                'code' => 'STORE_NOT_FOUND'
            ]));
            return null;
        }

        // Verify user has access to this store
        if (isset($app->user) && $app->user && !$app->user->checkStoreGroup($typeNum)) {
            $app->response->headers->set('Content-Type', 'application/json');
            $app->response->setStatus(403);
            $app->response->setBody(json_encode([
                'success' => false,
                'error' => 'Access denied to this store',
                'code' => 'STORE_ACCESS_DENIED'
            ]));
            return null;
        }

        return $store;
    };

    // Shift endpoints
    $app->get('/shifts', function ($typeNum) use ($app, $getValidatedStore) {
        $store = $getValidatedStore($typeNum);
        if ($store === null) return;
        $controller = new SchedulingController($app, $store);
        $controller->getShifts();
    });

    // ... more routes for POST, PUT, DELETE, etc.
});
```

---

## File 3: ScheduleCalendar.js (~600 lines)

```javascript
class ScheduleCalendar {
    constructor(containerId, typeNum, options = {}) {
        this.typeNum = typeNum;
        this.containerId = containerId;
        this.schedule = null;
        this.positions = [];
        this.employees = [];
        this.options = Object.assign({
            startHour: '06:00',
            endHour: '23:00',
            firstDayOfWeek: 1,
            defaultView: 'TimelineWeek'
        }, options);

        this.onLaborCostUpdate = options.onLaborCostUpdate || null;
        this.onError = options.onError || this.defaultErrorHandler.bind(this);
    }

    async initialize() {
        try {
            this.showLoading();

            const [employees, positions] = await Promise.all([
                this.fetchEmployees(),
                this.fetchPositions()
            ]);

            this.employees = employees;
            this.positions = positions;

            // Create Syncfusion Schedule instance
            this.schedule = new ej.schedule.Schedule({
                currentView: 'TimelineWeek',
                views: [
                    { option: 'Day' },
                    { option: 'Week' },
                    { option: 'TimelineWeek', displayName: 'Schedule' },
                    { option: 'Month' }
                ],
                group: { resources: ['Employees'] },
                resources: [{
                    field: 'employeeId',
                    name: 'Employees',
                    dataSource: this.formatEmployeesForSchedule(employees),
                    textField: 'name',
                    idField: 'id'
                }],
                allowDragAndDrop: true,
                allowResizing: true,
                actionBegin: this.onActionBegin.bind(this),
                // ... more config
            });

            this.schedule.appendTo(`#${this.containerId}`);
            await this.loadShifts();
            await this.updateLaborCosts();
        } catch (error) {
            this.onError('Failed to initialize schedule: ' + error.message);
        }
    }

    async onActionBegin(args) {
        if (args.requestType === 'eventCreate') {
            args.cancel = true; // Cancel default, use our API
            await this.createShift(args.data[0]);
        } else if (args.requestType === 'eventChange') {
            args.cancel = true;
            await this.updateShift(args.data);
        } else if (args.requestType === 'eventRemove') {
            args.cancel = true;
            await this.deleteShift(args.data[0].shiftId);
        }
    }

    async createShift(shiftData) {
        const response = await fetch(`/api/${this.typeNum}/schedule/shifts`, {
            method: 'POST',
            headers: { 'Content-Type': 'application/json' },
            body: JSON.stringify({
                employeeId: shiftData.employeeId,
                shiftStart: shiftData.shiftStart.toISOString(),
                shiftEnd: shiftData.shiftEnd.toISOString(),
                positionId: shiftData.positionId || null
            })
        });

        if (response.ok) {
            await this.loadShifts();
            this.updateLaborCostDisplay(result.laborCost);
        } else {
            const error = await response.json();
            this.onError(error.error || 'Failed to create shift');
        }
    }

    // ... more methods for update, delete, copy, etc.
}
```

---

## Specific Questions

1. **SQL Injection**: The getEmployees() query uses prepared statements correctly, but is the `typeNum` variable properly sanitized? It comes from the URL.

2. **Authorization**: Is the permission check order correct? Should store access be checked before permission check?

3. **Race Conditions**: The optimistic concurrency uses `updatedAt` timestamp. Is this sufficient or should we use a version number?

4. **Error Leakage**: Are we leaking too much information in error responses?

5. **Memory/Performance**: The getEmployees query has correlated subqueries. Should these be JOINs instead?

6. **JavaScript**: Is there any XSS risk in the templates (shift-block, employee-resource)?

7. **CSRF**: The API uses session auth. Is CSRF protection needed for the POST/PUT/DELETE endpoints?

Please provide specific, actionable feedback with code examples where improvements are needed.