Ревью PR #119: feat(customer): add order cancel

Функциональность полезная, реализация в целом рабочая. Есть ряд замечаний.

1. Критическое: дублирование getAllowedCancelStatusIds()

Метод идентично скопирован в два класса:

• CustomerOrderController::getAllowedCancelStatusIds() (строки 90–104)
• OrdersPageService::getAllowedCancelStatusIds() (строки 460–474)

Это нарушает DRY. Если логика изменится, придётся менять в двух местах — и кто-то забудет. Нужно вынести в общее место — например, в OrderStatusService (который уже инжектится в контроллер), или в отдельный статический хелпер.

2. Дублирование getAuthorizedCustomer()

Метод скопирован из CustomerAddressController. Теперь одинаковый код авторизации живёт минимум в двух контроллерах. Стоит вынести в базовый класс (например, BaseWebController) или trait. Иначе каждый новый Web-контроллер будет копировать этот блок.

3. Дублирование JS в двух шаблонах

JavaScript для кнопки отмены практически идентичен в:

• ms3_customer_orders.tpl (строки 104–126)
• ms3_customer_order_details.tpl (строки 197–219)

~20 строк одинакового кода. Лучше вынести в отдельный JS-файл или хотя бы в один общий блок/чанк.

4. Хардкод пути к api.php

'api_url' => rtrim($this->modx->getOption('site_url'), '/') . '/assets/components/minishop3/api.php',

Путь к api.php зашит в строку. Если у пользователя кастомный assets_url или маппинг ассетов, URL будет неправильным. Лучше использовать конфиг MS3:

'api_url' => $this->ms3->config['assetsUrl'] . 'api.php',

Дублируется в трёх местах сервиса (renderOrdersList, renderOrderDetails, getOrderDetailsData).

5. Маршрут: размещение внутри группы /customer

Маршрут добавлен внутри группы /customer с $tokenMiddleware:

$router->post('/orders/{id}/cancel', function($params) use ($modx) {
    $controller = new \MiniShop3\Controllers\Api\Web\CustomerOrderController($modx);
    return $controller->cancel($params);
}, [$tokenMiddleware]);

TokenMiddleware уже валидирует токен и записывает $_SESSION['ms3']['customer_id']. После этого getAuthorizedCustomer() в контроллере всегда найдёт customer через сессию — двойная проверка избыточна (хотя и не вредна). Это ещё один аргумент за вынос авторизации в базовый класс.

6. Мелочь: избыточный fallback

$cancelledStatusId = (int) $this->modx->getOption('ms3_status_canceled', null, 5) ?: 5;

getOption уже возвращает 5 по умолчанию. ?: 5 дублирует это. Достаточно:

$cancelledStatusId = (int) $this->modx->getOption('ms3_status_canceled', null, 5);

Что сделано хорошо

• IDOR защищён: 'customer_id' => $customer->get('id') при загрузке заказа
• Проверка допустимых статусов через настройку с fallback
• Лексиконы на двух языках
• Кнопка отмены условно показывается через can_cancel
• Используется OrderStatusService::change() вместо прямого изменения — сохраняются события и логи
• Кнопка блокируется после клика (cancelButton.disabled = true)

Итог