feat(grid): inline edit for category products table#134
Conversation
- GridFieldsConfig: editable + editor_type (text, number, boolean) for category-products - GridConfigService: persist editable/editor_type in grid config - CategoryProductsGrid: double-click to edit, save on blur/Enter, skip save when unchanged - ProductDataService: allowedUpdateFields whitelist, allowedResourceFields for published - Lexicons ru/en: field_editable, editor_type, inline_edit_*, inline_edit_hint Fixes #116
biz87
left a comment
There was a problem hiding this comment.
Code Review: PR #134
Общее впечатление
PR хорошо структурирован. Whitelist в ProductDataService — важное улучшение безопасности (старый код передавал raw input в fromArray() без фильтрации). Lifecycle инлайн-редактирования продуман: double-click → edit → blur/Enter saves, Escape cancels, unchanged values не шлют запрос.
Баг: autofocus не работает на динамически вставленных элементах
CategoryProductsGrid.vue:1029, 1040
<InputText ... autofocus />
<InputNumber ... autofocus />HTML-атрибут autofocus работает только при загрузке страницы. При динамической вставке элемента через v-if он не получит фокус. Пользователь двойным кликом откроет редактор, но курсор в инпуте не окажется — придётся кликать ещё раз.
Фикс: использовать Vue-директиву или ref + nextTick:
// В startInlineEdit(), после установки editingCell:
nextTick(() => {
const input = document.querySelector('.inline-edit-cell input')
if (input) input.focus()
})Или кастомная директива v-focus.
Баг: двойной вызов saveInlineEdit (Enter + blur)
CategoryProductsGrid.vue:1030-1031
@blur="saveInlineEdit(product, column)"
@keydown.enter="saveInlineEdit(product, column)"Когда пользователь нажимает Enter:
@keydown.enterвызываетsaveInlineEdit()→clearInlineEditState()→editingCell = null- Vue убирает
<InputText>из DOM (условиеv-else-ifбольше не true) - Удаление из DOM вызывает
blur→ второй вызовsaveInlineEdit()
Guard в начале saveInlineEdit (if (!editingCell.value || ...)) защищает от повторного API-вызова — второй вызов вернётся рано. Но при ошибке API (catch-блок делает return без clearInlineEditState), editingCell остаётся, и blur после Enter снова попытается сохранить.
Рекомендация: в @keydown.enter сначала сделать $event.target.blur() и убрать отдельный @keydown.enter handler, либо добавить флаг isSaving чтобы исключить параллельные вызовы.
Отсутствие MODX-событий при publish/unpublish
ProductDataService.php:501-512 — applyPublishedToResource()
Метод напрямую меняет published, publishedon, publishedby через set() + save(). MODX-события OnDocPublished / OnDocUnPublished не вызываются. Если плагины зависят от этих событий (кеширование, индексация, уведомления), они не сработают.
Рекомендация: Использовать $product->set('published', ...) + $product->save() с ручным вызовом событий, или как минимум оставить // TODO комментарий что события пропущены осознанно.
editor_options в whitelist но не используется
GridConfigService.php:190 — ключ editor_options добавлен в $configKeys. В лексиконах есть editor_type_select и editor_options. Но в UI select-редактор не реализован — editorTypeOptions содержит только text/number.
Не баг, но мёртвый конфиг-ключ. Если select планируется позже — стоит добавить // TODO: select editor комментарий.
Нет индикации загрузки при сохранении
CategoryProductsGrid.vue:420-428 — saveInlineEdit() делает await request.put(...) без визуального feedback'а. При медленном соединении пользователь может подумать что ничего не происходит и кликнуть снова.
Рекомендация (не блокер): добавить кратковременный loading-индикатор или disabled-состояние на ячейку.
Что сделано хорошо
- Whitelist
$allowedUpdateFields— критическое улучшение безопасности. Старый код передавал сырой input напрямую вfromArray() - Разделение resource/productData полей —
publishedобновляется через отдельный метод с правильной обработкойpublishedon/publishedby isInlineValueUnchanged()— грамотная оптимизация, пропускает API-вызов и тост если значение не изменилось- Guard в
saveInlineEdit— проверкаeditingCellв начале защищает от гонок - Лексиконы на двух языках — ru + en
isCategoryProductsGrid— inline edit показывается только для релевантного грида
Резюме
| Замечание | Серьёзность |
|---|---|
autofocus не работает динамически |
Баг (UX) |
| Enter + blur двойной вызов (edge case при ошибке) | Низкая |
| Нет MODX-событий при publish/unpublish | Средняя |
editor_options мёртвый ключ |
Косметика |
| Нет loading-индикатора | Nice to have |
…oductsGrid - Added loading state to prevent multiple save requests during inline editing. - Improved focus handling for inline edit inputs after DOM updates. - Updated saveInlineEdit function to handle saving state and prevent double invocation. - Adjusted UI to reflect saving state with visual cues. This update enhances user experience by ensuring smoother inline editing interactions.
biz87
left a comment
There was a problem hiding this comment.
Code Review: inline edit for category products table
+367 / -16 | 6 файлов | 2 коммита
Общая оценка
Фича добавляет инлайн-редактирование в таблице товаров категории (двойной клик по ячейке). Включает: UI в гриде, конфигурацию editable/editor_type в настройках колонок, whitelist-фильтрацию полей на бэкенде, обработку published через ивенты MODX.
1. Безопасность: whitelist — хорошо, но есть пробел
ProductDataService.php — до этого PR метод updateProductData() делал $productData->fromArray($data) без фильтрации. Теперь есть $allowedUpdateFields + array_intersect_key(). Это правильно и важно.
Однако: нет проверки прав на редактирование. Метод вызывается через /api/mgr/product-data/{id} — менеджерский API, доступ ограничен авторизацией. Но пользователь с view без save_document сможет редактировать. Стоит проверять $this->modx->hasPermission('save_document') или $product->checkPolicy('save').
2. Порядок сохранения: race condition между resource и productData
// Сначала сохраняется resource (published)
$updatedResource = $this->applyPublishedToResource($product, $published);
// Потом productData
if (!$productData->save()) {
return null; // resource уже сохранён!
}Если productData->save() упадёт, published уже изменён и сохранён. Это рассинхронизация — юзер получит ошибку, но published фактически изменился. Нужно сначала сохранять productData, потом resource. Или оба в транзакции.
3. Ответ API не используется на фронте
await request.put(`/api/mgr/product-data/${product.id}`, { [column.name]: value })
product[column.name] = value // локальное обновление нормализованным значениемОтвет сервера игнорируется. Если бэкенд трансформирует значение (округление цены, приведение типа), локальное состояние будет отличаться от серверного. Лучше:
const result = await request.put(...)
Object.assign(product, result.data) // или хотя бы product[column.name] = result.data[column.name]4. DOM-запрос для фокуса — хрупко
nextTick(() => {
const input = document.querySelector('.inline-edit-cell input')
if (input) input.focus()
})Глобальный document.querySelector найдёт первый .inline-edit-cell input на странице. Для PrimeVue InputNumber реальный <input> может быть вложен глубже. Надёжнее — ref на компоненте:
<InputText ref="inlineInput" ... />nextTick(() => inlineInput.value?.$el?.focus())5. @blur на InputNumber — потенциальная проблема PrimeVue
PrimeVue InputNumber оборачивает <input> в <span>. Событие @blur на компоненте может не пробрасываться корректно с внутреннего <input>. Нужно проверить, что save действительно вызывается при клике вне ячейки. Если нет — нужен @blur с .native или обработка через onBlur prop.
Аналогично @keydown.escape — может не долетать от вложенного input до компонента.
6. editor_options и editor_type_select — мёртвый код
'editor_options', // TODO: select editor - not yet implemented in UIЛексикон editor_type_select добавлен, editor_options сохраняется в конфиге, но UI для select-редактора нет. Это нормально для итеративной разработки, но стоит либо убрать до реализации, либо добавить issue. Сейчас это dead code + TODO в продакшн-коде.
7. Нет валидации значений на бэкенде
updateProductData() принимает данные и ставит через fromArray(). xPDO не валидирует значения — можно установить отрицательную цену, нечисловой stock и т.д. Минимальная валидация:
price/old_price>= 0stock— integerweight>= 0
8. Мелочи
(int)(bool)$resourceData['published']— двойное приведение.(int)$resourceData['published']достаточно если нужно 0/1. Или$resourceData['published'] ? 1 : 0для ясности.- Лексикон
inline_edit_hint— длинный текст в подсказке. Проверить что не ломает layout при узких экранах. cursor: textна.editable-cell— подсказывает юзеру, но визуально не отличает editable от non-editable ячеек. Лёгкий hover-эффект (background) был бы полезнее.
Итого
| Категория | Оценка |
|---|---|
| Идея / UX | ✅ Хорошо — инлайн-редактирование удобно |
| Безопасность | |
| Надёжность | |
| Код |
Рекомендация: исправить пункты 2, 3, 4, добавить проверку permissions (п.1) и минимальную валидацию (п.7).
- Implemented validation for price, old_price, stock, and weight fields to ensure they meet expected criteria before updates. - Enhanced updateProductData method to validate filtered data prior to saving. - Updated inline editing functionality in CategoryProductsGrid to improve focus handling for input elements.
biz87
left a comment
There was a problem hiding this comment.
Code Review
Хорошая фича, реализация в целом рабочая. Но есть ряд проблем разной степени критичности.
Критичные проблемы
1. Контроллер не различает причины ошибки
updateProductData() возвращает null для 5 разных ситуаций: нет прав, не найден продукт, не найдены данные, не прошла валидация, ошибка save. Контроллер на всё отвечает 500 "Failed to save". Клиент не может отличить ошибку прав (403) от невалидных данных (422) или отсутствия продукта (404).
2. Валидация stock не проверяет отрицательные значения
if (isset($filtered['stock']) && (int)$filtered['stock'] != $filtered['stock']) {
return false;
}Проверяет только «целое ли число», но stock = -10 пройдёт валидацию. При этом price, old_price и weight проверяются на < 0.
3. Частичное обновление без отката
Если productData->save() успешен, но applyPublishedToResource() падает — данные продукта уже сохранены, а published нет. Метод вернёт null, фронтенд покажет ошибку, хотя часть данных обновлена.
Архитектурные замечания
4. Ответ API — проверить структуру
Контроллер оборачивает результат в Response::success(['updated' => true, 'data' => $result]). Фронтенд делает:
const res = await request.put(...)
if (res?.data) Object.assign(product, res.data)Нужно убедиться, что res.data — это именно данные продукта, а не { updated: true, data: {...} }. Если обёртка лишняя, Object.assign перезапишет объект продукта полями updated и data.
Проблемы фронтенда
5. Race condition при быстром переключении ячеек
startInlineEdit не проверяет inlineEditSaving. Сценарий: blur на ячейке A запускает async-сохранение → пользователь dblclick на ячейку B → clearInlineEditState() из завершения сохранения A закроет редактирование B.
6. @keydown на InputNumber
PrimeVue InputNumber не всегда пробрасывает keydown. Нужно проверить, что Escape и Enter реально работают.
7. Object.assign(product, res.data) — мутация
Прямая мутация элемента массива через Object.assign может не вызвать реактивное обновление Vue в некоторых кейсах. Безопаснее обновлять через индекс массива.
Мелкие замечания
8. Неиспользуемые лексиконы
editor_type_select и editor_options добавлены в оба лексикона, но select-editor не реализован. Лучше не добавлять неиспользуемый код заранее.
9. isBooleanColumn — мёртвая ветка
column.editor_type === 'boolean'В UI нельзя выбрать editor_type = 'boolean' (только text/number). Эта проверка никогда не сработает.
Резюме
Основное перед мержем:
- Нормальные коды ошибок из сервиса → контроллер (403/404/422)
- Проверить структуру ответа API на фронтенде (
res.datavsres.data.data) - Добавить
stock < 0в валидацию - Защита от race condition при переключении ячеек
- Убрать неиспользуемые лексиконы
…uct updates - Enhanced the updateProductData method to return detailed error messages and codes for various failure scenarios. - Updated validation logic for stock to ensure it cannot be negative. - Adjusted inline editing functionality in CategoryProductsGrid to prevent race conditions during save operations. - Removed unused lexicon entries related to editor types in English and Russian translations. This update improves the robustness of product data handling and user experience during inline editing.
biz87
left a comment
There was a problem hiding this comment.
Code Review (повторный)
Все 9 замечаний из первого ревью исправлены в 4-м коммите.
Что исправлено
- HTTP-коды ошибок — контроллер возвращает 403/404/422/500 через structured response
stock < 0— валидация добавлена- Rollback при ошибке published —
$oldValuesсохраняются, откатываются при сбое - Ответ API —
Response::success($result['data'])напрямую, request.js корректно извлекает data - Race condition —
startInlineEditблокируется приinlineEditSaving - InputNumber keydown — wrapper div с
@keydown.enter.capture.prevent/@keydown.escape.capture.prevent - Реактивное обновление —
findIndex+ spread вместоObject.assign - Лексиконы —
editor_type_select,editor_optionsудалены isBooleanColumn— толькоcolumn.type === 'boolean'
Мелкое замечание (некритичное)
В updateProductData() переменная $fieldsToUpdate создаётся через array_intersect_key($filtered, array_flip(self::$allowedUpdateFields)), но $filtered уже отфильтрована тем же whitelist. Это дублирование — можно использовать $filtered напрямую.
LGTM
Описание
Реализовано быстрое редактирование данных прямо в таблице товаров категории (Category Products): по двойному клику ячейка переходит в режим редактирования (текст, число или чекбокс для boolean), сохранение по blur/Enter. Редактируемые поля и тип редактора настраиваются в «Утилиты → Поля таблицы» для грида «Товары категории». На бэкенде обновление идёт через PUT /api/mgr/product-data/{id} с белым списком полей; поле published обновляет ресурс msProduct.
Тип изменений
Связанные Issues
Closes #116
Как это было протестировано?
Включение «Редактируемое поле» и выбор типа редактора (текст/число) в настройках полей таблицы для грида «Товары категории».
Двойной клик по ячейке — появление инпута/чекбокса, ввод значения, сохранение по blur или Enter; при отсутствии изменений запрос не отправляется и тост «Изменения сохранены» не показывается.
Редактирование поля published (чекбокс) — переключение публикации товара с обновлением на бэкенде (msProduct).
Ручное тестирование
Автоматические тесты (PHPStan, ESLint)
Тестирование на разных версиях PHP/MODX
Конфигурация тестирования:
Скриншоты (если применимо)
Чеклист
Дополнительные заметки
publishedобновляется в ресурсе msProduct (published, publishedon, publishedby); остальные — в msProductData через существующий whitelist.