Skip to content

feat(extra-fields): repeater field type (ms3-repeater)#301

Open
Ibochkarev wants to merge 4 commits into
betafrom
feat/299-repeater-field
Open

feat(extra-fields): repeater field type (ms3-repeater)#301
Ibochkarev wants to merge 4 commits into
betafrom
feat/299-repeater-field

Conversation

@Ibochkarev

Copy link
Copy Markdown
Member

Описание

Добавлен тип extra field «повторитель» (ms3-repeater): JSON-массив объектов со схемой колонок, drag-and-drop сортировкой строк и автоматическим проставлением rank при сохранении (MIGX-паттерн).

Backend: миграция repeater_config, RepeaterFieldService, интеграция в ExtraFieldsService, ProductDataService (исключение из option-sync) и OrdersController (422 при invalid payload).

Vue Manager: RepeaterField, редактор схемы в ExtraFieldsManager, OrderExtraFieldsSection для msOrder/msOrderAddress.

Тип изменений

  • Исправление бага (non-breaking change)
  • Новая функциональность (non-breaking change)
  • Breaking change
  • Рефакторинг
  • Документация

Связанные Issues

Closes #299
Refs #298

Как это было протестировано?

  • Ручное тестирование (ExtraFieldsManager → product card / order / address)
  • Автоматические проверки (php -l, ESLint на изменённых Vue/JS)
  • Тестирование на разных версиях PHP/MODX

Конфигурация тестирования:

  • MiniShop3: feat/299-repeater-field
  • MODX: 3.x
  • PHP: 8.2+

Чеклист

  • Код соответствует стилю проекта
  • Лексиконы ru/en
  • ESLint / php -l на затронутых файлах
  • CHANGELOG (maintainer)

Дополнительные заметки

Repeater исключён из prepareOptionValues и saveOptions для msProductData. Order/address extra fields получили UI через OrderExtraFieldsSection. Grid sort/filter по JSON-path — follow-up из issue.

Для ревьюера: перед merge нужна миграция Phinx и npm run build в vueManager на стенде.

@Ibochkarev Ibochkarev requested a review from biz87 May 24, 2026 16:49
@Ibochkarev Ibochkarev self-assigned this May 24, 2026
@biz87

biz87 commented Jun 14, 2026

Copy link
Copy Markdown
Member

Спасибо за PR — фича архитектурно правильная и закрывает большой класс кейсов (характеристики товара, варианты в карточке, наборы файлов, бонусные уровни и т.п.). Прошёлся по backend, прогнал PHPStan. Есть один блокер и пара замечаний.

🔴 Блокер — регрессия PR #310

Ветка отрезалась от beta до мержа #310 (вчера). В текущем виде PR при мерже откатит свежезамёрженный фикс контракта Manager API. Сравнил с актуальным origin/beta:

OrdersController::create() addressFields (≈ строка 645):

Текущий beta:

if (array_key_exists($field, $params)) { ... }

В PR #301:

if (isset($params[$field])) { ... }    // откат

OrdersController::updateProduct() (≈ строки 1147–1180):

В текущем beta — switch ($field) + $nullClearable = ['options'] + явный null-skip для count/price/weight.

В PR #301 — старый if (isset(...)) с цепочкой if-ов. Это та самая логика, которую #310 заменил, чтобы partial payload с count: null или price: null не обнулял позиции заказа.

ExtraFieldsService::updateField():

В текущем beta — $nullClearable = ['description', 'select_options'] с per-field null semantics.

В PR #301 — старый if (isset($data[$fieldName])). Откат.

Что нужно: git rebase origin/beta, разрешить конфликты с сохранением и новых полей (repeater_config в allowed list, applyRepeaterConstraints, новые методы), и per-field null логики из #310. Концептуально изменения не пересекаются — просто разный snapshot базы.

Без rebase мерж нельзя — это публичный регресс контракта API через сутки после фикса.

Что хорошо (после исправления регрессии)

  • Миграция add_repeater_config_to_extra_fields — Phinx API, unprefixed name, идемпотентна через hasColumn. Без замечаний.
  • RepeaterFieldService — изолирован, понятный pipeline decode → normalize → validate, типизированные docblock'и, throws с осмысленными сообщениями.
  • Whitelist через array_column($config['columns'], 'key') в sanitizeRow — нельзя пропихнуть лишние ключи в строку. Защита от malicious payload работает.
  • Auto rank от 0 при normalize — не зависит от исходного порядка ключей в payload.
  • 422 на invalid repeater payload в OrdersController::updateOrder() и ProductDataService::updateProductDataFields() — правильный HTTP-код, понятная ошибка вместо тихого сохранения мусора.
  • Расширение width до 12 для repeater extra field — корректно для UX (таблица на пол-ширины не смотрелась бы).
  • PHPStan чист по всем backend-файлам.

🟡 Минорные замечания

1. Hardcoded class в getProductRepeaterFields

$this->productRepeaterFields = $this->getRepeaterFieldService()->getRepeaterFieldsForClass(
    'MiniShop3\Model\msProductData'
);

Если repeater задумывался расширяемым на другие модели — стоит вынести в константу/метод. Не блокер, но запах.

2. Лог не пишется при невалидном repeater_config в БД

RepeaterFieldService::parseConfig() на повреждённом JSON молча возвращает default. Безопасно для прод-пути, но при дебаге («почему у меня нет колонок?») следов нет. Хотя бы LOG_LEVEL_WARN с указанием field key.

3. Dead check в validateRows

processValue вызывает normalizeRows (которая обрезает по maxRows), затем validateRows (которая проверяет count > maxRows). После normalize этот check не может сработать. Если validateRows — standalone-API, ок. Иначе можно упростить.

4. Кеш $fieldsByClass request-scoped, без инвалидации

Если в течение одного запроса админ удалил/изменил repeater extra field и сразу сохранил товар — увидит старую конфигурацию. На практике редко (как правило, разные сессии), но стоит документировать ограничение или предусмотреть reset().

Vue / Frontend — нужна ручная проверка

Не разбирал детально 8 Vue-файлов (+600 строк), но в чеклисте PR не отмечен «ручное тестирование» — а это критично для UI-фичи такого объёма. До мержа стоит прокликать и отчитаться:

  • Создание repeater extra field через RepeaterSchemaEditor (добавить/удалить колонки, задать xtype/required).
  • В карточке товара: добавить/удалить/перетащить строки, сохранить, перезагрузить — данные на месте.
  • То же в форме заказа (OrderExtraFieldsSection) и адреса.
  • Валидация на min/max rows, required columns — сообщение пользователю понятное.
  • Корнер-кейсы: пустая таблица, одна строка, очень много строк (~50), длинные значения.

Резюме

Обязательно до мержа:

  1. Rebase на текущий beta с сохранением логики fix(api): per-field null payload semantics in Manager API (#289) #310 в OrdersController и ExtraFieldsService::updateField. Это блокер.
  2. Ручное тестирование UI — прокликать и отчитаться о результатах.

Желательно:

  1. Лог-предупреждение при невалидном repeater_config.
  2. Hardcoded model class — в константу или метод.

После rebase и UI-теста — мержим. Фича сильная, в CHANGELOG пойдёт топовым пунктом релиза.

@Ibochkarev Ibochkarev force-pushed the feat/299-repeater-field branch from b3e2980 to a66b180 Compare June 14, 2026 10:17
Ibochkarev added a commit that referenced this pull request Jun 14, 2026
- Warn in error log when repeater_config JSON is malformed (with field key)
- Use msProductData::class instead of hardcoded string in ProductDataService
- Drop redundant maxRows check in validateRows after normalizeRows enforces limit
@Ibochkarev

Copy link
Copy Markdown
Member Author

@biz87 можно смотреть повторно

@biz87

biz87 commented Jun 14, 2026

Copy link
Copy Markdown
Member

Прокликал фичу на dev-стенде. Дополняю первый ревью результатами ручного теста — нашёл два блокера в самом RepeaterField, не связанных с rebase #310.

🔴 Блокер 1 — фокус сбрасывается на каждый keystroke

vuedraggable использует :item-key="_ms3RowId" для дифа строк. В repeaterField.js:64 _ms3RowId генерится так:

normalized._ms3RowId = row?._ms3RowId ?? `row-${index}-${Date.now()}`

И stripRepeaterRowMeta стирает _ms3RowId перед emit'ом наверх (правильно — это служебка). Но дальше:

  1. Юзер печатает символ в ячейке → updateCellemitRows() → emit без _ms3RowId
  2. Родитель обновляет modelValue
  3. Watch на props.modelValue (deep) срабатывает → internalRows = normalizeRepeaterRows(...)
  4. На входе у normalizeRepeaterRows массив без _ms3RowId (родитель же стёр) → fallback row-${index}-${Date.now()}новый ID на каждый keystroke
  5. vuedraggable видит новый key → пересоздаёт DOM строки → input теряет фокус

Фикс — guard в watch от собственного emit'а:

let isInternalEmit = false

function emitRows() {
  const normalized = normalizeRepeaterRows(internalRows.value, schema.value)
  isInternalEmit = true
  emit('update:modelValue', stripRepeaterRowMeta(normalized, schema.value))
}

watch(
  [() => props.modelValue, () => props.config],
  () => {
    if (isInternalEmit) {
      isInternalEmit = false
      return
    }
    internalRows.value = normalizeRepeaterRows(
      parseRepeaterModelValue(props.modelValue),
      schema.value
    )
  },
  { immediate: true, deep: true }
)

Затрагивает оба столбца, но если один из них — numberfield, баг там менее заметен (InputNumber коммитит на blur).

🔴 Блокер 2 — сохранение не работает

Заполнил поле в карточке товара ID 152351, нажал MODX Save в шапке, перезагрузил — поле пустое. В БД проверил:

SELECT id, IFNULL(repeater,'NULL') FROM modx_ms3_products WHERE id = 152351;
+--------+----------+
| id     | repeater |
+--------+----------+
| 152351 | []       |
+--------+----------+

То есть колонка получила пустой массив, а не введённые строки.

Причина — отсутствие bridge между Vue-стейтом и legacy формой. В DynamicField.vue:185-191:

<RepeaterField
  v-else-if="fieldConfig.xtype === 'ms3-repeater'"
  v-model="localValue"
  :config="repeaterConfig"
  :disabled="disabled"
/>

Нет <input type="hidden" :name="fieldConfig.name" :value="...">, который есть для других xtype (например, ms3-combo-select на строке 182). Значение repeater'а живёт только в Vue, в legacy POST не попадает.

Что происходит при Save:

  1. POST legacy формы — без repeater в payload.
  2. modResource Update процессор → fromArray($properties)msProduct::set() для repeater не вызывается → в памяти остаётся NULL (для нового товара).
  3. msProductData::save()prepareObject($this) (msProductData.php:78) → getArraysValues() находит repeater среди json-полей.
  4. processValue(null, config)[]set('repeater', []) → в БД пишется [].

То есть prepareObject молча «нормализует» отсутствие данных в пустой массив. Пользователь видит «сохранено», на reload — пусто.

Фикс — добавить hidden input в DynamicField:

<RepeaterField ... />
<input
  v-if="fieldConfig.xtype === 'ms3-repeater'"
  type="hidden"
  :name="fieldConfig.name"
  :value="JSON.stringify(localValue || [])"
/>

xPDO с phptype='json' принимает строку, декод/энкод сделает сам.

🟡 UX-замечания (не блокеры)

1. Конфигуратор не помещается в модалку. В RepeaterSchemaEditor.vue:185-190 сетка column-row фиксирована 1fr 1fr 10rem auto 2.5rem, на узких модалках обрезается. Брейкпоинт 48rem (768px) ловит только мобильные. Поднять до 64rem — или сразу схлопывать в одну колонку при сжатии.

2. «Поле rank» вынесено первым в форме. Это служебная настройка имени ключа для авто-сортировки (по аналогии с MIGX). 99% пользователей не должны её видеть. Спрятать в свёрнутый блок «Расширенные настройки» или вообще убрать из UI, оставить хардкод rank в коде.

3. <style scoped> в RepeaterSchemaEditor.vue и RepeaterField.vue. По нашему опыту с MS3 Vue: scoped стили молча ломаются между Vite chunk'ами из-за hash mismatch. Использовать non-scoped с .vueApp префиксом — vite.config через postcss-prefix-selector всё равно его допишет.

Frontend (public output)

Поле в БД лежит как JSON-строка с phptype='json'. В sniippets MS3 (ms3_products и др.) — через pdoTools, которая $object->get() не вызывает и возвращает строки assoc-массивом. То есть в Fenom-шаблоне разработчик получит сырую JSON-строку и должен явно декодить ({var $rows = $specs | json_decode : true}).

Это MIGX-паттерн, который сообщество знает и принимает. Не блокер, но стоит явно зафиксировать в документации к фиче, чтобы не было сюрприза «почему не работает {$product.specs}».

Итоговый список блокеров для PR #301

  1. Rebase на текущий beta — иначе откатит контракт null payload из fix(api): per-field null payload semantics in Manager API (#289) #310.
  2. Focus loss в RepeaterField — фикс в emit-guard, см. выше.
  3. Сохранение не работает — добавить hidden input в DynamicField для ms3-repeater.

UX-полировка и документирование public output — желательно, но мерж не блокирует.

После фикса этих трёх — пересмотрю PR, прогоню повторно. Готов помочь с реализацией, если нужно.

…d address

Introduces repeater extra fields with schema-driven rows, DnD sorting, rank normalization, and manager UI across product data and order screens.

Closes #299
… saves

Validate repeater rows after normalization, return 422 from product-data API on invalid payload, and strip client-only row metadata before submit.
- Warn in error log when repeater_config JSON is malformed (with field key)
- Use msProductData::class instead of hardcoded string in ProductDataService
- Drop redundant maxRows check in validateRows after normalizeRows enforces limit
@Ibochkarev Ibochkarev force-pushed the feat/299-repeater-field branch from f52e4d9 to 025ccb1 Compare June 15, 2026 04:40
@biz87

biz87 commented Jun 15, 2026

Copy link
Copy Markdown
Member

Прогресс заметный — закрыто два из трёх блокеров. Но главный остался.

✅ Закрыто

1. Rebase #310 — в OrdersController сохранена логика per-field null payload ($nullClearable = ['options']), CLEAN merge status.

2. Focus loss — починен через isInternalEmit guard в RepeaterField.vue. Точно тот pattern, что я предлагал. Watch теперь не реагирует на собственные emit'ы, DOM остаётся стабильным, фокус не теряется.

🔴 Не закрыто — save всё ещё не персистит

Это критический блокер. У него два корня, ни один не закрыт. Без починки фичу нельзя использовать, что бы внешне ни выглядело корректно.

Корень 1 — DynamicField.vue не пробрасывает значение в legacy POST

<RepeaterField
    v-else-if="fieldConfig.xtype === 'ms3-repeater'"
    v-model="localValue"
    :config="repeaterConfig"
    :disabled="disabled"
/>

Hidden input для ms3-repeater отсутствует. Другие сложные xtype (например ms3-combo-select на той же странице) имеют hidden input для bridge между Vue-стейтом и legacy формой.

Последствие: при сохранении товара через MODX Save в шапке, значение repeater'а живёт только в Vue, в $_POST не попадает. Trait ProductDataPayloadTrait из недавно вмёрженного #298 не получает данных — он читает $this->getProperties(), а там пусто.

Фикс — добавить hidden input в DynamicField.vue:

<RepeaterField ... />
<input
    v-if="fieldConfig.xtype === 'ms3-repeater'"
    type="hidden"
    :name="fieldConfig.name"
    :value="JSON.stringify(localValue || [])"
/>

После этого save через MODX Save → modResource Update → trait #298 увидит repeater в properties и корректно сохранит через persistProductDataPayload().

Корень 2 — ProductDataService::updateProductData() whitelist drop

protected static array $allowedUpdateFields = [
    'article', 'price', 'old_price', 'stock', 'weight',
    'vendor_id', 'made_in', 'new', 'popular', 'favorite',
];
// ...
$filtered = array_intersect_key($data, array_flip(self::$allowedUpdateFields));
// repeater тут уже выкинут — его нет в статическом whitelist
$fieldsToUpdate = array_intersect_key($filtered, array_flip(self::$allowedUpdateFields));

$repeaterError = $this->normalizeRepeaterFieldsInPayload($fieldsToUpdate);
// ... вызывается на массиве без repeater — срабатывает вхолостую

Затем prepareObject() в save() нормализует null → [] и пишет в БД пустой массив. Иллюзия успешного сохранения.

Воспроизводил вчера на товаре 152351 — в repeater колонке записался [], остальные json-поля (tags, color, size) содержали реальные данные.

Фикс — расширить whitelist динамически:

$extraFieldKeys = array_keys($this->getProductRepeaterFields());
$allowedFields = array_merge(self::$allowedUpdateFields, $extraFieldKeys);
$filtered = array_intersect_key($data, array_flip($allowedFields));

Это закроет save через inline-таб ProductDataFields.vue (PUT /api/mgr/product-data/{id}).

Нужны оба фикса

Эти два пути ходят разными ручками и оба используются в разных местах админки. Починить нужно оба.

🟡 UX и стиль — пре-existing замечания

Не критично, остаётся как было:

  • <style scoped> в RepeaterField.vue и RepeaterSchemaEditor.vue — по нашему опыту с MS3 Vue scoped стили молча ломаются между Vite chunk'ами (hash mismatch).
  • Сетка column-row в schema editor: брейкпоинт 48rem не ловит typical modal width, конфигуратор не помещается.
  • Поле rank всё ещё на верхнем уровне формы редактора схемы (предлагал спрятать в advanced).

Эти три — follow-up'ы, не блокеры.

Резюме

Без починки save — фичу нельзя выкатывать. После двух правок (hidden input + whitelist) — пересмотрим.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] тип поля повторитель

2 participants