Code Review: fix: absolute URLs in customer address management (#142)

Общая оценка

Рекомендация: Approve — PR решает заявленную проблему, код качественный, подход корректен. Есть несколько некритичных замечаний.

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

1. Исправление проблемы с <base href>

Корректное решение: использование makeUrl() с параметром 'full' для формирования абсолютных URL. Относительные ссылки (?mode=create, ?mode=edit&id=..., ?) действительно ломаются при наличии <base href> в MODX.

2. Архитектура изменений

• page_url передаётся единообразно во все три метода AddressesPageService: renderList, renderCreateForm, renderEditForm
• В renderList URL добавляется и в общие данные, и в каждый элемент $addressData для row-чанка — правильно, т.к. pdoFetch передаёт данные в чанк по-разному

3. Интернационализация (второй коммит)

Замена хардкода на ключи лексикона в CustomerAddressController — правильный подход. Сообщения API теперь зависят от локали, что важно для мультиязычных сайтов.

4. Полнота лексикона

Добавлены ключи для всех сценариев: ms3_customer_address_deleted, ms3_customer_address_default_set, ms3_customer_address_already_exists, ошибки и т.д. Синхронизация en/ru выполнена.

Замечания и рекомендации

1. Потеря контекста в ms3_customer_err_field_required (низкий приоритет)

Файл: CustomerAddressController.php, метод create()

Было: "Field '{$field}' is required" — пользователь видел, какое именно поле пустое.
Стало: $this->modx->lexicon('ms3_customer_err_field_required') — общее «Поле обязательно для заполнения».

Рекомендация: добавить плейсхолдер в лексикон, например:

$_lang['ms3_customer_err_field_required'] = 'Field "[[+field]]" is required';

И вызывать: $this->modx->lexicon('ms3_customer_err_field_required', ['field' => $field]) — тогда UX будет как в оригинале.

2. Загрузка лексикона в конструкторе

$this->modx->lexicon->load('minishop3:customer');

Корректно. Лексикон подгружается один раз при создании контроллера; для API-контроллера это допустимо. Если в будущем появятся другие контроллеры с тем же лексиконом, можно вынести загрузку в общий bootstrap.

3. Удаление success-сообщений для getList/get

Убраны 'Addresses retrieved successfully' и 'Address retrieved successfully' — разумно, т.к. для списка/одного адреса обычно не показывают toast.

Безопасность

• Проверка customer_id при доступе к адресам сохранена
• makeUrl() использует ID ресурса из контекста — нет риска внедрения URL

Тестирование

Test plan в описании PR адекватен. Дополнительно стоит проверить:

• Сайт с modx.friendly_urls = 1 и без friendly_urls
• Сайт с modx.friendly_urls_prefix и friendly_urls_suffix
• Смена языка в ЛК — сообщения API должны соответствовать выбранной локали

Итог

PR готов к мержу. Замечание по ms3_customer_err_field_required — некритичное улучшение, можно внести в отдельный PR или оставить как есть.

Code Review (обновлено): fix: absolute URLs in customer address management (#142)

Вердикт

Approve — PR готов к мержу. Исправления корректны, архитектура выдержана, i18n реализована правильно.

Резюме изменений

Сильные стороны

1. Корректное решение проблемы <base href> — makeUrl(..., 'full') даёт абсолютный URL, что устраняет конфликт с базовым тегом MODX.

2. Единообразие — page_url передаётся во все три метода и во все чанки, включая $addressData для row (pdoFetch передаёт данные в чанк отдельно).

3. i18n — API-ответы зависят от локали, что важно для мультиязычных сайтов.

4. Полнота лексикона — добавлены все нужные ключи, en/ru синхронизированы.

Рекомендации (не блокируют мерж)

ms3_customer_err_field_required — потеря контекста

Текущее поведение: при пустом name, city или street пользователь видит «Поле обязательно для заполнения» без указания поля.

Было: "Field '{$field}' is required" — явно указывало поле.

Вариант улучшения (опционально, в отдельном PR):

// Лексикон: 'Field "[[+field]]" is required' / 'Поле «[[+field]]» обязательно'
return Response::error($this->modx->lexicon('ms3_customer_err_field_required', ['field' => $field]), 400)->getData();

Безопасность

• Проверка customer_id при доступе к адресам сохранена.
• makeUrl() использует ID ресурса из контекста — рисков внедрения URL нет.

Дополнительные проверки

• Сайт с friendly_urls = 0 и friendly_urls = 1
• Смена языка в ЛК — toast-сообщения API должны соответствовать локали